[ofa-general] Re: [PATCH V2 1/3] Create a new library libibnetdisc

Ira Weiny weiny2 at llnl.gov
Fri Jan 23 14:20:16 PST 2009


Sasha,

It sounds like we are in agreement with this architecture.

> >                        +--------+                     
> >                       -| diag 1 |-                    
> >                      / +--------+ \                   
> >   +-----------------+  +--------+  +-----------------+
> >   |  libibnetdisc   | -| diag 2 |--|  libibmad       |
> >   +-----------------+  +--------+  +-----------------+
> >               \                          /
> >                --------------------------

I am going to integrate your comments with a reworked version of the patches.
In the mean time I and/or Al will be submitting some patches for the libibmad
interface.

Ira

On Wed, 21 Jan 2009 14:07:27 +0200
Sasha Khapyorsky <sashak at voltaire.com> wrote:

> Hi Ira,
> 
> On 11:34 Tue 23 Dec     , Ira Weiny wrote:
> > > 
> > > What is the reason to redeclear custom NodeInfo and PortInfo structures?
> > > The original are defined by IBA and there are lot of utilities to work
> > > with them. Wouldn't it be better to use it as is?
> > >
> > 
> > [DISCLAIMER] First I want to answer your questions directly.  However, after
> >              writing this information, discussing with Al, and thinking it
> >              over.  I think I see where you are coming from and I _may_ agree
> >              with you.  So after reading these responses please read my
> >              thoughts regarding libibmad and this new lib.
> > 
> > 
> > I have 3 reasons I did it this way:
> > 
> >    1) This is pretty much the way that ibnetdiscover did things
> >       (By using mad_decode_field into these single fields)
> > 
> >    2) This makes libibnetdisc only dependent on libibmad rather than the OpenSM
> >       libs.  We have had some people complain that the diags require opensm-*
> >       things to be installed.  (This assumes you want to use ib_port_info_t from
> >       ib_types.h)
> 
> I thought about using libibmad rather than OpenSM stuff (in
> libibnetdisc), so PortInfo, NodeInfo and other SM attributes will be
> represented just as raw data and libibmad (or any other MAD interpreter)
> will be used by diag/whatever tool which uses libibnetdesc for decoding.
> 
> >    3) This structure is in host byte order and calls out each field
> >       independently rather than having to have intimate knowledge of the
> >       PortInfo wire packet.
> >       
> >       For example this is the code used in iblinkinfo with the above structure.
> > 
> > 
> > 		snprintf(link_str, 256,
> > 			"(%3s %s %6s/%8s)",
> > 			ibnd_linkwidth_str(port->info.link_width_active),
> > 			ibnd_linkspeed_str(port->info.link_speed_active, 1),
> > 			ibnd_linkstate_str(port->info.link_state),
> > 			ibnd_physstate_str(port->info.phys_state)
> > 			);
> > 
> > Here is the code if you use the ib_port_info_t from ib_types.h
> > 
> > 		snprintf(link_str, 256,
> > 			"(%3s %s %6s/%8s)",
> > 			ibnd_linkwidth_str(port->info.link_width_active),
> > 			ibnd_linkspeed_str(IB_PORT_LINK_SPEED_ACTIVE_MASK(port->info.link_speed), 1),
> > 			ibnd_linkstate_str(IB_PORT_STATE_MASK(port->info.state_info1)),
> > 			ibnd_physstate_str(IB_PORT_PHYS_STATE_MASK(port->info.state_info2) >> IB_PORT_PHYS_STATE_SHIFT)
> >                        //   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >                        //   This is particularly nasty compared to the above.
> > 			);
> > 
> > I no longer agree with reason 1 and 2.
> 
> Ok.
> 
> > However, reason 3 I believe is enough
> > justification to declare a new type.
> > 
> >    [DISCLAIMER] item 3 might be a mute point as well if you redefine what
> >    libibnetdisc is supposed to be.  See below.
> 
> Ok. See below.
> 
> > > > + * Str convert functions
> > > > + */
> > > > +char          *ibnd_linkwidth_str(int link_width);
> > > > +char          *ibnd_linkstate_str(int link_state);
> > > > +char          *ibnd_physstate_str(int phys_state);
> > > > +const char    *ibnd_node_type_str(ibnd_node_t *node);
> > > > +const char    *ibnd_node_type_str_short(ibnd_node_t *node);
> > > > +char          *ibnd_linkspeed_str(int link_speed, int data_rate);
> > > > +	/* if data_rate == 0 use "SDR", "DDR", etc. */
> > > > +	/* if data_rate == 1 use "2.5 Gbps", "5.0 Gbps", etc. */
> > > 
> > > Similar functions exist in libibmad. Why do we need another set?
> > 
> > 2 reasons.
> > 
> >    1) The strings returned are not compatible with the current output of
> >       ibnetdiscover and iblinkinfo...  I was trying to make sure that the
> >       library returned string which were backwards compatible.  That is
> >       actually the reason for the extra "data_rate" parameter of linkspeed.
> >       iblinkinfo and ibnetdiscover print this differently.  :-(
> > 
> >    2) But more importantly this is an ease of use issue.
> > 
> >       This:
> > 
> > 		snprintf(link_str, 256,
> > 			"(%3s %s %6s/%8s)",
> > 			ibnd_linkwidth_str(port->info.link_width_active),
> > 			ibnd_linkspeed_str(port->info.link_speed_active, 1),
> > 			ibnd_linkstate_str(port->info.link_state),
> > 			ibnd_physstate_str(port->info.phys_state)
> > 			);
> > 
> >       Becomes this:
> > 
> >       char buf[256];
> >       ...
> > 		snprintf(link_str, 256,
> > 			"(%3s %s %6s/%8s)",
> >          mad_dump_val(IB_PORT_LINK_WIDTH_ACTIVE_F, buf, 256, &port->info.link_width_active);
> >          mad_dump_val(IB_PORT_LINK_SPEED_ACTIVE_F, buf, 256, &port->info.link_speed_active);
> >                    //   ^^^^^^^^^^^^^^^^^
> >                    //   Not backwards compatible with the current ibnetdiscover
> >                    //   as it prints the data as "2.5 Gbps" rather than "SDR"
> >          mad_dump_val(IB_PORT_STATE_F, buf, 256, &port->info.link_state);
> >          mad_dump_val(IB_PORT_PHYS_STATE_F, buf, 256,
> >          &port->info.phys_state););
> > 
> >       Users don't need to go look up in mad.h for the field enum to print
> >       something they already have; "link_width_active".
> > 
> > 
> > Anyway, I think I am starting to see the difference in what we are thinking...
> > 
> > The ibnd_*_str functions and the ibnd_port_info_t were designed based on
> > libibnetdisc being a "one stop shop" for this data.  I envisioned this library
> > being a wrapper around lower level libraries which would abstract away some
> > details, something like this.
> > 
> >   +----------+  +----------+
> >   |  diag1   |  |  diag2   |
> >   +----------+  +----------+
> >           |         |
> >        +-----------------+
> >        |  libibnetdisc   |
> >        +-----------------+
> >                |
> >        +-----------------+
> >        |  libibmad       |
> >        +-----------------+
> > 
> > I think what you had in mind was something like:
> > 
> >                        +--------+                     
> >                       -| diag 1 |-                    
> >                      / +--------+ \                   
> >   +-----------------+  +--------+  +-----------------+
> >   |  libibnetdisc   | -| diag 2 |--|  libibmad       |
> >   +-----------------+  +--------+  +-----------------+
> >               \                          /
> >                --------------------------
> > 
> > In this case users of libibnetdisc might get back something like:
> > 
> >    typedef struct port {
> > 	   uint64_t guid;
> > 	   int portnum;
> > 	   int ext_portnum; /* optional if != 0 external port num */
> > 	   ibnd_node_t *node; /* node this port belongs to */
> > 	   struct port *remoteport; /* null if SMA, or does not exist */
> > 	   void *port_info; /* or uint8_t port_info[port_info_size] */
> >    } ibnd_port_t;
> > 
> > and decode port_info like this:
> > 
> >    uint32_t lid = mad_get_field(port->port_info, 0, IB_PORT_LID_F);
> >    mad_dump_val(IB_PORT_LID_F, port->port_info, &lid);
> > 
> > Is that what you are thinking?
> 
> Yes.
> 
> > If this is the case I don't think I object.  I
> > think it makes the end user of libibnetdisc work harder but it does offer some
> > advantages, namely less redefinition and a bit more flexibility.
> 
> More flexibility is even more important IMO.
> 
> > That said, I would like to clean up the mad interface at the same time.
> 
> Which is good thing by itself :) . I absolutely agree that we can
> improve our helpers.
> 
> > Just
> > figuring out the examples to write in this email have taken a lot of time.  I
> > don't think this is a good thing.
> > 
> > Here are some examples:
> > 
> > add something like:
> > 
> > static inline char *
> > mad_snprint_field(uint8_t *buf, int base_offs, int field,
> >                char *print_buf, int print_buf_size)
> > 
> > Therefore the above could be used in a print statement like:
> > 
> >    char tmp[256];
> >    printf("lid %s\n", mad_snprint_field(port->port_info, 0, IB_PORT_LID_F, tmp,
> >    256));
> > 
> >    [Although lid is a bad example since it could be done with "%d"...  But you
> >    get what I mean.]
> 
> Agree.
> 
> > 
> > And along those lines the difference between mad_dump_field and mad_dump_val
> > needs to be made more clear.  They have the same signature but one has a lot of
> > formating added to it which I don't think is appropriate at this level.
> > 
> >    "LinkState:.......................Active"
> >     vs.
> >    "Active"
> 
> Why not? I think it was designed to be just different levels. Maybe you
> are about unclear naming?
> 
> > Also, I don't think that the following declarations need to be public.
> > 
> > /* dump.c */
> > ib_mad_dump_fn
> > 	mad_dump_int, mad_dump_uint, mad_dump_hex, mad_dump_rhex,
> > 	mad_dump_bitfield, mad_dump_array, mad_dump_string,
> > 	mad_dump_linkwidth, mad_dump_linkwidthsup, mad_dump_linkwidthen,
> > 	mad_dump_linkdowndefstate,
> > 	mad_dump_linkspeed, mad_dump_linkspeedsup, mad_dump_linkspeeden,
> > 	mad_dump_portstate, mad_dump_portstates,
> > 	mad_dump_physportstate, mad_dump_portcapmask,
> > 	mad_dump_mtu, mad_dump_vlcap, mad_dump_opervls,
> > 	mad_dump_node_type,
> > 	mad_dump_sltovl, mad_dump_vlarbitration,
> > 	mad_dump_nodedesc, mad_dump_nodeinfo, mad_dump_portinfo, mad_dump_switchinfo,
> > 	mad_dump_perfcounters, mad_dump_perfcounters_ext;
> 
> Again, why not? Those helpers are widely used now in infiniband-diags.
> 
> > 
> > int	_mad_dump(ib_mad_dump_fn *fn, char *name, void *val, int valsz);
> > char *	_mad_dump_field(ib_field_t *f, char *name, char *buf, int bufsz,
> > 			void *val);
> > int	_mad_print_field(ib_field_t *f, char *name, void *val, int valsz);
> > char *	_mad_dump_val(ib_field_t *f, char *buf, int bufsz, void *val);
> > 
> > They confuse the ibmad layer.
> 
> I think there are two (at least) levels - lower is for more flexibility
> and higher is for easy to use.
> 
> Of course I agree that some improvements could be very useful - for
> example doing 'ib_mad_dump_fn' to return number of printed characters
> (similar to proposed mad_snprint_field()).
> 
> > If this is what you would like I will rework the library.
> 
> If we are agreed.
> 
> > Perhaps starting to
> > clean up libibmad along the way?
> 
> In parallel... :)
> 
> Sasha



More information about the general mailing list