[ofa-general] Re: [PATCH] infiniband-diags/ibsysstat: use RMPP for client/server communication

[Hal Rosenstock]

Sasha,

On Tue, Jan 27, 2009 at 7:19 AM, Sasha Khapyorsky <sashak at voltaire.com> wrote:
> Hi Hal,
>
> On 17:14 Mon 26 Jan     , Hal Rosenstock wrote:
>> On Mon, Jan 26, 2009 at 4:26 PM, Sasha Khapyorsky <sashak at voltaire.com> wrote:
>> >
>> > This patch adds support for bigger than (256 - vendor2 data offset) data
>> > sending by ibsysstat server using RMPP. It fixes bug#1237 - where server
>> > output was truncated due to MAD size limitation.
>>
>> Seems like the class version should be bumped for this change.
>
> Should it? Class vendor2 permits RMPP and it is defined in the spec as
> version 1. I think it was ibsysstat bug/decision to not use/handle it.

Just because the class allows RMPP doesn't mean all operations use it
so no this wasn't a bug IMO. It was done that way for extensibility
(possibility of longer exchanges/using RMPP) which wouldn't have been
possible with vendor range 1.

>> What's the behavior of old client with new server and new client with
>> old server ?
>
> Basically it works fine together. Old server responds short MAD with
> RMPP flags inactive.

This is technically a protocol violation even though it may work.

> The only interesting case is when old client gets
> long RMPP reply from new server - it will grab 256 bytes (so as it was
> before the data will be truncated) and server will get timeout from RMPP
> layer - and yes, seems we need to handle(drop) this MAD (as well as
> possible RMPP unrelated timeout/error on client side). Something like
> this:

Class version handling could also be used here to handle this.

-- Hal

> diff --git a/infiniband-diags/src/ibsysstat.c b/infiniband-diags/src/ibsysstat.c
> index c20a6f0..a145daf 100644
> --- a/infiniband-diags/src/ibsysstat.c
> +++ b/infiniband-diags/src/ibsysstat.c
> @@ -169,6 +174,11 @@ static char *ibsystat_serv(void)
>        DEBUG("starting to serve...");
>
>        while ((umad = mad_receive(buf, -1))) {
> +               if (umad_status(buf)) {
> +                       DEBUG("drop mad with status %x: %s", umad_status(buf),
> +                             strerror(umad_status(buf)));
> +                       continue;
> +               }
>
>                mad = umad_get_mad(umad);
>
> @@ -235,6 +245,9 @@ static char *ibsystat(ib_portid_t *portid, int attr)
>        if (umad_recv(fd, buf, &len, timeout) < 0)
>                IBPANIC("umad_recv failed.");
>
> +       if (umad_status(buf))
> +               return strerror(umad_status(buf));
> +
>        DEBUG("Got sysstat pong..");
>        if (attr != IB_PING_ATTR)
>                puts(data);
>
>
> Sasha
>