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

[Alex Naslednikov]

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
+	// 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 )
+	{
+		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;
+	}
+	
+	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) 
+	{
+		ASSERT ( FALSE );
+		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
+*
 *	ib_mgr
 *		IB resource manager.
 *

-----Original Message-----
From: Hefty, Sean [mailto:sean.hefty at intel.com] 
Sent: Tuesday, November 09, 2010 8:33 PM
To: Fab Tillier; 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

> > -	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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: large_send_3_revisited.patch
Type: application/octet-stream
Size: 3278 bytes
Desc: large_send_3_revisited.patch
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20101110/06fbe5cf/attachment.obj>