[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