[ofw] [Patch] [UMAD][Tools]
Hefty, Sean
sean.hefty at intel.com
Thu Jul 22 09:09:53 PDT 2010
FYI - in general patches to ibstat and other ib-diags should be submitted to the linux-rdma mail list and Sasha (both are copied on the reply).
> Hello,
>
> In fact this case should not happen. It was defined, as you said, to handle
> it if it were to occur.
>
> This patch fixes the wrong rate calculation within ibstat tool.
> Signed-off by: Irena Kruchkovsky (irena at mellanox.co.il)
> CR: XaleX
>
> Index: D:/Windows/MLNX_VPI/tools/infiniband-diags/src/ibstat.c
> ===================================================================
> --- D:/Windows/MLNX_VPI/tools/infiniband-diags/src/ibstat.c (revision 5005)
> +++ D:/Windows/MLNX_VPI/tools/infiniband-diags/src/ibstat.c (revision 6168)
> @@ -117,7 +117,7 @@
> printf("%sPhysical state: %s\n", pre,
> (unsigned)port->state <=
> 7 ? port_phy_state_str[port->phys_state] : "???");
> - printf("%sRate: %d\n", pre, port->rate);
> + printf("%sRate: %d Gbps\n", pre, port->rate);
> printf("%sBase lid: %d\n", pre, port->base_lid);
> printf("%sLMC: %d\n", pre, port->lmc);
> printf("%sSM lid: %d\n", pre, port->sm_lid);
>
> Index: D:/Windows/MLNX_VPI/ulp/libibumad/src/umad.cpp
> ===================================================================
> --- D:/Windows/MLNX_VPI/ulp/libibumad/src/umad.cpp (revision 5765)
> +++ D:/Windows/MLNX_VPI/ulp/libibumad/src/umad.cpp (revision 6092)
> @@ -141,9 +141,25 @@
> port->sm_sl = attr.sm_sl;
> port->state = attr.state;
> port->phys_state = attr.phys_state;
> - port->rate = attr.active_speed;
> port->capmask = attr.port_cap_flags;
>
> + switch (attr.active_width){
> + case 1:
> + port->rate = (unsigned) (2.5*attr.active_speed);
> + break;
> + case 2:
> + port->rate = 10*attr.active_speed; //2.5*4
> + break;
> + case 4:
> + port->rate = 20*attr.active_speed; //2.5*8
> + break;
> + case 8:
> + port->rate = 30*attr.active_speed; //2.5*12
> + break;
> + default:
> + port->rate = 0;
> + }
> +
> // Assume GID 0 contains port GUID and gid prefix
> ret = ibv_query_gid(context, (uint8_t) port->portnum, 0, &gid);
> if (ret != 0) {
>
> -----Original Message-----
> From: Hal Rosenstock [mailto:hal.rosenstock at gmail.com]
> Sent: Tuesday, July 20, 2010 11:04 PM
> To: Irena Kruchkovsky
> Cc: Alex Naslednikov; ofw at lists.openfabrics.org; Uri Habusha
> Subject: Re: [ofw] [Patch] [UMAD][Tools]
>
> Hi Irena,
>
> On Tue, Jul 20, 2010 at 8:08 AM, Irena Kruchkovsky <irena at mellanox.co.il>
> wrote:
> > Hello Hal,
> >
> > The original purpose of returning a 0 was to handle cases in which we get
> invalid port width (i.e. a constant that is not supported).
> > According to your example, is it OK if the returned value for this case
> will be “-1” instead of 0? The ibstat.c file will be changed back to be the
> same in Windows and Linux, but in this case the -1 (or any other error
> value) will be printed as is in the rate, if such a case occurs.
>
> I'm hoping we can get ibstat.c to be the same in Windows and Linux and
> constrain the changes to umad.cpp/libibumad. In Linux, the rate file
> is not written when the link width is not valid but I've never seen
> that happen. Does that occur in Windows or is this to handle it if it
> were to occur ?
>
> Anyhow, here's what I would propose:
>
> 1. Just add Gbps to Rate display (without the N/A for 0 case) in ibstat.c:
>
> <change:>
> printf("%sRate: %d\n", pre, port->rate);
> <to>
> printf("%sRate: %d Gbps\n", pre, port->rate);
>
> 2. changing umad.cpp as you originally proposed setting the port->rate
> to 0 for unknown link widths (yes, I flip flopped on this after
> looking at it again...)
>
> Thanks.
>
> -- Hal
>
> >
> > Thank you,
> > Irena
> >
> > -----Original Message-----
> > From: Hal Rosenstock [mailto:hal.rosenstock at gmail.com]
> > Sent: Tuesday, July 13, 2010 8:42 PM
> > To: Alex Naslednikov
> > Cc: Irena Kruchkovsky; ofw at lists.openfabrics.org
> > Subject: Re: [ofw] [Patch] [UMAD][Tools]
> >
> > On 7/13/10, Hal Rosenstock <hal.rosenstock at gmail.com> wrote:
> >> On Tue, Jul 13, 2010 at 11:45 AM, Alex Naslednikov
> <xalex at mellanox.co.il>
> >> wrote:
> >>> It can happen when the subnet isn't configured yet, for example.
> >>
> >> Isn't rate based on link speed and width ? Is one of those values
> >> invalid (reserved) ? What am I missing ?
> >
> > IMO ibstat.c should be the same in Windows and Linux. I'm not sure
> > what the underlying issue is in generating a rate of 0. The rate might
> > not be "real" in that the active (link speed/width) components are
> > only valid in certain port states but the value should never be 0
> > AFAIT. If a change needs to be made, it would be best to isolate it to
> > the Windows libibumad IMO.
> >
> > For comparison purposes, in Linux, libibumad parses the rate file
> > which is written in the kernel by sysfs.c as follows:
> >
> > struct ib_port_attr attr;
> > char *speed = "";
> > int rate;
> > ssize_t ret;
> >
> > ret = ib_query_port(p->ibdev, p->port_num, &attr);
> > if (ret)
> > return ret;
> >
> > switch (attr.active_speed) {
> > case 2: speed = " DDR"; break;
> > case 4: speed = " QDR"; break;
> > }
> >
> > rate = 25 * ib_width_enum_to_int(attr.active_width) *
> attr.active_speed;
> > if (rate < 0)
> > return -EINVAL;
> >
> > return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
> > rate / 10, rate % 10 ? ".5" : "",
> > ib_width_enum_to_int(attr.active_width), speed);
> >
> > where:
> > static inline int ib_width_enum_to_int(enum ib_port_width width)
> > {
> > switch (width) {
> > case IB_WIDTH_1X: return 1;
> > case IB_WIDTH_4X: return 4;
> > case IB_WIDTH_8X: return 8;
> > case IB_WIDTH_12X: return 12;
> > default: return -1;
> > }
> > }
> >
> > I didn't look to see how Windows libibumad handles this.
> >
> > -- Hal
> >
> >>
> >> -- Hal
> >>
> >>>
> >>> -----Original Message-----
> >>> From: ofw-bounces at lists.openfabrics.org
> >>> [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Hal Rosenstock
> >>> Sent: Tuesday, July 13, 2010 6:41 PM
> >>> To: Irena Kruchkovsky
> >>> Cc: ofw at lists.openfabrics.org
> >>> Subject: Re: [ofw] [Patch] [UMAD][Tools]
> >>>
> >>> On Tue, Jul 13, 2010 at 10:53 AM, Irena Kruchkovsky
> <irena at mellanox.co.il>
> >>> wrote:
> >>>> This patch fixes the wrong rate calculation within ibstat tool.
> >>>>
> >>>> Signed-off by: Irena Kruchkovsky (irena at mellanox.co.il)
> >>>>
> >>>> CR: Alexander Naslednikov (xalex at mellanox.co.il)
> >>>>
> >>>>
> >>>>
> >>>> Index: D:/Windows/MLNX_VPI/tools/infiniband-diags/src/ibstat.c
> >>>>
> >>>> ===================================================================
> >>>>
> >>>> --- D:/Windows/MLNX_VPI/tools/infiniband-diags/src/ibstat.c (revision
> >>>> 6077)
> >>>>
> >>>> +++ D:/Windows/MLNX_VPI/tools/infiniband-diags/src/ibstat.c
> >>>> (revision 6092)
> >>>>
> >>>> @@ -117,7 +117,15 @@
> >>>>
> >>>> printf("%sPhysical state: %s\n", pre,
> >>>>
> >>>> (unsigned)port->state <=
> >>>>
> >>>> 7 ? port_phy_state_str[port->phys_state] :
> >>>> "???");
> >>>>
> >>>> - printf("%sRate: %d\n", pre, port->rate);
> >>>>
> >>>> + if (port->rate != 0)
> >>>
> >>> Just wondering:
> >>> When is the rate 0 returned by umad ? Is that a valid case ?
> >>>
> >>> -- Hal
> >>>
> >>>>
> >>>> + {
> >>>>
> >>>> + printf("%sRate: %d Gbps\n", pre,
> >>>> +port->rate);
> >>>>
> >>>> + }
> >>>>
> >>>> + else
> >>>>
> >>>> + {
> >>>>
> >>>> + printf("%sRate: N\\A\n", pre);
> >>>>
> >>>> + }
> >>>>
> >>>> +
> >>>>
> >>>> printf("%sBase lid: %d\n", pre, port->base_lid);
> >>>>
> >>>> printf("%sLMC: %d\n", pre, port->lmc);
> >>>>
> >>>> printf("%sSM lid: %d\n", pre, port->sm_lid);
> >>>>
> >>>> Index: D:/Windows/MLNX_VPI/ulp/libibumad/src/umad.cpp
> >>>>
> >>>> ===================================================================
> >>>>
> >>>> --- D:/Windows/MLNX_VPI/ulp/libibumad/src/umad.cpp
> >>>> (revision
> >>>> 6077)
> >>>>
> >>>> +++ D:/Windows/MLNX_VPI/ulp/libibumad/src/umad.cpp (revision
> >>>> +++ 6092)
> >>>>
> >>>> @@ -141,9 +141,25 @@
> >>>>
> >>>> port->sm_sl = attr.sm_sl;
> >>>>
> >>>> port->state = attr.state;
> >>>>
> >>>> port->phys_state = attr.phys_state;
> >>>>
> >>>> - port->rate = attr.active_speed;
> >>>>
> >>>> port->capmask = attr.port_cap_flags;
> >>>>
> >>>>
> >>>>
> >>>> + switch (attr.active_width){
> >>>>
> >>>> + case 1:
> >>>>
> >>>> + port->rate = (unsigned)
> >>>> (2.5*attr.active_speed);
> >>>>
> >>>> + break;
> >>>>
> >>>> + case 2:
> >>>>
> >>>> + port->rate =
> >>>> 10*attr.active_speed; //2.5*4
> >>>>
> >>>> + break;
> >>>>
> >>>> + case 4:
> >>>>
> >>>> + port->rate =
> >>>> 20*attr.active_speed; //2.5*8
> >>>>
> >>>> + break;
> >>>>
> >>>> + case 8:
> >>>>
> >>>> + port->rate =
> >>>> 30*attr.active_speed; //2.5*12
> >>>>
> >>>> + break;
> >>>>
> >>>> + default:
> >>>>
> >>>> + port->rate = 0;
> >>>>
> >>>> + }
> >>>>
> >>>> +
> >>>>
> >>>> // Assume GID 0 contains port GUID and gid prefix
> >>>>
> >>>> ret = ibv_query_gid(context, (uint8_t) port->portnum,
> >>>> 0, &gid);
> >>>>
> >>>> if (ret != 0) {
> >>>>
> >>>> _______________________________________________
> >>>> ofw mailing list
> >>>> ofw at lists.openfabrics.org
> >>>> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
> >>>>
> >>> _______________________________________________
> >>> ofw mailing list
> >>> ofw at lists.openfabrics.org
> >>> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
> >>>
> >>
> >
More information about the ofw
mailing list