[ofw] patch: [ibal] use safe function for working on mutex.

Tzachi Dar tzachid at mellanox.co.il
Tue Oct 28 02:40:10 PDT 2008


Applied on 1700, 1701
 
Thanks
Tzachi


________________________________

	From: Smith, Stan [mailto:stan.smith at intel.com] 
	Sent: Tuesday, October 28, 2008 1:45 AM
	To: Tzachi Dar; ofw at lists.openfabrics.org
	Subject: RE: [ofw] patch: [ibal] use safe function for working
on mutex.
	
	
	Thanks for taking the time to explain.
	See comments inline.

________________________________

	From: Tzachi Dar [mailto:tzachid at mellanox.co.il] 
	Sent: Monday, October 27, 2008 3:07 PM
	To: Smith, Stan; ofw at lists.openfabrics.org
	Subject: RE: [ofw] patch: [ibal] use safe function for working
on mutex.
	
	
	Hi,
	 
	The original code was talking about ExAcquireFastMutexUnsafe.
	MSDN says: 
	ExAcquireFastMutexUnsafe can be safely called only at IRQL =
APC_LEVEL. Use ExAcquireFastMutex
<ms-help://MS.WDK.v10.6001.071220/Kernel_r/hh/Kernel_r/k102_5f581434-dda
0-4560-8867-3fa7f590e447.xml.htm>  if the caller might run at IRQL <
APC_LEVEL.
	Since we are not running at IRQL = APC_LEVEL it can not be used
and we should move to ExAcquireFastMutex
<ms-help://MS.WDK.v10.6001.071220/Kernel_r/hh/Kernel_r/k102_5f581434-dda
0-4560-8867-3fa7f590e447.xml.htm>  .
	 
	But ExAcquireFastMutex
<ms-help://MS.WDK.v10.6001.071220/Kernel_r/hh/Kernel_r/k102_5f581434-dda
0-4560-8867-3fa7f590e447.xml.htm>  indeed moves us to APC_LEVEL. Under
this lock, we call PsCreateSystemThread which must be run at  IRQL:
PASSIVE_LEVEL. So again not good. 
	 
	 
	stan> 
	ExAcquireFastMutex() exclusion is only for a very short code
path, with the original level restored by ExReleaseFastMutex. 
	Then it's the call to al_initialize() you are worried about. OK,
I understand. 
	 
	 
	Please also note that:
	"ExAcquireFastMutex puts the caller into a wait state if the
given fast mutex cannot be acquired immediately. Otherwise, the caller
is given ownership of the fast mutex with APCs to the current thread
disabled until it releases the fast mutex." Not that bad, but not
exactly the side affect we were thinking about.
	 
	So, I want to "normal mutex": (KeInitializeMutex) but then MSDN
says: "Only highest-level drivers, such as FSDs that use executive
worker threads, are likely to use a mutex object.". This is not our
case.
	Also it says: "Acquiring ownership of a mutex prevents the
delivery of normal kernel-mode asynchronous procedure calls (APCs). The
thread will not be preempted by an APC unless the kernel issues an
APC_LEVEL software interrupt to run a special kernel APC, such as the
I/O manager's IRP completion routine that returns results to the
original requester of an I/O operation" Again a side affect not that
desired.
	 
	For this reasons I believe that events are the best choice. 
	 
	stan> Agreed - thanks for working me thru this. 
	 
	Thanks
	Tzachi


________________________________

		From: Smith, Stan [mailto:stan.smith at intel.com] 
		Sent: Monday, October 27, 2008 10:03 PM
		To: Tzachi Dar; ofw at lists.openfabrics.org
		Subject: RE: [ofw] patch: [ibal] use safe function for
working on mutex.
		
		
		 

________________________________

		From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Tzachi Dar
		Sent: Monday, October 27, 2008 10:21 AM
		To: Tzachi Dar; ofw at lists.openfabrics.org
		Subject: RE: [ofw] patch: [ibal] use safe function for
working on mutex.
		
		
		After some more experiments I have realized that working
with mutex is not good enough as well since they tend to bring the user
to APC_LEVEL. 
		 
		Could you explain why the mutex is insufficient?
		What is the behavior you see that makes you believe the
mutex is not providing mutual exclusion?
		At what code point is the IRQL @ dispatch and/or in an
