[ros-diffs] [ion] 25472: - Fix several bugs in the rundown protection implementation, mostly related to incorrect loop restarting in case of a race condition. - The rundown event is a sync event, not a notification event. - Only take slow path when waiting for release if the value changed *and* is still not active, not if only one of the two is true.

Author: ion
Date: Tue Jan 16 00:12:32 2007
New Revision: 25472

URL: http://svn.reactos.org/svn/reactos?rev=25472&view=rev
Log:
- Fix several bugs in the rundown protection implementation, mostly related to incorrect loop restarting in case of a race condition.
- The rundown event is a sync event, not a notification event.
- Only take slow path when waiting for release if the value changed *and* is still not active, not if only one of the two is true.

Modified:
    trunk/reactos/ntoskrnl/ex/rundown.c
    trunk/reactos/ntoskrnl/include/internal/ex.h

Modified: trunk/reactos/ntoskrnl/ex/rundown.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/rundown.c?rev=2...
==============================================================================
--- trunk/reactos/ntoskrnl/ex/rundown.c (original)
+++ trunk/reactos/ntoskrnl/ex/rundown.c Tue Jan 16 00:12:32 2007
@@ -36,21 +36,21 @@
 {
     ULONG_PTR Value = RunRef->Count, NewValue;

-    /* Make sure a rundown is not active */
-    if (Value & EX_RUNDOWN_ACTIVE) return FALSE;
-
     /* Loop until successfully incremented the counter */
     for (;;)
     {
+        /* Make sure a rundown is not active */
+        if (Value & EX_RUNDOWN_ACTIVE) return FALSE;
+
         /* Add a reference */
         NewValue = Value + EX_RUNDOWN_COUNT_INC;

/* Change the value */
-        Value = ExpChangeRundown(RunRef, NewValue, Value);
-        if (Value == NewValue) return TRUE;
-
-        /* Make sure a rundown is not active */
-        if (Value & EX_RUNDOWN_ACTIVE) return FALSE;
+        NewValue = ExpChangeRundown(RunRef, NewValue, Value);
+        if (NewValue == Value) return TRUE;
+
+        /* Update it */
+        Value = NewValue;
     }
 }

@@ -78,25 +78,22 @@
                               IN ULONG Count)
 {
     ULONG_PTR Value = RunRef->Count, NewValue;
-
-    /* Make sure a rundown is not active */
-    if (Value & EX_RUNDOWN_ACTIVE) return FALSE;
-
-    /* Convert the count to our internal representation */
-    Count <<= EX_RUNDOWN_COUNT_SHIFT;

/* Loop until successfully incremented the counter */
     for (;;)
     {
-        /* Add references */
-        NewValue = Value + Count;
-
-        /* Change the value */
-        Value = ExpChangeRundown(RunRef, NewValue, Value);
-        if (Value == NewValue) return TRUE;
-
         /* Make sure a rundown is not active */
         if (Value & EX_RUNDOWN_ACTIVE) return FALSE;
+
+        /* Add references */
+        NewValue = Value + EX_RUNDOWN_COUNT_INC * Count;
+
+        /* Change the value */
+        NewValue = ExpChangeRundown(RunRef, NewValue, Value);
+        if (NewValue == Value) return TRUE;
+
+        /* Update the value */
+        Value = NewValue;
     }
 }

@@ -201,11 +198,11 @@
     ULONG_PTR Value = RunRef->Count, NewValue;
     PEX_RUNDOWN_WAIT_BLOCK WaitBlock;

