[openib-general] [PATCH][RFC] kDAPL: remove dat wrapper funct ion dat_ia_query()
Itamar Rabenstein
itamar at mellanox.co.il
Thu Jun 9 01:37:25 PDT 2005
Hi Tom,
It looks very good to me but i want to ask why do we need the dat_common
struct ?
as we can see in dat.h dat_common is defined as :
struct dat_common {
struct dat_provider *provider;
union dat_context context;
};
and is being use as data member of any dat_* struct
There are 9 places were we instance dat_common
(dat_ia, dat_ep, dat_evd, ...)
i think that that dat_* struct should be like this
struct dat_ia {
struct dat_provider *provider;
union dat_context context;
};
instaed of
struct dat_ia {
struct dat_common common;
};
if we do it (only 5 lines more in code )
every time we want to call a function it will be
ret = ia->provider->dat_ia_query(ia, NULL,
instaed of
ret = ia->common.provider->dat_ia_query(ia, NULL,
i think that this is more readable.
and i agree that we dont need the dat_ before the functions name
Itamar
> -----Original Message-----
> From: Tom Duffy [mailto:tduffy at sun.com]
> Sent: Thursday, June 09, 2005 4:40 AM
> To: Grant Grundler
> Cc: openib-general at openib.org
> Subject: Re: [openib-general] [PATCH][RFC] kDAPL: remove dat wrapper
> function dat_ia_query()
>
>
> On Wed, 2005-06-08 at 17:07 -0700, Grant Grundler wrote:
> > > - ret = dat_ia_query (test_ptr->ia,
> > > - NULL,
> > > - &test_ptr->ia_attr,
> > > - NULL);
> > > + ret =
> test_ptr->ia->common.provider->dat_ia_query(test_ptr->ia, NULL,
> > > +
> &test_ptr->ia_attr, NULL);
> >
> > Maybe not reference test_ptr in this line so often?
> > It could read:
> > ret = ia->common.provider->dat_ia_query(ia, NULL,
> &test_ptr->ia_attr,
> > NULL);
>
> Yup, cleaner. New patch attached.
>
> > > - .ia_query_func = &dapl_ia_query,
> > > + .dat_ia_query = &dapl_ia_query,
> >
> > Yes. I like this alot better too.
>
> Or should the name in dat_provider just be ia_query? Any need for the
> dat_?
>
> Signed-off-by: Tom Duffy <tduffy at sun.com>
>
> Index: linux-kernel-return/test/dapltest/test/dapl_performance_util.c
> ===================================================================
> ---
> linux-kernel-return/test/dapltest/test/dapl_performance
> _util.c (revision 2573)
> +++
> linux-kernel-return/test/dapltest/test/dapl_performance
> _util.c (working copy)
> @@ -64,10 +64,7 @@ DT_Performance_Test_Create (
> test_ptr->ia = ia;
> test_ptr->cmd = &pt_ptr->Params.u.Performance_Cmd;
>
> - ret = dat_ia_query (test_ptr->ia,
> - NULL,
> - &test_ptr->ia_attr,
> - NULL);
> + ret = ia->common.provider->dat_ia_query(ia, NULL,
> &test_ptr->ia_attr, NULL);
> if ( DAT_SUCCESS != ret)
> {
> DT_Tdep_PT_Printf (phead, "Test[" F64x "]: dat_ia_query
> error: %s\n",
> Index: linux-kernel-return/test/dapltest/test/dapl_fft_queryinfo.c
> ===================================================================
> ---
> linux-kernel-return/test/dapltest/test/dapl_fft_queryinfo.c
> (revision 2573)
> +++
> linux-kernel-return/test/dapltest/test/dapl_fft_queryinfo.c
> (working copy)
> @@ -165,10 +165,8 @@ int DT_queryinfo_basic (Params_t *params
> {
> if (result_wanted == DAT_SUCCESS)
> {
> - rc = dat_ia_query (ia,
> - &evd,
> - &ia_attributes,
> - &provider_attributes);
> + rc = ia->common.provider->dat_ia_query(ia, &evd,
> &ia_attributes,
> +
> &provider_attributes);
> }
> else if (result_wanted == DAT_INVALID_PARAMETER)
> {
> @@ -177,17 +175,13 @@ int DT_queryinfo_basic (Params_t *params
> * NULL out ia_attr and for the DAT_IA_ATTR_MASK to
> * have values
> */
> - rc = dat_ia_query (ia,
> - &evd,
> - NULL,
> - &provider_attributes);
> + rc = ia->common.provider->dat_ia_query(ia, &evd, NULL,
> +
> &provider_attributes);
> }
> else if (result_wanted == DAT_INVALID_HANDLE)
> {
> - rc = dat_ia_query (ia,
> - &evd,
> - &ia_attributes,
> - &provider_attributes);
> + rc = ia->common.provider->dat_ia_query(ia, &evd,
> &ia_attributes,
> +
> &provider_attributes);
> }
> }
>
> Index: linux-kernel-return/test/dapltest/test/dapl_test_util.c
> ===================================================================
> --- linux-kernel-return/test/dapltest/test/dapl_test_util.c
> (revision 2573)
> +++ linux-kernel-return/test/dapltest/test/dapl_test_util.c
> (working copy)
> @@ -44,10 +44,9 @@ DT_query ( Per_Test_Data_t *pt_ptr,
> phead = pt_ptr->Params.phead;
>
> /* Query the IA */
> - ret = dat_ia_query (ia,
> - &async_evd_hdl,
> - &pt_ptr->ia_attr,
> - &pt_ptr->provider_attr);
> + ret = ia->common.provider->dat_ia_query(ia, &async_evd_hdl,
> + &pt_ptr->ia_attr,
> + &pt_ptr->provider_attr);
> if (ret != DAT_SUCCESS)
> {
> DT_Tdep_PT_Printf (phead, "%s: dat_ia_query error: %s\n",
> Index: linux-kernel-return/dat-provider/dapl_provider.c
> ===================================================================
> --- linux-kernel-return/dat-provider/dapl_provider.c (revision 2573)
> +++ linux-kernel-return/dat-provider/dapl_provider.c (working copy)
> @@ -67,7 +67,7 @@ static struct dat_provider g_dapl_provid
> .extension = NULL,
>
> .ia_open_func = &dapl_ia_open,
> - .ia_query_func = &dapl_ia_query,
> + .dat_ia_query = &dapl_ia_query,
> .ia_close_func = &dapl_ia_close,
> .ia_memtype_hint_func = &dapl_ia_memtype_hint,
>
> Index: linux-kernel-return/dat/api.c
> ===================================================================
> --- linux-kernel-return/dat/api.c (revision 2573)
> +++ linux-kernel-return/dat/api.c (working copy)
> @@ -405,8 +405,8 @@ u32 dat_ia_close(struct dat_ia *ia, enum
> provider = ia->common.provider;
> ia_name = provider->device_name;
>
> - if (DAT_SUCCESS != (status = dat_ia_query(ia, NULL, NULL,
> - &provider_attr)))
> + if (DAT_SUCCESS != (status =
> ia->common.provider->dat_ia_query(ia, NULL, NULL,
> +
> &provider_attr)))
> dat_dbg_print(DAT_DBG_TYPE_CONSUMER_API,
> "dat_ia_query for IA %s
> failed\n", ia_name);
> else if (DAT_SUCCESS != (status =
> provider->ia_close_func(ia, flags)))
> Index: linux-kernel-return/dat/dat.h
> ===================================================================
> --- linux-kernel-return/dat/dat.h (revision 2573)
> +++ linux-kernel-return/dat/dat.h (working copy)
> @@ -1057,10 +1057,6 @@ typedef u32 (*DAT_IA_OPENV_FUNC)(const c
>
> typedef u32 (*DAT_IA_CLOSE_FUNC)(struct dat_ia *, enum
> dat_close_flags);
>
> -typedef u32 (*DAT_IA_QUERY_FUNC)(struct dat_ia *, struct dat_evd **,
> - struct dat_ia_attr *,
> - struct dat_provider_attr *);
> -
> typedef u32 (*DAT_CR_QUERY_FUNC)(struct dat_cr *, struct
> dat_cr_param *);
>
> typedef u32 (*DAT_CR_ACCEPT_FUNC)(struct dat_cr *, struct
> dat_ep *, int,
> @@ -1223,7 +1219,8 @@ struct dat_provider {
> void *extension;
>
> DAT_IA_OPEN_FUNC ia_open_func;
> - DAT_IA_QUERY_FUNC ia_query_func;
> + u32 (*dat_ia_query)(struct dat_ia *, struct dat_evd **,
> + struct dat_ia_attr *, struct
> dat_provider_attr *);
> DAT_IA_CLOSE_FUNC ia_close_func;
> DAT_IA_MEMTYPE_HINT_FUNC ia_memtype_hint_func;
>
> @@ -1396,14 +1393,6 @@ static inline u32 dat_ia_memtype_hint(st
> preferred_alignment);
> }
>
> -static inline u32 dat_ia_query(struct dat_ia *ia, struct
> dat_evd **async_evd,
> - struct dat_ia_attr *ia_attr,
> - struct dat_provider_attr *provider_attr)
> -{
> - return DAT_CALL_PROVIDER_FUNC(
> - ia_query_func, ia, async_evd, ia_attr,
> provider_attr);
> -}
> -
> static inline u32 dat_cr_accept(struct dat_cr *cr, struct dat_ep *ep,
> int private_data_size, const
> void *private_data)
> {
>
> _______________________________________________
> openib-general mailing list
> openib-general at openib.org
> http://openib.org/mailman/listinfo/openib-general
>
> To unsubscribe, please visit
http://openib.org/mailman/listinfo/openib-general
More information about the general
mailing list