[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