[ofw] [IPoIB_NDIS6_CM] [Patch 3/3] Improper IPoIB behavior during simultaneous sends on both sides

[Alex Naslednikov]

Thank you for the comments.
Will fix (including a comment for UD_QP_USED_SGE) and commit

-----Original Message-----
From: Fab Tillier [mailto:ftillier at microsoft.com] 
Sent: Tuesday, November 09, 2010 8:30 PM
To: Alex Naslednikov; ofw at lists.openfabrics.org
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 )
>  		{