From 2e03cf0bb5f21702e1bdc84557dee712ca6821d5 Mon Sep 17 00:00:00 2001 From: Alex Ionescu Date: Mon, 15 Jan 2007 21:12:32 +0000 Subject: [PATCH] - 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. svn path=/trunk/; revision=25472 --- reactos/ntoskrnl/ex/rundown.c | 130 +++++++++++++------------ reactos/ntoskrnl/include/internal/ex.h | 14 +-- 2 files changed, 76 insertions(+), 68 deletions(-) diff --git a/reactos/ntoskrnl/ex/rundown.c b/reactos/ntoskrnl/ex/rundown.c index 2fc1e5d0f04..aa1ab24c612 100644 --- a/reactos/ntoskrnl/ex/rundown.c +++ b/reactos/ntoskrnl/ex/rundown.c @@ -36,21 +36,21 @@ ExfAcquireRundownProtection(IN PEX_RUNDOWN_REF RunRef) { 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; + NewValue = ExpChangeRundown(RunRef, NewValue, Value); + if (NewValue == Value) return TRUE; - /* Make sure a rundown is not active */ - if (Value & EX_RUNDOWN_ACTIVE) return FALSE; + /* Update it */ + Value = NewValue; } } @@ -79,24 +79,21 @@ ExfAcquireRundownProtectionEx(IN PEX_RUNDOWN_REF RunRef, { 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 @@ ExfReleaseRundownProtection(IN PEX_RUNDOWN_REF RunRef) 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,23 +211,28 @@ ExfReleaseRundownProtection(IN PEX_RUNDOWN_REF RunRef) NewValue = Value - EX_RUNDOWN_COUNT_INC; /* Change the value */ - Value = ExpChangeRundown(RunRef, NewValue, Value); - if (Value == NewValue) return; + NewValue = ExpChangeRundown(RunRef, NewValue, Value); + if (NewValue == Value) break; - /* Loop again if we're still not active */ - if (Value & EX_RUNDOWN_ACTIVE) 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)); - /* 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); + } - /* 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; + } } } @@ -260,37 +262,43 @@ ExfReleaseRundownProtectionEx(IN PEX_RUNDOWN_REF RunRef, 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; + NewValue = ExpChangeRundown(RunRef, NewValue, Value); + if (NewValue == Value) break; - /* Loop again if we're still not active */ - if (Value & EX_RUNDOWN_ACTIVE) break; + /* Update value */ + Value = NewValue; } - } + else + { + /* Get the wait block */ + WaitBlock = (PEX_RUNDOWN_WAIT_BLOCK)(Value & ~EX_RUNDOWN_ACTIVE); + ASSERT((WaitBlock->Count >= Count) || (KeNumberProcessors > 1)); - /* 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); + } - /* 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); + /* We're all done */ + break; + } } } @@ -330,17 +338,17 @@ ExfWaitForRundownProtectionRelease(IN PEX_RUNDOWN_REF RunRef) 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 */ diff --git a/reactos/ntoskrnl/include/internal/ex.h b/reactos/ntoskrnl/include/internal/ex.h index 97472a845a3..9f95bfc7102 100644 --- a/reactos/ntoskrnl/include/internal/ex.h +++ b/reactos/ntoskrnl/include/internal/ex.h @@ -364,7 +364,7 @@ BOOLEAN 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 @@ _ExAcquireRundownProtection(IN PEX_RUNDOWN_REF RunRef) 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 @@ VOID 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 @@ _ExReleaseRundownProtection(IN PEX_RUNDOWN_REF RunRef) 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 @@ _ExWaitForRundownProtectionRelease(IN PEX_RUNDOWN_REF RunRef) /* 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);