Hi Al, On Wed, Sep 21, 2011 at 2:25 PM, Albert Chu <[email protected]> wrote: > Hey Hal, > > Ira answered most of the important stuff already. But some extra > details ... > >> Second the current alignment breaks libibmad. So it would be a lot >> more code to support the miss-alignment and would probably have to be >> changed anyway. > > As far as I can tell, all the code in libibmad currently assumes 32 bit > alignment. For example, "BITSOFFS(48, 32)" leads to an offset of -16, > which is not good. I played around with updating the code in libibmad, > but it was a nice chunk of changes for 1 measly field. We could remove > the problem field until the issue is resolved in IBTA? Or perhaps we > need to really redo chunks of libibmad to deal with this.
OK; maybe this is a new case of misalignment than has been dealt with in the past. Ideally/hopefully, we don't need to deal with this. -- Hal >> Also, would you comment on what testing has been done with this ? > > I've gotten ibccquery to talk to Mellanox CAs and Switches and output > the current config. I've got a side tool that can do rudimentary > congestion control settings, so at the minimum I can twiddle some values > and they show up in the output of ibccquery. I haven't done any testing > w/ real network traffic yet, so there could be lingering bugs. But this > is just the initial first pass. > > Al > > On Wed, 2011-09-21 at 08:49 -0700, Ira Weiny wrote: >> On Wed, 21 Sep 2011 07:17:38 -0700 >> Hal Rosenstock <[email protected]> wrote: >> >> > Hi Al, >> > >> > On Mon, Sep 19, 2011 at 6:06 PM, Albert Chu <[email protected]> wrote: >> > > The following patches add a new tool ibccquery to infiniband-diags. It >> > > supports the querying of various congestion control settings. Related >> > > updates to libibmad are also included. >> > >> > Looks good to me :-) Just a few comments below: >> > >> > Attaching rather than inlining patches makes it harder to comment. >> > >> > On 0001-Add-support-for-congestion-control-mads.patch, is ib_rpc_cc_t >> > really needed ? Couldn't mkey in existing rpc struct just be >> > reused/overloaded for this (and change comment to indicate mkey or >> > cckey) and then some code could be eliminated ? >> >> I am not sure I like overloading fields like this. I will take a look at it >> and see if it "looks" good but in general to keep ABI and clarity of the code >> I preferred the separate struct. >> >> > >> > On 0001-Support-ibccquery-congestion-control-query-tool.patch, I'm >> > worried about the following: >> > + /* XXX: Q3/2010 errata lists first entry offset at 80, but we >> > assume >> > + * will be updated to 96 once CurrentTimeStamp field is word >> > aligned. >> > + * In addition, assume max 13 log events instead of 16. Due to >> > + * errata changes increasing size of CA log event, 16 log >> > events is >> > + * no longer possible to fit in max MAD size. >> > + */ >> > >> > As far as the 13 v. 16 entries, this appears correct to me (MAD size) >> > but I'm concerned about changing the offset from 80 to 96 for better >> > alignment as this is putting the cart before the horse a little as >> > since these changes have not been finalized AFAIK at the IBTA. >> >> Yes, it is a bit premature. I have submitted the above alignment as a >> comment >> to the IBTA but as you say it is not published. Most importantly the >> miss-alignment breaks the convention of the spec. So I don't think the IBTA >> will reject the comment. >> >> Second the current alignment breaks libibmad. So it would be a lot more code >> to support the miss-alignment and would probably have to be changed anyway. >> >> > >> > Also, would you comment on what testing has been done with this ? >> > >> >> Right, the real question is what does current hardware do? >> >> We have been unable to determine if any of the vendors support the errata >> fully or specifically the miss-aligned CurrentTimeStamp. When I asked the >> vendors I got concrete answers back, so we proceeded with trying to reverse >> engineer it. Right now the query succeeds, that is all we know. >> >> Perhaps someone on the list can help us find out? :-D >> >> In the meantime we wanted to get comments on the patches. >> >> Ira >> >> > -- Hal >> > >> > > Al >> > > >> > > -- >> > > Albert Chu >> > > [email protected] >> > > Computer Scientist >> > > High Performance Systems Division >> > > Lawrence Livermore National Laboratory >> > > >> > > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> > the body of a message to [email protected] >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > -- > Albert Chu > [email protected] > Computer Scientist > High Performance Systems Division > Lawrence Livermore National Laboratory > > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
