Thank you for the comments. Will fix (including a comment for UD_QP_USED_SGE) and commit
-----Original Message----- From: Fab Tillier [mailto:[email protected]] Sent: Tuesday, November 09, 2010 8:30 PM To: Alex Naslednikov; [email protected] Subject: RE: [ofw] [IPoIB_NDIS6_CM] [Patch 3/3] Improper IPoIB behavior during simultaneous sends on both sides Alex Naslednikov wrote on Tue, 9 Nov 2010 at 09:24:26 > Signed-off by: Alexander Naslednikov (xalex at mellanox.co.il) > > Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp > =================================================================== --- > ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp (revision 2984) +++ > ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp (working copy) @@ -1112,8 > +1112,13 @@ > qp_create.sq_depth = p_port->p_adapter->params.sq_depth; > > #define UD_QP_USED_SGE 3 What the heck is UD_QP_USED_SGE? How about a comment? > - 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. > + { > + p_port->p_ca_attrs->max_sges = MAX_SEND_SGE; > + } > + p_port->p_ca_attrs->max_sges -= UD_QP_USED_SGE; > + qp_create.sq_sge = p_port->p_ca_attrs->max_sges; > + > if ( !p_port->p_ca_attrs->ipoib_csum ) > { > /* checksum is not supported by device > @@ -3799,7 +3804,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->p_ca_attrs->max_sges ) > { > IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_SEND, > ("Too many buffers to fit in WR ds_array. Copying > data.\n") ); > @@ -4270,13 +4275,13 @@ > 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->p_ca_attrs- > max_sges || > 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->p_ca_attrs->max_sges , p_sgl->Elements[0].Length ) ); No space before comma. The line is also too long. > > if( !s_buf->p_port->p_adapter->params.cm_enabled ) > { _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
