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, cgutman(a)svn.reactos.org 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/incl…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- 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/recm…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- 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/recm…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- 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/tcpi…
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  = 
>  ======================================================================
>  --- 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