Re: [PATCH v3 1/2] libibnetdisc: Convert to a multi-smp algorithm

[Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>]

On Tue, 11 May 2010 09:42:34 -0700
Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:

> On 13:53 Mon 10 May     , Ira Weiny wrote:
> > > > 
> > > >    int ibnd_discover_fabric(ibnd_fabric_t **fabric,
> > > > 			    cosnt char *ca_name,  <== could we even default this?
> > > 
> > > I would think about ca_name and port_number. And this is of course may
> > > have default (NULL, 0).
> > 
> > Ok, ca_name and ca_port will be explicit.
> > 
> > > 
> > > > 			    struct ibnd_config *cfg);
> > > 
> > > What is wrong with current ibdn_fabric_t *ibnd_discover_fabric(...)? Why
> > > do we need an extra parameter?
> > 
> > Well we are breaking the interface again so I figure we might as well clean some things up.  Returning an int allows us to have a reason for the failure returned to the caller rather than just "NULL".  We have cleaned up most of the internals of the library to allow for this.
> 
> But we want to keep API simple, no?

Ok, patch to follow.

> 
> > 
> > > 
> > > > 
> > > > I don't mind the ibnd_config_t struct but I don't think it should be visible
> > > > to the user.  Make it opaque and use "set" functions.  Something like.
> > > > 
> > > > ibnd_fabric_t *fabric;
> > > > ibnd_config_t cfg;
> > > > ib_portid_t * from;
> > > > 
> > > > ibnd_set_hops(&cfg, hops);         <== default -1
> > > > ibnd_set_port_num(&cfg, port_num); <== default 1
> > > > ibnd_set_max_smps(&cfg, max_smps); <== default 2
> > > > ibnd_set_from_node(&cfg, from);    <== default NULL
> > > 
> > > I would prefer to not complicate API with ibnd_set_this() helpers. It
> > > would be necessary to add new ones in the future which will lead to API
> > > changes.
> > 
> > See below.
> > 
> > > 
> > > > if (ibnd_discover_fabric(&fabric, "foo", &cfg)) {  <== anything not in cfg is
> > > >                                                        defaulted here
> > > >    fprintf(stderr, "Wow it failed\n");
> > > > }
> > > > 
> > > > This allows us to change ibnd_config structure any time we want without
> > > > affecting the API.  I don't think the "pad" you used is a good idea.
> > > 
> > > Without padding we will break ABI each time when new field is added to
> > > the config structure.
> > 
> > No it does not iff you use the ibnd_set_this() helpers and make the config private.
> 
> In you example 'ibnd_config_t cfg' is on the stack... :)

yes, bad example.

> 
> I would really suggest to keep API simple and useful - to fill up
> a structure described in header files is much simpler than pass over
> various helpers calls.

I disagree but since you are looking to branch I think this bug needs to be fixed.

A patch is following this email,
Ira

> 
> Sasha
> 
> > 
> > > 
> > > > Also since we are breaking the API we might as well return the fabric as a
> > > > parameter and have an error code.  But I could go either way on this one.
> > > > 
> > > > Ira
> > > > 
> > > > 
> > > > [*] query_smp.c probably should have it's own timeout here but we can discuss
> > > > later.
> > > > 
> > > > [#] What sucks about this is that libibmad already has the functionality to
> > > > open the umad port and configure it (50 line function).  Now we will be
> > > > duplicating this functionality.
> > > 
> > > I think you can use mad_rpc_open_port(ca_name, port_number, ...) just
> > > fine (and so the rest of libibmad stuff) - it will open separate fd.
> > 
> > Yes for the libibmad functionality we can do that.  I was speaking of the use of the umad layer.  To use that layer we have to duplicate the functionality of mad_rpc_open_port in every tool which requires umad layer access, correct?  Right now we are mixing the 2 layers (mad and umad) in saquery (get_bind_handle) as well as libibnetdisc.
> > 
> > I think we need to be careful we don't do this again!
> > 
> > Ira
> > 
> > > 
> > > Sasha
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  http://**vger.kernel.org/majordomo-info.html
> > > 
> > 
> > 
> > -- 
> > Ira Weiny
> > Math Programmer/Computer Scientist
> > Lawrence Livermore National Lab
> > 925-423-8008
> > weiny2-i2BcT+NCU+M@public.gmane.org
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://*vger.kernel.org/majordomo-info.html
> 


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html