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

[Sasha Khapyorsky]

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