interrupt service routine? 
		PNP callbacks are in an AL thread context (passive).
		 
		thanks,
		 
		stan.
		 
		So I suggest to replace them to events.
		 
		The following patch was generated (but not tested yet):
		 
		Thanks
		Tzachi
		 
		 
		Index: bus_driver.c
	
===================================================================
		--- bus_driver.c (revision 3372)
		+++ bus_driver.c (working copy)
		@@ -799,7 +799,7 @@
		  // Mutex to synchronize multiple threads creating &
deleting 
		  // control deviceobjects. 
		 
		- ExInitializeFastMutex(&g_ControlMutex);
		+ KeInitializeEvent(&g_ControlEvent,
SynchronizationEvent, TRUE);
		  g_bfi_InstanceCount = 0;
		  memset(  __out_bcount(sizeof(g_bus_filters))
(void*)g_bus_filters, 0,
		    sizeof(g_bus_filters) );
		Index: bus_driver.h
	
===================================================================
		--- bus_driver.h (revision 3372)
		+++ bus_driver.h (working copy)
		@@ -255,7 +255,7 @@
		 
		 extern bus_filter_t g_bus_filters[MAX_BUS_FILTERS];
		 extern ULONG g_bfi_InstanceCount;
		-extern FAST_MUTEX g_ControlMutex; // serializes
InstanceCount & g_bus_filters
		+extern KEVENT g_ControlEvent; // serializes
InstanceCount & g_bus_filters
		 
		 extern bus_filter_t *alloc_bfi( IN DRIVER_OBJECT *, OUT
int * );
		 extern int free_bfi( IN bus_filter_t *p_bfi );
		@@ -265,4 +265,12 @@
		 extern bus_filter_t *get_set_bfi_by_ca_guid( IN net64_t
ca_guid );
		 extern char *get_obj_state_str(cl_state_t state);
		 
		+inline VOID lock_control_event() {
		+ KeWaitForSingleObject(&g_ControlEvent, Executive,
KernelMode , FALSE, NULL);
		+}
		+
		+inline VOID unlock_control_event() {
		+ KeSetEvent(&g_ControlEvent, 0, FALSE);
		+}
		+
		 #endif /* !defined _BUS_DRIVER_H_ */
		Index: bus_iou_mgr.c
	
