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

[Hal Rosenstock]

On Mon, 2005-05-02 at 14:46, Sean Hefty wrote: 
> 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.

Right but it seems to me it needs this information.

> 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.

And also visa versa, if RMPP is active, the field is supposed to have
the ACTIVE bit on.

> I can think of a couple of options here.  We can extend this API to 
> indicate if RMPP is active or not.

That's what I thought at first.

> Have the user clear this field if RMPP is not active.

This seems fine to me and consistent with how remote_qkey is being
handled. This should be documented. 

>   Or let the RMPP code clear the field when sending 
> the MAD...

How would it know to do this ?

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

Yes, that would save each consumer from doing this explictly.

> > 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...

True. Should this be added to the comments ?

> > 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.

OK.

-- Hal