[ofa-general] Re: [PATCH] infiniband-diags/src/ibnetdiscover.c: Fix the PortInfo data on the port we discover a switch on.
Hal Rosenstock
hrosenstock at xsigo.com
Fri Jun 6 10:43:22 PDT 2008
Hi Ira,
On Fri, 2008-06-06 at 10:33 -0700, Ira Weiny wrote:
> Hey Hal, comments inline...
>
> On Fri, 06 Jun 2008 04:13:41 -0700
> Hal Rosenstock <hrosenstock at xsigo.com> wrote:
>
> > Hi Ira,
> >
> > On Thu, 2008-06-05 at 19:12 -0700, Ira Weiny wrote:
> > > I noticed over the last couple days that ibnetdiscover would print the
> > > incorrect speed on the port I would run ibnetdiscover from. For example:
> > >
> > > Switch 24 "S-0008f10400411f56" # "SW1 wopr ISR9024D (MLX4 FW)" base port 0 lid 11 lmc 0
> > > ...
> > > [13] "H-0002c90200219e64"[1](2c90200219e65) # "wopri" lid 32 4xSDR
> > >
> > > ^^^
> > > (Note from the switch side of things it thinks the speed is SDR.)
> > > ...
> > > Ca 2 "H-0002c90200219e64" # "wopri"
> > > [1](2c90200219e65) "S-0008f10400411f56"[13] # lid 32 lmc 0 "SW1 wopr ISR9024D (MLX4 FW)" lid 11 4xDDR
> > >
> > > ^^^
> > > (but here DDR.)
> >
> > Yes, that is clearly wrong. Good find.
> >
> > > It turns out that when you first discover a switch the port object created gets
> > > the PortInfo of Port "0".
> >
> > Right; this trick works for peer CA and router ports but not switch
> > ports where the actual port number is needed.
>
> Ah! I guess that is the reason the PortInfo is querried here instead of just
> in "get_port" later? I was somewhat curious why things were done this way.
> That makes a lot more sense.
>
> >
> > > This patch queries again for the PortInfo of the port we "came in on".
> > >
> > > Ira
> > >
> > >
> > > From 5bc66af276c7baabd4d66be9df0379271cb625b4 Mon Sep 17 00:00:00 2001
> > > From: Ira K. Weiny <weiny2 at llnl.gov>
> > > Date: Thu, 5 Jun 2008 19:03:44 -0700
> > > Subject: [PATCH] infiniband-diags/src/ibnetdiscover.c: Fix the PortInfo data on the port we discover a switch on.
> > >
> > > Previously the portinfo data for that port object would be the PortInfo of
> > > port "0" of the switch. Specifically this would cause the speed of the link
> > > to be printed incorrectly.
> >
> > Did you try back to back CAs too ?
>
> No. See below regarding ca/routers.
>
> >
> > Nit: this looks like 2 patches to me:
> > 1. Factor out redundant code by adding decode_port_info routine
> > 2. Fix link speed of local CA in switch display
>
> Well technically yes but I think #1 really is a result of the fix because now
> the code is repeated 3 times instead of 2. I could split it, if you like, but
> it seemed like the fix required some code clean up to be "clean". (Although I
> admit that the patch itself is more complex.)
It's between you and Sasha; just a nit in separating the changes.
> >
> > > Signed-off-by: Ira K. Weiny <weiny2 at llnl.gov>
> > > ---
> > > infiniband-diags/src/ibnetdiscover.c | 33 +++++++++++++++++++--------------
> > > 1 files changed, 19 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/infiniband-diags/src/ibnetdiscover.c b/infiniband-diags/src/ibnetdiscover.c
> > > index b8d4e92..20da1ea 100644
> > > --- a/infiniband-diags/src/ibnetdiscover.c
> > > +++ b/infiniband-diags/src/ibnetdiscover.c
> > > @@ -129,6 +129,18 @@ node_type_str2(Node *node)
> > > return "??";
> > > }
> > >
> > > +void
> > > +decode_port_info(void *pi, Port *port)
> > > +{
> > > + mad_decode_field(pi, IB_PORT_LID_F, &port->lid);
> > > + mad_decode_field(pi, IB_PORT_LMC_F, &port->lmc);
> > > + mad_decode_field(pi, IB_PORT_STATE_F, &port->state);
> > > + mad_decode_field(pi, IB_PORT_PHYS_STATE_F, &port->physstate);
> > > + mad_decode_field(pi, IB_PORT_LINK_WIDTH_ACTIVE_F, &port->linkwidth);
> > > + mad_decode_field(pi, IB_PORT_LINK_SPEED_ACTIVE_F, &port->linkspeed);
> > > +}
> > > +
> > > +
> > > int
> > > get_port(Port *port, int portnum, ib_portid_t *portid)
> > > {
> > > @@ -139,13 +151,7 @@ get_port(Port *port, int portnum, ib_portid_t *portid)
> > >
> > > if (!smp_query(pi, portid, IB_ATTR_PORT_INFO, portnum, timeout))
> > > return -1;
> > > -
> > > - mad_decode_field(pi, IB_PORT_LID_F, &port->lid);
> > > - mad_decode_field(pi, IB_PORT_LMC_F, &port->lmc);
> > > - mad_decode_field(pi, IB_PORT_STATE_F, &port->state);
> > > - mad_decode_field(pi, IB_PORT_PHYS_STATE_F, &port->physstate);
> > > - mad_decode_field(pi, IB_PORT_LINK_WIDTH_ACTIVE_F, &port->linkwidth);
> > > - mad_decode_field(pi, IB_PORT_LINK_SPEED_ACTIVE_F, &port->linkspeed);
> > > + decode_port_info(pi, port);
> > >
> > > DEBUG("portid %s portnum %d: lid %d state %d physstate %d %s %s",
> > > portid2str(portid), portnum, port->lid, port->state, port->physstate, get_linkwidth_str(port->linkwidth), get_linkspeed_str(port->linkspeed));
> > > @@ -181,13 +187,7 @@ get_node(Node *node, Port *port, ib_portid_t *portid)
> > >
> > > if (!smp_query(pi, portid, IB_ATTR_PORT_INFO, 0, timeout))
> > > return -1;
> > > -
> > > - mad_decode_field(pi, IB_PORT_LID_F, &port->lid);
> > > - mad_decode_field(pi, IB_PORT_LMC_F, &port->lmc);
> > > - mad_decode_field(pi, IB_PORT_STATE_F, &port->state);
> > > - mad_decode_field(pi, IB_PORT_PHYS_STATE_F, &port->physstate);
> > > - mad_decode_field(pi, IB_PORT_LINK_WIDTH_ACTIVE_F, &port->linkwidth);
> > > - mad_decode_field(pi, IB_PORT_LINK_SPEED_ACTIVE_F, &port->linkspeed);
> > > + decode_port_info(pi, port);
> > >
> > > if (node->type != SWITCH_NODE)
> > > return 0;
> > > @@ -195,6 +195,11 @@ get_node(Node *node, Port *port, ib_portid_t *portid)
> > > node->smalid = port->lid;
> > > node->smalmc = port->lmc;
> > >
> > > + /* after we have the sma information find out the real PortInfo for this port */
> > > + if (!smp_query(pi, portid, IB_ATTR_PORT_INFO, node->localport, timeout))
> > > + return -1;
> > > + decode_port_info(pi, port);
> > > +
> >
> > Seems like this may only be needed when peer is switch but maybe it's
> > not worth saving those queries (I didn't look to see if the peer is
> > known here).
>
> This is past a check for not Switch. Here is a more complete chunk:
>
> if (node->type != SWITCH_NODE)
> return 0;
>
> node->smalid = port->lid;
> node->smalmc = port->lmc;
>
> /* after we have the sma information find out the real PortInfo for this port */
> if (!smp_query(pi, portid, IB_ATTR_PORT_INFO, node->localport, timeout))
> return -1;
> decode_port_info(pi, port);
>
>
> So I think for CA's/routers, back to back it should work the same. Also
> the additional query will only be issued if needed.
Looks good to me.
-- Hal
> Ira
>
> >
> > -- Hal
> >
> > > if (!smp_query(si, portid, IB_ATTR_SWITCH_INFO, 0, timeout))
> > > node->smaenhsp0 = 0; /* assume base SP0 */
> > > else
> >
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
More information about the general
mailing list