15 Jan
2007
15 Jan
'07
9:12 p.m.
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=…
==============================================================================
--- 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/…
==============================================================================
--- 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);