[ofw] [IPoIB_NDIS6_CM][Patch] Improving shutter mechanism for recv flow
Fab Tillier
ftillier at microsoft.com
Wed Oct 27 11:30:56 PDT 2010
Shouldn't this fix be checked in, and then a different fix for the ipoib_reset/pause issue?
Seems like holding off on patches just to make them bigger makes it harder in to review changes in the future. Small changes are easier to digest.
Cheers,
-Fab
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Wednesday, October 27, 2010 8:31 AM
To: Smith, Stan; ofw at lists.openfabrics.org
Subject: Re: [ofw] [IPoIB_NDIS6_CM][Patch] Improving shutter mechanism for recv flow
Please, wait one day more !
We found a case when ipoib got pause during reset process
(i.e. ipoib_reset() and return pending status, than ipoib_pause() called, before the reset was completed)
I have a fix also for this situation, but I would like to test this with the regression suite)
XaleX
From: Smith, Stan [mailto:stan.smith at intel.com]
Sent: Wednesday, October 27, 2010 5:26 PM
To: Alex Naslednikov; ofw at lists.openfabrics.org
Subject: RE: [ofw] [IPoIB_NDIS6_CM][Patch] Improving shutter mechanism for recv flow
Hello,
Patch tests out with no problems - expected behavior.
Would you like for me to svn commit to trunk and WOF2-3 branch?
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/20101027/93e4d895/attachment.html>
More information about the ofw
mailing list