ion@svn.reactos.com wrote:
- DPRINT("Waiting on Critical Section: %x\n", CriticalSection);
- if (CriticalSection->DebugInfo) CriticalSection->DebugInfo->EntryCount++;
I'm just reading the diffs here, so I could be way off, but shouldn't this use an InterlockedIncrement... otherwise you could get a context switch between in the middle of your increment to another threading attempting to lock the critical section. It is just debug information, but still...
Also, looking at the rest of the code, I notice that spin count doesn't appear to be implemented.
I don't have a build environment even setup, so take the attached patch with a grain of salt.
The changes in RtlpCreateCriticalSEctionSem() are because the InterlockCompareExchangePointer() should already have written the new event into the data structure.
Maybe on day I'll manage to get my build environment setup again.
- Joseph
Index: C:/Users/galb/ReactOS/reactos/lib/ntdll/rtl/critical.c =================================================================== --- C:/Users/galb/ReactOS/reactos/lib/ntdll/rtl/critical.c (revision 12774) +++ C:/Users/galb/ReactOS/reactos/lib/ntdll/rtl/critical.c (working copy) @@ -133,7 +133,17 @@ { HANDLE Thread = (HANDLE)NtCurrentTeb()->Cid.UniqueThread;
- /* Try to Lock it */ + /* Try for SpinCount times to Lock it */ + for(LONG i = 0; i < CriticalSection->SpinCount; i++) + { + if ( RtlTryEnterCriticalSection(CriticalSection) ) + return STATUS_SUCCESS; + } + + /* + * Still didn't get it; we have to bump the LockCount, which + * might get it for us, and then wait + */ if (InterlockedIncrement(&CriticalSection->LockCount)) {
/* @@ -398,12 +408,12 @@
/* Increase the Debug Entry count */ DPRINT("Waiting on Critical Section: %x\n", CriticalSection); - if (CriticalSection->DebugInfo) CriticalSection->DebugInfo->EntryCount++; + if (CriticalSection->DebugInfo) InterlockedIncrement(&CriticalSection->DebugInfo->EntryCount);
for (;;) {
/* Increase the number of times we've had contention */ - if (CriticalSection->DebugInfo) CriticalSection->DebugInfo->ContentionCount++; + if (CriticalSection->DebugInfo) InterlockedIncrement(&CriticalSection->DebugInfo->ContentionCount);
/* Wait on the Event */ DPRINT("Waiting on Event: %x\n", CriticalSection->LockSemaphore); @@ -523,21 +533,14 @@ return; }
- if (!(hEvent = InterlockedCompareExchangePointer((PVOID*)&CriticalSection->LockSemaphore, - (PVOID)hNewEvent, - 0))) { - - /* We created a new event succesffuly */ - hEvent = hNewEvent; - } else { - - /* Some just created an event */ - DPRINT("Closing already created event!\n"); - NtClose(hNewEvent); + + if ( InterlockedCompareExchangePointer((PVOID*)&CriticalSection->LockSemaphore, + (PVOID)hNewEvent, + 0)) != 0) { + /* Someone else just created the event */ + NtClose(hNewEvent); } - - /* Set either the new or the old */ - CriticalSection->LockSemaphore = hEvent; + DPRINT("Event set!\n"); }