[ros-dev] Re: [ros-diffs] [ion] 12774: Add more Critical Section Debug Data and fix two bugs.

[Joseph Galbraith]

ion(a)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");
      }