Applied at 2988 -----Original Message----- From: Alex Naslednikov Sent: Thursday, November 11, 2010 1:52 PM To: 'Fab Tillier'; Hefty, Sean; [email protected] Subject: RE: [ofw] [IPoIB_NDIS6_CM] [Patch 3/3] Improper IPoIB behavior during simultaneous sends on both sides
Fab, Thank you for the comments. Please, SB -----Original Message----- From: Fab Tillier [mailto:[email protected]] Sent: Wednesday, November 10, 2010 7:20 PM To: Alex Naslednikov; Hefty, Sean; [email protected] Subject: RE: [ofw] [IPoIB_NDIS6_CM] [Patch 3/3] Improper IPoIB behavior during simultaneous sends on both sides Alex Naslednikov wrote on Wed, 10 Nov 2010 at 05:40:48 > Please, find the updated patch > In addition to the community comments, it contains also additional > verification code > and new variable on the port structure to store maximum number of SG > elements > > Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp > =================================================================== > --- ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp (revision 2986) > +++ ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp (working copy) > @@ -1110,10 +1110,23 @@ > qp_create.rq_sge = 2; /* To support buffers spanning pages. */ > qp_create.h_rq_cq = p_port->ib_mgr.h_recv_cq; > qp_create.sq_depth = p_port->p_adapter->params.sq_depth; > - > + > + // p_ca_attrs->max_sges contains the maximum number of SG > elements > + // available by HW. 3 of the them are reserved for an internal > use Internal use for UD transfer? It would be helpful to document what they are used for. [XaleX] Maybe it would be wise to refer to spec document, but it should be done as a separate patch. If this comment is not acceptable, I can remove it > + // Thus, the maximum of SGE for UD QP is limited by (p_ca_attrs- > >max_sges - 3) > #define UD_QP_USED_SGE 3 > - qp_create.sq_sge = MAX_SEND_SGE < p_port->p_ca_attrs->max_sges ? > - MAX_SEND_SGE : ( p_port->p_ca_attrs->max_sges - > UD_QP_USED_SGE ); > + if ( p_port->p_ca_attrs->max_sges > MAX_SEND_SGE ) Why not check against MAX_SEND_SGE + UD_QP_USED_SGE? Or is MAX_SEND_SGE inclusive of the reserved SGE entries? [XaleX] The second is correct > + { > + p_port->max_sq_sge_supported = MAX_SEND_SGE - > UD_QP_USED_SGE; > + } > + else > + { > + p_port->max_sq_sge_supported = p_port->p_ca_attrs->max_sges > - UD_QP_USED_SGE; If you make the change above, you will be able to remove the '- UD_QP_USED_SGE' here. [XaleX] I didn't > + } > + > + p_port->p_ca_attrs->max_sges -= UD_QP_USED_SGE; > + qp_create.sq_sge = p_port->max_sq_sge_supported; > + > if ( !p_port->p_ca_attrs->ipoib_csum ) > { > /* checksum is not supported by device > @@ -1162,6 +1175,11 @@ > p_port->p_adapter->p_ifc->get_err_str( status )) ); > return status; > } > + if ( qp_attr.sq_sge < qp_create.sq_sge) The HCA driver is not allowed to reduce the number of SGEs requested - it must fail the request if the number of SGEs requested exceeds its capabilities. You can replace the if with ASSERT( qp_attr.sq_sge >= qp_create.sq_sge ) and that should be good enough. This is not the place to validate that the HCA driver behaves properly. [XaleX] This is correct, so I would add the following: ASSERT ( qp_attr.sq_sge >= qp_create.sq_sge ); > + { > + ASSERT ( FALSE ); Any assertions should have meaningful expressions. Seeing in the debugger "Assertion failed: FALSE" is not helpful. > + p_port->max_sq_sge_supported = qp_attr.sq_sge; > + } > p_port->ib_mgr.qpn = qp_attr.num; > > /* Register all of physical memory */ > @@ -3799,7 +3817,7 @@ > } > > /* Remember that one of the DS entries is reserved for the IPoIB > header. */ > - if( num_pages >= MAX_SEND_SGE ) > + if( num_pages >= p_port->max_sq_sge_supported ) > { > IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_SEND, > ("Too many buffers to fit in WR ds_array. Copying > data.\n") ); > @@ -4270,13 +4288,14 @@ > Copy only N+1-MAX_SEND_SGE, where N is a lenght of SG List > Remember that one of the DS entries is reserved for the IPoIB > header. > */ > - if( ( p_sgl->NumberOfElements >= MAX_SEND_SGE || > + if( ( p_sgl->NumberOfElements >= s_buf->p_port->max_sq_sge_supported || > p_sgl->Elements[0].Length < sizeof(eth_hdr_t)) ) > { > IPOIB_PRINT( TRACE_LEVEL_WARNING, IPOIB_DBG_SEND, > ("Too many buffers %d to fit in WR ds_array[%d] \ > Or buffer[0] length %d < Eth header. Copying > data.\n", > - p_sgl->NumberOfElements, MAX_SEND_SGE, p_sgl- > >Elements[0].Length ) ); > + p_sgl->NumberOfElements, s_buf->p_port- > >max_sq_sge_supported , > + p_sgl->Elements[0].Length ) ); > > if( !s_buf->p_port->p_adapter->params.cm_enabled ) > { > Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_port.h > =================================================================== > --- ulp/ipoib_NDIS6_CM/kernel/ipoib_port.h (revision 2982) > +++ ulp/ipoib_NDIS6_CM/kernel/ipoib_port.h (working copy) > @@ -590,6 +590,7 @@ > > struct _ipoib_adapter *p_adapter; > uint8_t port_num; > + uint32_t max_sq_sge_supported; > > KEVENT sa_event; > > @@ -653,6 +654,9 @@ > * port_num > * Port number of this adapter. > * > +* max_sq_sge_supported > +* Maximum number of send QP SG elements supported by HW This isn't quite right - the HW could support more than this, as the maximum here is MAX_SEND_SGE, no? It's the maximum number of SGEs that will be used on this instance. [XaleX] Please, note that MAX_SEND_SGE is an upper limit but not a lower limit, cause if query_device_attr will return the lower number, max_sq_sge_supported will be less than MAX_SEND_SGE. Thus, the description may change to: "The actual number of SGEs that will be used for UD QP" > +* > * ib_mgr > * IB resource manager. > * > > -----Original Message----- > From: Hefty, Sean [mailto:[email protected]] > Sent: Tuesday, November 09, 2010 8:33 PM > To: Fab Tillier; Alex Naslednikov; [email protected] > Subject: RE: [ofw] [IPoIB_NDIS6_CM] [Patch 3/3] Improper IPoIB behavior > during simultaneous sends on both sides > > > > - qp_create.sq_sge = MAX_SEND_SGE < p_port->p_ca_attrs->max_sges ? > > > - MAX_SEND_SGE : ( p_port->p_ca_attrs->max_sges - > > > UD_QP_USED_SGE ); > > > + if ( MAX_SEND_SGE < p_port->p_ca_attrs->max_sges ) > > > > I find this notation very hard to read: "CONSTANT <operator> > variable". I > > *much* prefer variable <operator> CONSTANT. The argument that it > prevents > > errors is weak, at best, since compilers will generate warnings for > > assignment within conditionals. When looking at this code, I have to > think > > "if p_port->p_ca_attrs->max_sges > MAX_SEND_SGE" to be able to reason > the > > code flow. > > Agree with this I do. Backwards makes the logic it does. _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
