[Openib-windows] Using of fast mutexes in WinIb

[Leonid Keller]

Hi Fab, 
I'm sending you a patch, that gives a particular solution to one case of
using fast mutexes, which increase IRQL and prevent using of
AllocateBuffer (to remind - see below).
I've done that as you once suggested - by using usual mutexes (instead
of cl_mutex) in the file in question.


Index: E:/svn.wininf/branches/ulp/ipoib/kernel/ipoib_adapter.h
===================================================================
--- E:/svn.wininf/branches/ulp/ipoib/kernel/ipoib_adapter.h
(revision 1005)
+++ E:/svn.wininf/branches/ulp/ipoib/kernel/ipoib_adapter.h
(revision 1032)
@@ -161,9 +161,7 @@
 	uint8_t					mcast_array_size;
 
 	cl_qpool_t				item_pool;
-
-	cl_mutex_t				mutex;
-
+	KMUTEX					mutex;
 	cl_vector_t				ip_vector;
 
 	cl_perf_t				perf;



Index: E:/svn.wininf/branches/ulp/ipoib/kernel/ipoib_adapter.c
===================================================================
--- E:/svn.wininf/branches/ulp/ipoib/kernel/ipoib_adapter.c
(revision 1005)
+++ E:/svn.wininf/branches/ulp/ipoib/kernel/ipoib_adapter.c
(revision 1032)
@@ -234,7 +234,7 @@
 	 * between destruction and AL callbacks (PnP, Query,
Destruction).
 	 * The lock provides protection
 	 */
-	cl_mutex_acquire( &p_adapter->mutex );
+	KeWaitForMutexObject( &p_adapter->mutex, Executive, KernelMode,
FALSE,  NULL );
 	cl_obj_lock( &p_adapter->obj );
 	p_adapter->state = IB_PNP_PORT_REMOVE;
 
@@ -247,7 +247,7 @@
 
 	cl_obj_unlock( &p_adapter->obj );
 
-	cl_mutex_release( &p_adapter->mutex );
+	KeReleaseMutex( &p_adapter->mutex, FALSE );
 
 	cl_obj_destroy( &p_adapter->obj );
 
@@ -263,7 +263,6 @@
 	cl_spinlock_construct( &p_adapter->send_stat_lock );
 	cl_spinlock_construct( &p_adapter->recv_stat_lock );
 	cl_qpool_construct( &p_adapter->item_pool );
-	cl_mutex_construct( &p_adapter->mutex );
 	cl_vector_construct( &p_adapter->ip_vector );
 
 	cl_perf_construct( &p_adapter->perf );
@@ -315,13 +314,7 @@
 		return IB_ERROR;
 	}
 
-	cl_status = cl_mutex_init( &p_adapter->mutex );
-	if( cl_status != CL_SUCCESS )
-	{
-		IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR,
-			("cl_mutex_init returned %s\n",
cl_status_text[cl_status]) );
-		return IB_ERROR;
-	}
+	KeInitializeMutex( &p_adapter->mutex, 0 );
 
 	/* We manually manage the size and capacity of the vector. */
 	cl_status = cl_vector_init( &p_adapter->ip_vector, 0,
@@ -463,7 +456,6 @@
 	cl_qpool_destroy( &p_adapter->item_pool );
 	cl_spinlock_destroy( &p_adapter->recv_stat_lock );
 	cl_spinlock_destroy( &p_adapter->send_stat_lock );
-	cl_mutex_destroy( &p_adapter->mutex );
 	cl_obj_deinit( p_obj );
 
 	cl_perf_destroy( &p_adapter->perf, TRUE );
@@ -493,13 +485,13 @@
 	CL_ASSERT( p_adapter );
 
 	/* Synchronize with destruction */
-	cl_mutex_acquire( &p_adapter->mutex );
+	KeWaitForMutexObject( &p_adapter->mutex, Executive, KernelMode,
FALSE,  NULL );
 	cl_obj_lock( &p_adapter->obj );
 	old_state = p_adapter->state;
 	cl_obj_unlock( &p_adapter->obj );
 	if( old_state == IB_PNP_PORT_REMOVE )
 	{
-		cl_mutex_release( &p_adapter->mutex );
+		KeReleaseMutex( &p_adapter->mutex, FALSE );
 		IPOIB_TRACE_EXIT( IPOIB_DBG_PNP,
 			("Aborting - Adapter destroying.\n") );
 		return IB_NOT_DONE;
@@ -632,7 +624,7 @@
 		break;
 	}
 
-	cl_mutex_release( &p_adapter->mutex );
+	KeReleaseMutex( &p_adapter->mutex, FALSE );
 
 	IPOIB_EXIT( IPOIB_DBG_PNP );
 	return status;
@@ -750,8 +742,7 @@
 	p_adapter = PARENT_STRUCT( context, ipoib_adapter_t, obj );
 
 	/* Synchronize with destruction */
-	cl_mutex_acquire( &p_adapter->mutex );
-
+	KeWaitForMutexObject( &p_adapter->mutex, Executive, KernelMode,
FALSE,  NULL );
 	cl_obj_lock( &p_adapter->obj );
 
 	CL_ASSERT( !p_adapter->h_pnp );
@@ -797,7 +788,7 @@
 	/* Dereference the adapter since the previous registration is
now gone. */
 	cl_obj_deref( &p_adapter->obj );
 
-	cl_mutex_release( &p_adapter->mutex );
+	KeReleaseMutex( &p_adapter->mutex, FALSE );
 
 	IPOIB_EXIT( IPOIB_DBG_INIT );
 }






 

