Committed to trunk and branch - 2352-3.
> -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Leonid Keller > Sent: Friday, August 14, 2009 11:37 AM > To: Fab Tillier; Tzachi Dar; Sean Hefty > Cc: [email protected] > Subject: RE: [ofw][patch][ND provider] Improving latency of ms-mpi > > > OK, I return pMaxInlineData to be only an out parameter. > I've also added some little improvement according to Fab comments. > > I think, this patch should go also to the release branch. > Agree ? > > Index: ulp/nd/user/NdEndpoint.cpp > =================================================================== > --- ulp/nd/user/NdEndpoint.cpp (revision 2351) > +++ ulp/nd/user/NdEndpoint.cpp (working copy) > @@ -98,7 +98,7 @@ > __in SIZE_T nOutboundSge, > __in SIZE_T InboundReadLimit, > __in SIZE_T OutboundReadLimit, > - __in_opt __out_opt SIZE_T* pMaxInlineData > + __out_opt SIZE_T* pMaxInlineData > ) > { > ND_ENTER( ND_DBG_NDI ); > @@ -124,11 +124,7 @@ > m_pParent->m_Ifc.user_verbs.post_recv != NULL /*|| > m_pParent->m_Ifc.user_verbs.bind_mw != NULL*/ ); > > - UINT32 InlineSize; > - if ( pMaxInlineData ) > - InlineSize = (UINT32)*pMaxInlineData; > - else > - InlineSize = g_nd_max_inline_size; > + m_MaxInlineSize = g_nd_max_inline_size; > > HRESULT hr = CreateQp( > pInboundCq, > @@ -139,7 +135,7 @@ > nOutboundSge, > InboundReadLimit, > OutboundReadLimit, > - InlineSize ); > + m_MaxInlineSize ); > > if( FAILED( hr ) ) > return hr; > @@ -151,12 +147,11 @@ > return hr; > } > else > - InlineSize = (UINT32)qp_attr.sq_max_inline; > + m_MaxInlineSize = (UINT32)qp_attr.sq_max_inline; > > > m_Ird = (UINT8)InboundReadLimit; > m_Ord = (UINT8)OutboundReadLimit; > - m_MaxInlineSize = InlineSize; > > // Move the QP to the INIT state so users can post receives. > hr = ModifyQp( IB_QPS_INIT ); > @@ -164,7 +159,7 @@ > DestroyQp(); > > if( SUCCEEDED( hr ) && pMaxInlineData != NULL ) > - *pMaxInlineData = InlineSize; > + *pMaxInlineData = m_MaxInlineSize; > > return hr; > } > Index: ulp/nd/user/NdEndpoint.h > =================================================================== > --- ulp/nd/user/NdEndpoint.h (revision 2351) > +++ ulp/nd/user/NdEndpoint.h (working copy) > @@ -67,7 +67,7 @@ > __in SIZE_T nOutboundSge, > __in SIZE_T InboundReadLimit, > __in SIZE_T OutboundReadLimit, > - __in_opt __out_opt SIZE_T* pMaxInlineData > + __out_opt SIZE_T* pMaxInlineData > ); > > public: > > > -----Original Message----- > > From: Fab Tillier [mailto:[email protected]] > > Sent: Wednesday, August 12, 2009 7:59 PM > > To: Tzachi Dar; Sean Hefty; Leonid Keller > > Cc: [email protected] > > Subject: RE: [ofw][patch][ND provider] Improving latency of ms-mpi > > > > I think specifying a default of 160 (or whatever is appropriate for > > the hardware), that can be overridden via an environment > variable is > > the right thing to do. Changing the parameter to be in/out is the > > wrong thing to do at this point in time. Changing the API > contract is > > only going to confuse application developers, and in the > longer term > > will just result in support issues. > > > > The max inline parameter is an out parameter. Until there's a new > > interface that defines it otherwise, it should remain an out > > parameter. > > > > -Fab > > > > > -----Original Message----- > > > From: Tzachi Dar [mailto:[email protected]] > > > Sent: Wednesday, August 12, 2009 4:40 AM > > > To: Sean Hefty; Leonid Keller; Fab Tillier > > > Cc: [email protected] > > > Subject: RE: [ofw][patch][ND provider] Improving latency of ms-mpi > > > > > > I guess that we all have good points here, but I believe we > > should be > > > moving forward: > > > > > > Since an important goal of this project is reaching low > latency, I > > > don't see how we can not do inline send at all. > > > > > > I agree that changing the api might break existing > applications but: > > > 1) I don't think there is really a big number of > > applications > > > currently. 2) The "break" actually means that > inline will be > > > used on a > > > place where it is not supposed to. This might mean "sub-optimal" > > > performance, but I believe that all applications will > > continue to work. > > > (We need to make sure that if the number used is too big, > > we will use > > > a good default (maybe 0). > > > 3) Giving applications control over the inline size > > (that is > > > using the parameter as in/out) will be the best technical > solution > > > going forward. > > > > > > Our suggestion is this: add a new environment variable. If that > > > variable doesn't exist the parameter is out only. If it > is set, the > > > parameter is in/out. This will promise that older > > applications still work as well. > > > People who write according to the new api will set this > environment > > > variable. Fab, can you make sure that the new nd version > will allow > > > this parameter to be in/out? > > > > > > Controlling inline or not will still be done using an environment > > > variable (as Leonid sent). > > > > > > Does this makes sense? > > > > > > Thanks > > > Tzachi > > > > > > > > > > > > > > > > > >> -----Original Message----- > > >> From: Sean Hefty [mailto:[email protected]] > > >> Sent: Wednesday, August 12, 2009 2:03 AM > > >> To: Leonid Keller; Fab Tillier; Tzachi Dar > > >> Cc: [email protected] > > >> Subject: RE: [ofw][patch][ND provider] Improving latency > of ms-mpi > > >> > > >> My point was that NetworkDirect is a published API whose > > definition > > >> is owned entirely by Microsoft. All implementations MUST > > adhere to > > >> the published ND API specification, or they are not > > compliant. It's > > >> not about legality, who maintains a specific ND > implementation, or > > >> whether a specific change is considered an improvement > over what's > > >> there. > > >> > > >> We are free to change IBAL, WinVerbs, or other APIs > because we own > > >> them. The only cost of doing so is breaking existing > > applications. > > >> But for ND, our choice is to be compliant or not. > > >> > > >> The way I've gone about requesting changes to ND is to > > send comments > > >> to MS using the links at the bottom of the ND documentation. I > > >> expect that these messages get routed directly to Fab, who > > rolls his > > >> eyes before hitting the delete key. > > >> :) > > >> > > >> > > >>> This change adds a new facility. Tzachi and me have brought the > > >>> reasons. So it's good from engineer point view. I don't > > know whether > > >>> we may do it from legal one. But we used to change Ibal > > API and ND > > >>> sits in the same Open Source Tree with IBAL. Who is he > > maintainer of > > >>> ND provider code ? I thought it's Fab. Fab, do you > > approve this API > > >>> change ? > > >>> > > >>> > > >>>> -----Original Message----- > > >>>> From: Sean Hefty [mailto:[email protected]] > > >>>> Sent: Monday, August 10, 2009 9:05 PM > > >>>> To: Leonid Keller; Fab Tillier; Tzachi Dar > > >>>> Cc: [email protected] > > >>>> Subject: RE: [ofw][patch][ND provider] Improving latency > > of ms-mpi > > >>>> > > >>>>> This latter parameter should be IN OUT, because the > > driver takes > > >>>>> its value as a hint. It really re-calculates it, trying to > > >>>>> maximize in the limits of WQE size. > > >>>> > > >>>> Isn't this a MS defined API? I don't believe we can > change the > > >>>> NDEndpoint APIs at all. > > >>>> > > >>>> - Sean > > >>>> > > >>>> > > >> > > >> > > > _______________________________________________ > ofw mailing list > [email protected] > http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw > _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
