[ofw] patch: [ibal] use safe function for working on mutex.
Tzachi Dar
tzachid at mellanox.co.il
Mon Oct 27 15:06:42 PDT 2008
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.
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.
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/f685f899/attachment.html>
More information about the ofw
mailing list