-    /* Check if rundown is not active */
-    if (!(Value & EX_RUNDOWN_ACTIVE))
+    /* Loop until successfully incremented the counter */
+    for (;;)
     {
-        /* Loop until successfully incremented the counter */
-        for (;;)
+        /* Check if rundown is not active */
+        if (!(Value & EX_RUNDOWN_ACTIVE))
         {
             /* Sanity check */
             ASSERT((Value >= EX_RUNDOWN_COUNT_INC) || (KeNumberProcessors > 1));
@@ -214,24 +211,29 @@
             NewValue = Value - EX_RUNDOWN_COUNT_INC;

/* Change the value */
-            Value = ExpChangeRundown(RunRef, NewValue, Value);
-            if (Value == NewValue) return;
-
-            /* Loop again if we're still not active */
-            if (Value & EX_RUNDOWN_ACTIVE) break;
+            NewValue = ExpChangeRundown(RunRef, NewValue, Value);
+            if (NewValue == Value) break;
+
+            /* Update value */
+            Value = NewValue;
+        }
+        else
+        {
+            /* Get the wait block */
+            WaitBlock = (PEX_RUNDOWN_WAIT_BLOCK)(Value & ~EX_RUNDOWN_ACTIVE);
+            ASSERT((WaitBlock->Count > 0) || (KeNumberProcessors > 1));
+
+            /* Remove the one count */
+            if (!InterlockedDecrementSizeT(&WaitBlock->Count))
+            {
+                /* We're down to 0 now, so signal the event */
+                KeSetEvent(&WaitBlock->WakeEvent, IO_NO_INCREMENT, FALSE);
+            }
+
+            /* We're all done */
+            break;
         }
     }
-
-    /* Get the wait block */
-    WaitBlock = (PEX_RUNDOWN_WAIT_BLOCK)(Value & ~EX_RUNDOWN_ACTIVE);
-    ASSERT((WaitBlock->Count > 0) || (KeNumberProcessors > 1));
-
-    /* Remove the one count */
-    if (!InterlockedDecrementSizeT(&WaitBlock->Count))
-    {
-        /* We're down to 0 now, so signal the event */
-        KeSetEvent(&WaitBlock->WakeEvent, IO_NO_INCREMENT, FALSE);
-    }
 }

/*++
@@ -260,37 +262,43 @@
     ULONG_PTR Value = RunRef->Count, NewValue;
     PEX_RUNDOWN_WAIT_BLOCK WaitBlock;

-    /* Check if rundown is not active */
-    if (!(Value & EX_RUNDOWN_ACTIVE))
+    /* Loop until successfully incremented the counter */
+    for (;;)
     {
-        /* Loop until successfully incremented the counter */
-        for (;;)
+        /* Check if rundown is not active */
+        if (!(Value & EX_RUNDOWN_ACTIVE))
         {
             /* Sanity check */
-            ASSERT((Value >= EX_RUNDOWN_COUNT_INC * Count) || (KeNumberProcessors > 1));
+            ASSERT((Value >= EX_RUNDOWN_COUNT_INC * Count) ||
+                   (KeNumberProcessors > 1));

/* Get the new value */
-            NewValue = Value - (Count * EX_RUNDOWN_COUNT_INC);
+            NewValue = Value - EX_RUNDOWN_COUNT_INC * Count;

/* Change the value */
-            Value = ExpChangeRundown(RunRef, NewValue, Value);
-            if (Value == NewValue) return;
-
-            /* Loop again if we're still not active */
-            if (Value & EX_RUNDOWN_ACTIVE) break;
+            NewValue = ExpChangeRundown(RunRef, NewValue, Value);
+            if (NewValue == Value) break;
+
+            /* Update value */
+            Value = NewValue;
         }
-    }
-
-    /* Get the wait block */
-    WaitBlock = (PEX_RUNDOWN_WAIT_BLOCK)(Value & ~EX_RUNDOWN_ACTIVE);
-    ASSERT((WaitBlock->Count >= Count) || (KeNumberProcessors > 1));
-
-    /* Remove the count */
-    if (InterlockedExchangeAddSizeT(&WaitBlock->Count, -(LONG)Count) ==
-        (LONG)Count)
-    {
-        /* We're down to 0 now, so signal the event */
-        KeSetEvent(&WaitBlock->WakeEvent, IO_NO_INCREMENT, FALSE);
+        else
+        {
+            /* Get the wait block */
+            WaitBlock = (PEX_RUNDOWN_WAIT_BLOCK)(Value & ~EX_RUNDOWN_ACTIVE);
+            ASSERT((WaitBlock->Count >= Count) || (KeNumberProcessors > 1));
+
+            /* Remove the counts */
+            if (InterlockedExchangeAddSizeT(&WaitBlock->Count,
+                                            -(LONG)Count) == (LONG)Count)
+            {
+                /* We're down to 0 now, so signal the event */
+                KeSetEvent(&WaitBlock->WakeEvent, IO_NO_INCREMENT, FALSE);
+            }
+
+            /* We're all done */
+            break;
+        }
     }
 }

