[ofw] patch: [ibal] use safe function for working on mutex.
Smith, Stan
stan.smith at intel.com
Tue Oct 28 08:58:46 PDT 2008
Hello Tzachi,
While driving home last night I had some additional thoughts on the FastMutex problem.
Stepping back from the problem, I realized the only place where the ExAcquireFastMutex() is incorrectly used is over the call to al_initialize() in the bus driver start routine fdo_start(). Incorrect in that thread creation occurs within al_initialize() and the FastMutex alters IRQL. I believe all other uses of the FastMutex calls are correct.
Since al_initialize() needs to be called exactly one time, the FastMutex was added around the al_initialize() call in fdo_start() as the driver start routine is called for each HCA in the system; effect of being an HCA filter driver.
A simple fix, would be to use an atomic exchange lock barrier around the al_initialize() call in fdo_start() instead of the FastMutex calls. The reason this could be superior to moving the al_initialize() call to DriverStart() is due to the need to make the IoSetDeviceInterfaceState() calls for al_ifc_name and ci_ifc_name in the first fdo_start() call as we do not want users calling IBAL too early.
Comments?
thank you,
stan.
________________________________
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.
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/ba69097a/attachment.html>
More information about the ofw
mailing list