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

[Hal Rosenstock]

On Mon, 2005-05-02 at 15:30, Hal Rosenstock wrote:
> 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. 

This routine already needs hdr_len and data_len to calculate the buffer
size so doing this does not remove the need to pass these to this
routine.

Plus it seems this would be easy to screw up if left to the user:
                rmpp_mad->rmpp_hdr.paylen_newwin = cpu_to_be32(hdr_len -
                        offsetof(struct ib_rmpp_mad, data) + data_len);

So I'm having second thoughts...

-- Hal