===================================================================
		--- bus_iou_mgr.c (revision 3373)
		+++ bus_iou_mgr.c (working copy)
		@@ -483,13 +483,13 @@
		   */
		  if ( !bus_globals.h_pnp_iou )
		  {
		-  ExAcquireFastMutex(&g_ControlMutex);
		+  lock_control_event();
		   if ( !bus_globals.h_pnp_iou )
		   {
		    bus_globals.h_pnp_iou = (ib_pnp_handle_t)1; /* block
others */ 
		    need_pnp_reg = TRUE;
		   }
		-  ExReleaseFastMutex(&g_ControlMutex);
		+  unlock_control_event();
		 
		   if ( need_pnp_reg )
		   {
		Index: bus_pnp.c
	
===================================================================
		--- bus_pnp.c (revision 3373)
		+++ bus_pnp.c (working copy)
		@@ -51,7 +51,7 @@
		 static UNICODE_STRING al_ifc_name;
		 static UNICODE_STRING ci_ifc_name;
		 
		-FAST_MUTEX    g_ControlMutex;
		+KEVENT     g_ControlEvent;
		 ULONG     g_bfi_InstanceCount;
		 bus_filter_t   g_bus_filters[MAX_BUS_FILTERS];
		 
		@@ -334,7 +334,7 @@
		   return status;
		  }
		 
		- ExAcquireFastMutex(&g_ControlMutex);
		+ lock_control_event();
		  if ( !gh_al ) {
		   /* Initialize AL */
		   ib_status = al_initialize();
		@@ -343,12 +343,12 @@
		    al_cleanup();
		    BUS_TRACE_EXIT( BUS_DBG_ERROR, ("al_initialize
returned %s.\n",
		        ib_get_err_str(ib_status)) );
		-   ExReleaseFastMutex(&g_ControlMutex);
		+   unlock_control_event();
		    return STATUS_UNSUCCESSFUL;
		   }
		   AL_init_here = TRUE;
		  }
		- ExReleaseFastMutex(&g_ControlMutex);
		+ unlock_control_event();
		 
		  /* Initialize the port manager. */
		  ib_status = create_port_mgr( p_ext->bus_filter,
&p_ext->p_port_mgr );
		@@ -1252,7 +1252,7 @@
		 
		  CL_ASSERT((obj_type == BFI_PORT_MGR_OBJ) || (obj_type
== BFI_IOU_MGR_OBJ));
		 
		-    ExAcquireFastMutex(&g_ControlMutex);
		+ lock_control_event();
		 
		  for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++) {
		 
		@@ -1272,7 +1272,7 @@
		    }
		   }
		  }
		- ExReleaseFastMutex(&g_ControlMutex);
		+ unlock_control_event();
		 
		  BUS_PRINT( BUS_DBG_PNP,
		     ("%s() cl_obj %p type %s_MGR_OBJ --> bfi[%d] %p\n",
		@@ -1302,7 +1302,7 @@
		   return matched;
		  }
		 
		- ExAcquireFastMutex(&g_ControlMutex);
		+ lock_control_event();
		 
		  for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++)
		  {
		@@ -1315,7 +1315,7 @@
		    break;
		   }
		  }
		- ExReleaseFastMutex(&g_ControlMutex);
		+ unlock_control_event();
		 
		 #if DBG
		  if ( !matched )
		@@ -1376,7 +1376,7 @@
		   */
		  if ( !matched )
		  {
		-  ExAcquireFastMutex(&g_ControlMutex);
		+  lock_control_event();
		 
		   for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++)
		   {
		@@ -1391,7 +1391,7 @@
		     break;
		    }
		   }
		-  ExReleaseFastMutex(&g_ControlMutex);
		+  unlock_control_event();
		  }
		 
		  BUS_PRINT( BUS_DBG_PNP,
		@@ -1413,7 +1413,7 @@
		      * IoCreateDeviceSecure & IoCreateSymbolicLink must
be called at
		      * PASSIVE_LEVEL.
		   */
		- ExAcquireFastMutex(&g_ControlMutex);
		+ lock_control_event();
		 
		  // find 1st unused bfi slot.
		  for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++)
		@@ -1430,7 +1430,7 @@
		    break;
		   }
		  }
		- ExReleaseFastMutex(&g_ControlMutex);
		+ unlock_control_event();
		 
		 #if DBG
		  RtlStringCbPrintfA ( p_bfi->whoami,
		@@ -1453,12 +1453,12 @@
		 {
		  int remaining;
		 
		- ExAcquireFastMutex(&g_ControlMutex);
		+ lock_control_event();
		  p_bfi->p_bus_ext = NULL;
		  p_bfi->ca_guid = 0ULL;
		  remaining = --g_bfi_InstanceCount; // one less bfi
in-use
		- ExReleaseFastMutex(&g_ControlMutex);
		-
		+ unlock_control_event();
		+ 
		  return remaining;
		 }
		 
		@@ -1467,10 +1467,10 @@
		 {
		  int ic;
		 
		- ExAcquireFastMutex(&g_ControlMutex);
		+ lock_control_event();
		  ic = g_bfi_InstanceCount;
		- ExReleaseFastMutex(&g_ControlMutex);
		-
		+ unlock_control_event();
		+ 
		  return ic;
		 }
		 
		Index: bus_port_mgr.c
	
===================================================================
		--- bus_port_mgr.c (revision 3373)
		+++ bus_port_mgr.c (working copy)
		@@ -483,12 +483,12 @@
		   */
		  if ( !bus_globals.h_pnp_port )
		  {
		-  ExAcquireFastMutex(&g_ControlMutex);
		+  lock_control_event();
		   if ( !bus_globals.h_pnp_port ) {
		    bus_globals.h_pnp_port = (ib_pnp_handle_t)1; /*
block others */
		    need_pnp_reg = TRUE;
		   }
		-  ExReleaseFastMutex(&g_ControlMutex);
		+  unlock_control_event();
		 
		   if ( need_pnp_reg )
		   {
		@@ -1005,10 +1005,10 @@
		   if ( !p_bfi->p_bus_ext )
		    continue;
		   GO = FALSE;
		-  ExAcquireFastMutex(&g_ControlMutex);
		+  lock_control_event();
		   if ( p_bfi->ca_guid && p_bfi->p_port_mgr )
		    GO = TRUE;
		-  ExReleaseFastMutex(&g_ControlMutex);
		+  unlock_control_event();
		   if ( GO == FALSE )
		    continue;
		   status = _port_mgr_pkey_rem( pkeys, p_bfi->p_port_mgr
);
		@@ -1149,10 +1149,10 @@
		   if ( !p_bfi->p_bus_ext )
		    continue;
		   GO = FALSE;
		-  ExAcquireFastMutex(&g_ControlMutex);
		+  lock_control_event();
		   if ( p_bfi->ca_guid && p_bfi->p_port_mgr )
		    GO = TRUE;
		-  ExReleaseFastMutex(&g_ControlMutex);
		+  unlock_control_event();
		   if ( GO == FALSE )
		    continue;
		   status = _port_mgr_pkey_add( pkeys, p_bfi,
p_bfi->p_port_mgr );
		


________________________________

			From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Tzachi Dar
			Sent: Monday, October 27, 2008 4:58 PM
			To: ofw at lists.openfabrics.org
			Subject: [ofw] patch: [ibal] use safe function
for working on mutex.
			
			
			The unsafe functions can only be used from apc
level. We call them from passive level, so we use the safe functions.
			 
			Index:
Q:/projinf3/trunk/core/bus/kernel/bus_port_mgr.c
	
===================================================================
			---
Q:/projinf3/trunk/core/bus/kernel/bus_port_mgr.c (revision 3372)
			+++
Q:/projinf3/trunk/core/bus/kernel/bus_port_mgr.c (revision 3373)
			@@ -483,12 +483,12 @@
			   */
			  if ( !bus_globals.h_pnp_port )
			  {
			-  ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+  ExAcquireFastMutex(&g_ControlMutex);
			   if ( !bus_globals.h_pnp_port ) {
			    bus_globals.h_pnp_port = (ib_pnp_handle_t)1;
/* block others */
			    need_pnp_reg = TRUE;
			   }
			-  ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+  ExReleaseFastMutex(&g_ControlMutex);
			 
			   if ( need_pnp_reg )
			   {
			@@ -1005,10 +1005,10 @@
			   if ( !p_bfi->p_bus_ext )
			    continue;
			   GO = FALSE;
			-  ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+  ExAcquireFastMutex(&g_ControlMutex);
			   if ( p_bfi->ca_guid && p_bfi->p_port_mgr )
			    GO = TRUE;
			-  ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+  ExReleaseFastMutex(&g_ControlMutex);
			   if ( GO == FALSE )
			    continue;
			   status = _port_mgr_pkey_rem( pkeys,
p_bfi->p_port_mgr );
			@@ -1149,10 +1149,10 @@
			   if ( !p_bfi->p_bus_ext )
			    continue;
			   GO = FALSE;
			-  ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+  ExAcquireFastMutex(&g_ControlMutex);
			   if ( p_bfi->ca_guid && p_bfi->p_port_mgr )
			    GO = TRUE;
			-  ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+  ExReleaseFastMutex(&g_ControlMutex);
			   if ( GO == FALSE )
			    continue;
			   status = _port_mgr_pkey_add( pkeys, p_bfi,
p_bfi->p_port_mgr );
			Index:
Q:/projinf3/trunk/core/bus/kernel/bus_pnp.c
	
===================================================================
			--- Q:/projinf3/trunk/core/bus/kernel/bus_pnp.c
(revision 3372)
			+++ Q:/projinf3/trunk/core/bus/kernel/bus_pnp.c
(revision 3373)
			@@ -334,7 +334,7 @@
			   return status;
			  }
			 
			- ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+ ExAcquireFastMutex(&g_ControlMutex);
			  if ( !gh_al ) {
			   /* Initialize AL */
			   ib_status = al_initialize();
			@@ -343,12 +343,12 @@
			    al_cleanup();
			    BUS_TRACE_EXIT( BUS_DBG_ERROR,
("al_initialize returned %s.\n",
			        ib_get_err_str(ib_status)) );
			-   ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+   ExReleaseFastMutex(&g_ControlMutex);
			    return STATUS_UNSUCCESSFUL;
			   }
			   AL_init_here = TRUE;
			  }
			- ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+ ExReleaseFastMutex(&g_ControlMutex);
			 
			  /* Initialize the port manager. */
			  ib_status = create_port_mgr(
p_ext->bus_filter, &p_ext->p_port_mgr );
			@@ -1252,7 +1252,7 @@
			 
			  CL_ASSERT((obj_type == BFI_PORT_MGR_OBJ) ||
(obj_type == BFI_IOU_MGR_OBJ));
			 
			-    ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+    ExAcquireFastMutex(&g_ControlMutex);
			 
			  for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++) {
			 
			@@ -1272,7 +1272,7 @@
			    }
			   }
			  }
			- ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+ ExReleaseFastMutex(&g_ControlMutex);
			 
			  BUS_PRINT( BUS_DBG_PNP,
			     ("%s() cl_obj %p type %s_MGR_OBJ -->
bfi[%d] %p\n",
			@@ -1302,7 +1302,7 @@
			   return matched;
			  }
			 
			- ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+ ExAcquireFastMutex(&g_ControlMutex);
			 
			  for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++)
			  {
			@@ -1315,7 +1315,7 @@
			    break;
			   }
			  }
			- ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+ ExReleaseFastMutex(&g_ControlMutex);
			 
			 #if DBG
			  if ( !matched )
			@@ -1376,7 +1376,7 @@
			   */
			  if ( !matched )
			  {
			-  ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+  ExAcquireFastMutex(&g_ControlMutex);
			 
			   for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++)
			   {
			@@ -1391,7 +1391,7 @@
			     break;
			    }
			   }
			-  ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+  ExReleaseFastMutex(&g_ControlMutex);
			  }
			 
			  BUS_PRINT( BUS_DBG_PNP,
			@@ -1413,7 +1413,7 @@
			      * IoCreateDeviceSecure &
IoCreateSymbolicLink must be called at
			      * PASSIVE_LEVEL.
			   */
			- ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+ ExAcquireFastMutex(&g_ControlMutex);
			 
			  // find 1st unused bfi slot.
			  for(p_bfi=g_bus_filters; p_bfi <
&g_bus_filters[MAX_BUS_FILTERS]; p_bfi++)
			@@ -1430,7 +1430,7 @@
			    break;
			   }
			  }
			- ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+ ExReleaseFastMutex(&g_ControlMutex);
			 
			 #if DBG
			  RtlStringCbPrintfA ( p_bfi->whoami,
			@@ -1453,11 +1453,11 @@
			 {
			  int remaining;
			 
			- ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+ ExAcquireFastMutex(&g_ControlMutex);
			  p_bfi->p_bus_ext = NULL;
			  p_bfi->ca_guid = 0ULL;
			  remaining = --g_bfi_InstanceCount; // one less
bfi in-use
			- ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+ ExReleaseFastMutex(&g_ControlMutex);
			 
			  return remaining;
			 }
			@@ -1467,9 +1467,9 @@
			 {
			  int ic;
			 
			- ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+ ExAcquireFastMutex(&g_ControlMutex);
			  ic = g_bfi_InstanceCount;
			- ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+ ExReleaseFastMutex(&g_ControlMutex);
			 
			  return ic;
			 }
			Index:
Q:/projinf3/trunk/core/bus/kernel/bus_iou_mgr.c
	
===================================================================
			---
Q:/projinf3/trunk/core/bus/kernel/bus_iou_mgr.c (revision 3372)
			+++
Q:/projinf3/trunk/core/bus/kernel/bus_iou_mgr.c (revision 3373)
			@@ -483,13 +483,13 @@
			   */
			  if ( !bus_globals.h_pnp_iou )
			  {
			-  ExAcquireFastMutexUnsafe(&g_ControlMutex);
			+  ExAcquireFastMutex(&g_ControlMutex);
			   if ( !bus_globals.h_pnp_iou )
			   {
			    bus_globals.h_pnp_iou = (ib_pnp_handle_t)1;
/* block others */ 
			    need_pnp_reg = TRUE;
			   }
			-  ExReleaseFastMutexUnsafe(&g_ControlMutex);
			+  ExReleaseFastMutex(&g_ControlMutex);
			 
			   if ( need_pnp_reg )
			   {
			
			 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20081028/031646f4/attachment.html>


More information about the ofw mailing list