> -----Original Message-----
> From: Leonid Keller 
> Sent: Thursday, November 17, 2005 4:32 PM
> To: 'Fab Tillier'; Leonid Keller
> Cc: openib-windows at openib.org
> Subject: RE: [Openib-windows] Using of fast mutexes in WinIb
> 
> SB
> 
> > -----Original Message-----
> > From: Fab Tillier [mailto:ftillier at silverstorm.com]
> > Sent: Wednesday, November 16, 2005 7:24 PM
> > To: 'Leonid Keller'
> > Cc: openib-windows at openib.org
> > Subject: RE: [Openib-windows] Using of fast mutexes in WinIb
> > 
> > 
> > Hi Leo,
> > 
> > > From: Leonid Keller [mailto:leonid at mellanox.co.il]
> > > Sent: Tuesday, November 15, 2005 7:32 AM
> > > 
> > > Hi Fab,
> > > I come across the following problem: implementation of
> > > cl_mutex_acquire() via Fast Mutexes causes all the code 
> in critical 
> > > section to work at APC_LEVEL.
> > > 
> > > The first case i saw, was at the start-up of IpoIb driver,
> > which takes
> > > mutex in __ipoib_pnp_cb and makes all MTHCA driver control
> > verbs to work
> > > at APC_LEVEL, which is troublesome.
> > > (e.g., create_cq calls AllocateCommonBuffer, which requires 
> > > PASSIVE_LEVEL).
> > 
> > Why does create_cq call AllocateCommonBuffer?  Are you 
> making multiple 
> > page-sized calls instead of a single larger call for the 
> cases where 
> > the memory required spans multiple pages?  Physically contiguous 
> > memory is a scarce resource, so any time you can break up your 
> > requests into page sized requests the better.
> 
> The algorithm of allocating tries to allocate one contiguous buffer.
> If it fails, it requests lesser buffers. In the worst case it 
> will allocate N buffers 1 page size each.
> 
> I used AllocateCommonBuffer in the first time, because it 
> returns bus addresses.
> 
> One can implement that for the work at DISPATCH_LEVEL:
> 	va = MmAllocateContiguousMemorySpecifyCache(...);
> 	p_mdl = IoAllocateMdl( va, ...);
> 	MmBuildMdlForNonPagedPool( p_mdl );
> 	la = p_adapter->MapTransfer(adapter, p_mdl ...);
> 
> It has 2 little drawbacks as far as i see:
> 	1) MmAllocateContiguousMemorySpecifyCache allocates 
> always an integer number of pages;
> 	2) MapTransfer fails, when the number of map registers 
> gets exceeded. (But maybe AllocateCommonBuffer will also fail 
> in this case)
> 
> What do you think ?
> 	
> 
> > 
> > Is this for the CQE ring buffer?  Why not use regular memory? 
> >  The memory has to
> > be registered with the HCA anyway, right?
> > 
> > > There are several ways to solve the problem:
> > > 	1) Implement cl_mutex_acquire() via regular mutexes. It is a 
> > > general and secure, but a bit ineffective way.
> > > 	2) Add to complib "regular mutexes" and use them in 
> that function. 
> > > It's more effective, but then we need  to
> > check/fix all other
> > > uses of fast mutexes.
> > 
> > I don't think we should add a new mutex abstraction to complib that 
> > only has meaning in the Windows kernel.  I'd much rather 
> see the code 
> > just make native NT calls for regular mutexes.
> > 
> > > 	3) Invent some other sync mechanism for that function. 
> It lefts the 
> > > problem with fast mutexes in other functions.
> > > 
> > > What do you think ?
> > 
> > Can we change these on a case-by-case basis?  That is, change the 
> > mutex in IPoIB's PnP callback to a regular mutex for now, 
> and change 
> > any others as we go forward?
> > 
> 
> > Finding incorrect IRQL issues should be quick if your calls assert 
> > that IRQL is correct.
> > 
> > BTW, have you had a chance to look over the new verb API that
> 
> Not yet, sorry. Occupied by preparing the driver to the release.
> I hope, i'll find time soon ...
> 
> > would allow calls
> > at IRQL <= DISPATCH_LEVEL?  Any comments?  I'd really like 
> to get past 
> > these IRQL limitations.
> > 
> > - Fab
> > 
>