[openib-general] Re: Some more RMPP Questions and Comments (mad_rmpp.c)

[Sean Hefty]

Hal Rosenstock wrote:
> from rmpp.c::ib_create_send_mad
> ...
>         if (mad_agent->rmpp_version) {
>                 struct ib_rmpp_mad *rmpp_mad;
>                 rmpp_mad = (struct ib_rmpp_mad *)send_buf->mad;
>                 rmpp_mad->rmpp_hdr.paylen_newwin = cpu_to_be32(hdr_len -
>                         offsetof(struct ib_rmpp_mad, data) + data_len);
>         }
> 
> Should the above also be based on whether RMPP is active in the MAD to
> be sent ? I guess this depends on whether one MAD agent can be shared
> for RMPP and non RMPP sends.

ib_create_send_mad doesn't have the information to know if the MAD will 
active RMPP or not, just that the header is present.  Hmm... I was 
originally thinking that it wouldn't matter if this field was set or 
not, but if RMPP is not active, the field is supposed to be set to 0.

I can think of a couple of options here.  We can extend this API to 
indicate if RMPP is active or not.  Have the user clear this field if 
RMPP is not active.  Or let the RMPP code clear the field when sending 
the MAD...

It might even make sense to have calls to initialize the MAD and RMPP 
header information given a list of parameters.

> Also from rmpp.c::ib_create_send_mad:
> 
> send_buf->send_wr.wr.ud.remote_qkey = IB_QP_SET_QKEY;
> 
> Should the remote_qkey be a passed in parameter to this routine ?

The user could override this once the MAD has been returned, but I 
don't object to adding this...

> from mad_rmpp.c: 
> static int data_offset(u8 mgmt_class)
> {
>         if (mgmt_class == IB_MGMT_CLASS_SUBN_ADM)
>                 return offsetof(struct ib_sa_mad, data);
>         else if ((mgmt_class >= IB_MGMT_CLASS_VENDOR_RANGE2_START) &&
>                  (mgmt_class <= IB_MGMT_CLASS_VENDOR_RANGE2_END))
>                 return offsetof(struct ib_vendor_mad, data);
>         else
>                 return offsetof(struct ib_rmpp_mad, data);
> }
> 
> Seems like there are a number of places where vendor range 2 is checked.
> There already is a (static) function for this in mad.c. Should this be used
> (and made global) ?

I thought about this too, but didn't think that this check was complex 
or long enough to create a function for it...  I doubt many clients 
outside of the MAD layer itself would need such a check however, so 
sharing it between mad.c and mad_rmpp.c seems fine, but I doubt we need 
an exposed API.

- Sean