@@ -330,17 +338,17 @@
                                                 EX_RUNDOWN_ACTIVE);

/* Start waitblock set loop */
-    for(;;)
+    for (;;)
     {
         /* Save the count */
         Count = Value >> EX_RUNDOWN_COUNT_SHIFT;

-        /* If the count is over one or we don't have en event yet, create it */
-        if (Count || !Event)
+        /* If the count is over one and we don't have en event yet, create it */
+        if ((Count) && !(Event))
         {
             /* Initialize the event */
             KeInitializeEvent(&WaitBlock.WakeEvent,
-                              NotificationEvent,
+                              SynchronizationEvent,
                               FALSE);

/* Set the pointer */

Modified: trunk/reactos/ntoskrnl/include/internal/ex.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/e...
==============================================================================
--- trunk/reactos/ntoskrnl/include/internal/ex.h (original)
+++ trunk/reactos/ntoskrnl/include/internal/ex.h Tue Jan 16 00:12:32 2007
@@ -364,7 +364,7 @@
 FORCEINLINE
 _ExAcquireRundownProtection(IN PEX_RUNDOWN_REF RunRef)
 {
-    ULONG_PTR Value, NewValue, OldValue;
+    ULONG_PTR Value, NewValue;

/* Get the current value and mask the active bit */
     Value = RunRef->Count &~ EX_RUNDOWN_ACTIVE;
@@ -373,8 +373,8 @@
     NewValue = Value + EX_RUNDOWN_COUNT_INC;

/* Change the value */
-    OldValue = ExpChangeRundown(RunRef, NewValue, Value);
-    if (OldValue != Value)
+    NewValue = ExpChangeRundown(RunRef, NewValue, Value);
+    if (NewValue != Value)
     {
         /* Rundown was active, use long path */
         return ExfAcquireRundownProtection(RunRef);
@@ -405,7 +405,7 @@
 FORCEINLINE
 _ExReleaseRundownProtection(IN PEX_RUNDOWN_REF RunRef)
 {
-    ULONG_PTR Value, NewValue, OldValue;
+    ULONG_PTR Value, NewValue;

/* Get the current value and mask the active bit */
     Value = RunRef->Count &~ EX_RUNDOWN_ACTIVE;
@@ -414,10 +414,10 @@
     NewValue = Value - EX_RUNDOWN_COUNT_INC;

/* Change the value */
-    OldValue = ExpChangeRundown(RunRef, NewValue, Value);
+    NewValue = ExpChangeRundown(RunRef, NewValue, Value);

/* Check if the rundown was active */
-    if (OldValue != Value)
+    if (NewValue != Value)
     {
         /* Rundown was active, use long path */
         ExfReleaseRundownProtection(RunRef);
@@ -476,7 +476,7 @@

/* Set the active bit */
     Value = ExpChangeRundown(RunRef, EX_RUNDOWN_ACTIVE, 0);
-    if ((Value) || (Value != EX_RUNDOWN_ACTIVE))
+    if ((Value) && (Value != EX_RUNDOWN_ACTIVE))
     {
         /* If the the rundown wasn't already active, then take the long path */
         ExfWaitForRundownProtectionRelease(RunRef);