Re: [ros-dev] [ros-diffs] [cgutman] 42400: - Partial rewrite of recursive mutex code - Makes the recursive mutex faster and smaller - Fixes several unprotected accesses to recursive mutex

[Alex Ionescu]

Mutexes are already recursive, while fast-mutexes are not so I still  
don't understand the point of this library. Just use a KMUTEX.

On 4-Aug-09, at 4:51 PM, [e-mail] wrote:
> Author: cgutman
> Date: Wed Aug  5 01:51:39 2009
> New Revision: 42400
>
> URL: http://svn.reactos.org/svn/reactos?rev=42400&view=rev
> Log:
> Partial rewrite of recursive mutex code
> Makes the recursive mutex faster and smaller
> Fixes several unprotected accesses to recursive mutex
>
> Modified:
>    trunk/reactos/drivers/network/tcpip/include/lock.h
>    trunk/reactos/drivers/network/tcpip/recmutex/recmutex.c
>    trunk/reactos/drivers/network/tcpip/recmutex/recmutex.h
>    trunk/reactos/drivers/network/tcpip/tcpip/lock.c
>
> Modified: trunk/reactos/drivers/network/tcpip/include/lock.h
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/inclu...
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/reactos/drivers/network/tcpip/include/lock.h [iso-8859-1]  
> (original)
> +++ trunk/reactos/drivers/network/tcpip/include/lock.h [iso-8859-1]  
> Wed Aug  5 01:51:39 2009
> @@ -13,7 +13,7 @@
> extern VOID TcpipAcquireFastMutex( PFAST_MUTEX Mutex );
> extern VOID TcpipReleaseFastMutex( PFAST_MUTEX Mutex );
> extern VOID TcpipRecursiveMutexInit( PRECURSIVE_MUTEX RecMutex );
> -extern UINT TcpipRecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex,
> +extern VOID TcpipRecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex,
>    			      BOOLEAN ToWrite );
> extern VOID TcpipRecursiveMutexLeave( PRECURSIVE_MUTEX RecMutex );
>
> Modified: trunk/reactos/drivers/network/tcpip/recmutex/recmutex.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/recmu...
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/reactos/drivers/network/tcpip/recmutex/recmutex.c  
> [iso-8859-1] (original)
> +++ trunk/reactos/drivers/network/tcpip/recmutex/recmutex.c  
> [iso-8859-1] Wed Aug  5 01:51:39 2009
> @@ -3,7 +3,6 @@
>
> VOID RecursiveMutexInit( PRECURSIVE_MUTEX RecMutex ) {
>     RtlZeroMemory( RecMutex, sizeof(*RecMutex) );
> KeInitializeSpinLock( &RecMutex->SpinLock );
> ExInitializeFastMutex( &RecMutex->Mutex );
> KeInitializeEvent( &RecMutex->StateLockedEvent,
>    NotificationEvent, FALSE );
>
> @@ -11,76 +10,53 @@
>
> /* NOTE: When we leave, the FAST_MUTEX must have been released.  The  
> result
> is that we always exit in the same irql as entering */
>
> -SIZE_T RecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex, BOOLEAN  
> ToWrite ) {
> +VOID RecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex ) {
>     NTSTATUS Status = STATUS_SUCCESS;
>     PVOID CurrentThread = KeGetCurrentThread();
> KIRQL CurrentIrql;
>
> KIRQL CurrentIrql = KeGetCurrentIrql();
>
> ASSERT(RecMutex);
>
> ASSERT(CurrentIrql <= APC_LEVEL);
>
> /* Wait for the previous user to unlock the RecMutex state.
>
> There might be
>     * multiple waiters waiting to change the state.  We need to  
> check each
>     * time we get the event whether somebody still has the state  
> locked */
> if( !RecMutex ) return FALSE;
> ExAcquireFastMutex( &RecMutex->Mutex );
> if( CurrentThread == RecMutex->CurrentThread ||
>    (!ToWrite && !RecMutex->Writer) ) {
> if( CurrentThread == RecMutex->CurrentThread ) {
>        RecMutex->LockCount++;
>        return TRUE;
>        ExReleaseFastMutex( &RecMutex->Mutex );
>        return;
> }
> CurrentIrql = KeGetCurrentIrql();
> ASSERT(CurrentIrql <= DISPATCH_LEVEL);
> if( CurrentIrql <= APC_LEVEL ) {
>    ExAcquireFastMutex( &RecMutex->Mutex );
>    while( RecMutex->Locked ) {
>        ExReleaseFastMutex( &RecMutex->Mutex );
>        Status = KeWaitForSingleObject( &RecMutex- 
> > StateLockedEvent,
>            UserRequest,
>            KernelMode,
>            FALSE,
>            NULL );
>        ExAcquireFastMutex( &RecMutex->Mutex );
>    }
>    RecMutex->OldIrql = CurrentIrql;
>    RecMutex->Locked = TRUE;
>    RecMutex->Writer = ToWrite;
>    RecMutex->CurrentThread = CurrentThread;
>    RecMutex->LockCount++;
>    ExReleaseFastMutex( &RecMutex->Mutex );
> } else {
>    KeAcquireSpinLockAtDpcLevel( &RecMutex->SpinLock );
>    RecMutex->OldIrql = DISPATCH_LEVEL;
>    RecMutex->Locked = TRUE;
>    RecMutex->Writer = ToWrite;
>    RecMutex->CurrentThread = CurrentThread;
>    RecMutex->LockCount++;
> while( RecMutex->LockCount != 0 ) {
>     ExReleaseFastMutex( &RecMutex->Mutex );
>     Status = KeWaitForSingleObject( &RecMutex->StateLockedEvent,
>         UserRequest,
>         KernelMode,
>         FALSE,
>         NULL );
>     ExAcquireFastMutex( &RecMutex->Mutex );
> }
> return TRUE;
> RecMutex->CurrentThread = CurrentThread;
> RecMutex->LockCount++;
> ExReleaseFastMutex( &RecMutex->Mutex );
>
> }
>
> VOID RecursiveMutexLeave( PRECURSIVE_MUTEX RecMutex ) {
>
> ASSERT(RecMutex);
>
> ExAcquireFastMutex( &RecMutex->Mutex );
>
> ASSERT(RecMutex->LockCount > 0);
> RecMutex->LockCount--;
>
> if( !RecMutex->LockCount ) {
>    RecMutex->CurrentThread = NULL;
>    if( RecMutex->OldIrql <= APC_LEVEL ) {
>        ExAcquireFastMutex( &RecMutex->Mutex );
>        RecMutex->Locked = FALSE;
>        RecMutex->Writer = FALSE;
>        ExReleaseFastMutex( &RecMutex->Mutex );
>    } else {
>        RecMutex->Locked = FALSE;
>        RecMutex->Writer = FALSE;
>        KeReleaseSpinLockFromDpcLevel( &RecMutex->SpinLock );
>    }
>   KePulseEvent( &RecMutex->StateLockedEvent,  
>
> IO_NETWORK_INCREMENT,
>             FALSE );
>     }
> ExReleaseFastMutex( &RecMutex->Mutex );
>
> }
>
> Modified: trunk/reactos/drivers/network/tcpip/recmutex/recmutex.h
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/recmu...
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/reactos/drivers/network/tcpip/recmutex/recmutex.h  
> [iso-8859-1] (original)
> +++ trunk/reactos/drivers/network/tcpip/recmutex/recmutex.h  
> [iso-8859-1] Wed Aug  5 01:51:39 2009
> @@ -10,20 +10,12 @@
>     PVOID CurrentThread;
>     /* Notification event which signals that another thread can take  
> over */
>     KEVENT StateLockedEvent;
> /* IRQL from spin lock */
> KIRQL OldIrql;
> /* Is Locked */
> BOOLEAN Locked;
> /* Is reader or writer phase */
> BOOLEAN Writer;
> /* Spin lock needed for */
> KSPIN_LOCK SpinLock;
>
> } RECURSIVE_MUTEX, *PRECURSIVE_MUTEX;
>
> extern VOID RecursiveMutexInit( PRECURSIVE_MUTEX RecMutex );
> -extern SIZE_T RecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex,  
> BOOLEAN ToRead );
> +extern VOID RecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex );
> extern VOID RecursiveMutexLeave( PRECURSIVE_MUTEX RecMutex );
>
> -#define ASSERT_LOCKED(x) ASSERT((x)->Locked)
> +#define ASSERT_LOCKED(x)
>
> #endif/*_ROSRTL_RECMUTEX_H*/
>
> Modified: trunk/reactos/drivers/network/tcpip/tcpip/lock.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/tcpip...
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/reactos/drivers/network/tcpip/tcpip/lock.c [iso-8859-1]  
> (original)
> +++ trunk/reactos/drivers/network/tcpip/tcpip/lock.c [iso-8859-1]  
> Wed Aug  5 01:51:39 2009
> @@ -48,11 +48,9 @@
>     RecursiveMutexInit( RecMutex );
> }
>
> -UINT TcpipRecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex, BOOLEAN  
> ToWrite ) {
> UINT Ret;
>
> +VOID TcpipRecursiveMutexEnter( PRECURSIVE_MUTEX RecMutex, BOOLEAN  
> ToWrite ) {
>     //TI_DbgPrint(DEBUG_LOCK,("Locking\n"));
> Ret = RecursiveMutexEnter( RecMutex, ToWrite );
> return Ret;
> RecursiveMutexEnter( RecMutex );
>
> }
>
> VOID TcpipRecursiveMutexLeave( PRECURSIVE_MUTEX RecMutex ) {

Best regards,
Alex Ionescu