[ofw] [IPoIB_NDIS6_CM][Patch] Improving shutter mechanism for recv flow
Smith, Stan
stan.smith at intel.com
Tue Oct 26 10:02:04 PDT 2010
Thanks for the file!
A minor point about __ipoib_complete_reset(), the 'inline' directive is usually reserved for those functions which are in critical speed paths. I was curious about your thinking as to why you chose to inline?
Otherwise the patch looks good, will give it a test run today.
stan.
________________________________
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Tuesday, October 26, 2010 2:59 AM
To: Alex Naslednikov; ofw at lists.openfabrics.org
Subject: Re: [ofw] [IPoIB_NDIS6_CM][Patch] Improving shutter mechanism for recv flow
Now with the patch attached by file
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Tuesday, October 26, 2010 11:57 AM
To: ofw at lists.openfabrics.org
Subject: [ofw] [IPoIB_NDIS6_CM][Patch] Improving shutter mechanism for recv flow
This patch avoids the situation when the recv shutter can be shut twice.
It was tested with regression run that failed beforehand along with manual debug
(forcing NDIS to call to ipoib_reset() )
Signed-off by: Alexander Naslednikov (xalex at mellanox.co.il)
Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_adapter.cpp
===================================================================
--- ulp/ipoib_NDIS6_CM/kernel/ipoib_adapter.cpp (revision 2975)
+++ ulp/ipoib_NDIS6_CM/kernel/ipoib_adapter.cpp (working copy)
@@ -111,6 +111,28 @@
OUT UINT *p_len);
+static inline void
+ __ipoib_complete_reset(
+ IN ipoib_adapter_t* const p_adapter,
+ IN NDIS_STATUS Status )
+{
+ KLOCK_QUEUE_HANDLE hdl;
+
+ KeAcquireInStackQueuedSpinLock( &g_ipoib.lock, &hdl );
+ if ( p_adapter->ipoib_state & ~IPOIB_RESET_OR_DOWN )
+ {
+ //Alive the shutter even if the status was not NDIS_STATUS_SUCCESS
+ shutter_alive( &p_adapter->recv_shutter );
+ p_adapter->ipoib_state = p_adapter->ipoib_state & ~IPOIB_RESET_OR_DOWN;
+ ASSERT( p_adapter->ipoib_state == IPOIB_RUNNING );
+ }
+ KeReleaseInStackQueuedSpinLock( &hdl );
+
+ p_adapter->reset = FALSE;
+ NdisMResetComplete(
+ p_adapter->h_adapter, Status, TRUE );
+}
+
/* Implementation */
ib_api_status_t
ipoib_create_adapter(
@@ -800,9 +822,7 @@
if( p_adapter->reset && p_adapter->state != IB_PNP_PORT_INIT )
{
- p_adapter->reset = FALSE;
- NdisMResetComplete(
- p_adapter->h_adapter, NDIS_STATUS_SUCCESS, TRUE );
+ __ipoib_complete_reset( p_adapter, NDIS_STATUS_SUCCESS );
}
status = IB_SUCCESS;
break;
@@ -978,6 +998,7 @@
{
ib_api_status_t status;
ib_pnp_handle_t h_pnp;
+ KLOCK_QUEUE_HANDLE hdl;
IPOIB_ENTER( IPOIB_DBG_INIT );
@@ -992,8 +1013,17 @@
h_pnp = p_adapter->h_pnp;
p_adapter->h_pnp = NULL;
status = p_adapter->p_ifc->dereg_pnp( h_pnp, __ipoib_pnp_dereg );
+
// Wait until NDIS will return all indicated NBLs that were received
- shutter_shut( &p_adapter->recv_shutter );
+ // Avoid shutting the shutter twice
+ KeAcquireInStackQueuedSpinLock( &g_ipoib.lock, &hdl );
+ if ( p_adapter->ipoib_state == IPOIB_RUNNING ) {
+ shutter_shut( &p_adapter->recv_shutter );
+ // Notify that reset started and disable other shutter_shut
+ p_adapter->ipoib_state |= IPOIB_RESET_OR_DOWN;
+ }
+ KeReleaseInStackQueuedSpinLock( &hdl );
+
if( status == IB_SUCCESS )
status = IB_NOT_DONE;
}
@@ -1007,7 +1037,12 @@
p_adapter->reset = TRUE;
}
else {
- p_adapter->reset = FALSE;
+ //do not call to __ipoib_complete_reset, because we return completion status directly from here
+ p_adapter->reset = FALSE;
+ KeAcquireInStackQueuedSpinLock( &g_ipoib.lock, &hdl );
+ p_adapter->ipoib_state = p_adapter->ipoib_state & ~IPOIB_RESET_OR_DOWN;
+ shutter_alive( &p_adapter->recv_shutter );
+ KeReleaseInStackQueuedSpinLock( &hdl );
}
IPOIB_EXIT( IPOIB_DBG_INIT );
return status;
@@ -1074,19 +1109,15 @@
status = __ipoib_pnp_reg( p_adapter, IB_PNP_FLAG_REG_COMPLETE );
if( status != IB_SUCCESS )
{
- p_adapter->reset = FALSE;
IPOIB_PRINT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
("__ipoib_pnp_reg returned %s\n",
p_adapter->p_ifc->get_err_str( status )) );
- NdisMResetComplete(
- p_adapter->h_adapter, NDIS_STATUS_HARD_ERRORS, TRUE );
+ __ipoib_complete_reset( p_adapter, NDIS_STATUS_HARD_ERRORS );
}
}
else
{
- p_adapter->reset = FALSE;
- NdisMResetComplete(
- p_adapter->h_adapter, NDIS_STATUS_SUCCESS, TRUE );
+ __ipoib_complete_reset( p_adapter, NDIS_STATUS_SUCCESS );
status = IB_SUCCESS;
}
@@ -1261,10 +1292,7 @@
if( p_adapter->reset )
{
- //ASSERT(FALSE);
- p_adapter->reset = FALSE;
- NdisMResetComplete(
- p_adapter->h_adapter, NDIS_STATUS_SUCCESS, TRUE );
+ __ipoib_complete_reset( p_adapter, NDIS_STATUS_SUCCESS );
}
IPOIB_EXIT( IPOIB_DBG_INIT );
@@ -1323,9 +1351,7 @@
if( p_adapter->reset )
{
- p_adapter->reset = FALSE;
- NdisMResetComplete(
- p_adapter->h_adapter, NDIS_STATUS_SUCCESS, TRUE );
+ __ipoib_complete_reset( p_adapter, NDIS_STATUS_SUCCESS );
}
IPOIB_EXIT( IPOIB_DBG_INIT );
Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_adapter.h
===================================================================
--- ulp/ipoib_NDIS6_CM/kernel/ipoib_adapter.h (revision 2975)
+++ ulp/ipoib_NDIS6_CM/kernel/ipoib_adapter.h (working copy)
@@ -70,14 +70,14 @@
CSUM_BYPASS
} csum_flag_t;
-typedef enum _ipoib_state
-{
- IPOIB_INIT = -1,
- IPOIB_PAUSED,
- IPOIB_PAUSING,
- IPOIB_RUNNING
-} ipoib_state_t;
+typedef uint32_t ipoib_state_t;
+#define IPOIB_INIT 1
+#define IPOIB_PAUSED 2
+#define IPOIB_PAUSING 4
+#define IPOIB_RUNNING 8
+#define IPOIB_RESET_OR_DOWN 16
+
typedef struct _ipoib_offloads_cap_ {
boolean_t lso;
boolean_t send_chksum_offload;
Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_driver.cpp
===================================================================
--- ulp/ipoib_NDIS6_CM/kernel/ipoib_driver.cpp (revision 2975)
+++ ulp/ipoib_NDIS6_CM/kernel/ipoib_driver.cpp (working copy)
@@ -3155,8 +3155,8 @@
p_adapter = (ipoib_adapter_t*)adapter_context;
cl_obj_lock( &p_adapter->obj );
- if( p_adapter->ipoib_state == IPOIB_PAUSING ||
- p_adapter->ipoib_state == IPOIB_PAUSED)
+ if( p_adapter->ipoib_state & IPOIB_PAUSING ||
+ p_adapter->ipoib_state & IPOIB_PAUSED)
{
status = NDIS_STATUS_PAUSED;
IPOIB_PRINT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
@@ -3275,6 +3275,8 @@
IN NDIS_HANDLE adapter_context,
IN NDIS_SHUTDOWN_ACTION shutdown_action)
{
+ KLOCK_QUEUE_HANDLE hdl;
+
IPOIB_ENTER( IPOIB_DBG_INIT ) ;
UNUSED_PARAM( shutdown_action );
@@ -3287,7 +3289,16 @@
("Shutter shut, state = %d\n", p_adapter->ipoib_state));
IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_RECV,
("Shutter shut, state = %d\n", p_adapter->ipoib_state));
- shutter_shut ( &p_adapter->recv_shutter );
+ // Wait until NDIS will return all indicated NBLs that were received
+ // Avoid shutting the shutter twice
+ KeAcquireInStackQueuedSpinLock( &g_ipoib.lock, &hdl );
+ ASSERT ( !(p_adapter->ipoib_state & IPOIB_RESET_OR_DOWN) );
+ if ( p_adapter->ipoib_state == IPOIB_RUNNING ) { //ensure that there was no active reset
+ shutter_shut( &p_adapter->recv_shutter );
+ // Notify that shutter was already shut
+ p_adapter->ipoib_state |= IPOIB_RESET_OR_DOWN;
+ }
+ KeReleaseInStackQueuedSpinLock( &hdl );
}
IPOIB_EXIT( IPOIB_DBG_INIT );
@@ -3965,6 +3976,7 @@
cl_free( context );
}
+
static NDIS_STATUS
ipoib_pause(
IN NDIS_HANDLE adapter_context,
@@ -3972,28 +3984,37 @@
{
ipoib_adapter_t *p_adapter;
KLOCK_QUEUE_HANDLE hdl;
+ ipoib_state_t prev_state;
- UNREFERENCED_PARAMETER(pause_parameters);
+ UNREFERENCED_PARAMETER( pause_parameters );
IPOIB_ENTER( IPOIB_DBG_INIT );
- CL_ASSERT(adapter_context);
+ CL_ASSERT( adapter_context );
p_adapter = (ipoib_adapter_t*)adapter_context;
- CL_ASSERT(p_adapter->ipoib_state == IPOIB_RUNNING);
+
KeAcquireInStackQueuedSpinLock( &g_ipoib.lock, &hdl );
+ prev_state = p_adapter->ipoib_state;
+ CL_ASSERT ( prev_state & IPOIB_RUNNING );
p_adapter->ipoib_state = IPOIB_PAUSING;
KeReleaseInStackQueuedSpinLock( &hdl );
- if (p_adapter->p_port) {
+ if ( p_adapter->p_port ) {
cl_spinlock_acquire( &p_adapter->p_port->send_lock );
- ipoib_port_resume(p_adapter->p_port,FALSE);
+ ipoib_port_resume( p_adapter->p_port,FALSE );
cl_spinlock_release( &p_adapter->p_port->send_lock );
}
+ if ( prev_state == IPOIB_RUNNING ) { //i.e there was no active reset
+ IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_RECV,
+ ("Shutter shut, state = %d\n", p_adapter->ipoib_state) );
+ shutter_shut ( &p_adapter->recv_shutter );
+ }
+ else {
+ ASSERT ( prev_state & IPOIB_RESET_OR_DOWN);
+ IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
+ ("Got reset when IPoIB port wasn't active\n") );
+ }
- IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_RECV,
- ("Shutter shut, state = %d\n", p_adapter->ipoib_state));
- shutter_shut ( &p_adapter->recv_shutter );
-
KeAcquireInStackQueuedSpinLock( &g_ipoib.lock, &hdl );
p_adapter->ipoib_state = IPOIB_PAUSED;
KeReleaseInStackQueuedSpinLock( &hdl );
@@ -4025,7 +4046,7 @@
// Check to see if we need to change any attributes
}
- if ( (p_adapter->ipoib_state == IPOIB_PAUSED) || (p_adapter->ipoib_state == IPOIB_INIT) ) {
+ if ( (p_adapter->ipoib_state & IPOIB_PAUSED) || (p_adapter->ipoib_state & IPOIB_INIT) ) {
IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_RECV,
("Shutter Alive, ipoib_state = %d\n", p_adapter->ipoib_state));
shutter_alive( &p_adapter->recv_shutter );
Alexander (XaleX) Naslednikov
SW Networking Team
Mellanox Technologies
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20101026/b1afdb8e/attachment.html>
More information about the ofw
mailing list