[ofw] patch: [ibal] use safe function for working on mutex.
Smith, Stan
stan.smith at intel.com
Mon Oct 27 16:45:03 PDT 2008
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-dda0-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-dda0-4560-8867-3fa7f590e447.xml.htm> .
But ExAcquireFastMutex<ms-help://MS.WDK.v10.6001.071220/Kernel_r/hh/Kernel_r/k102_5f581434-dda0-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/20081027/f2c87d73/attachment.html>
More information about the ofw
mailing list