Avoid long busy-waiting between hijack retries. (#103212)

* refactor suspension sleep into minipal_microsleep, avoid long busy-waiting * rename * Apply suggestions from code review Co-authored-by: Jan Kotas <[email protected]> --------- Co-authored-by: Jan Kotas <[email protected]>
dotnet · Jun 28, 2024 · 9501cce · 9501cce
1 parent a900bbf
commit 9501cce
Show file tree

Hide file tree

Showing 11 changed files with 247 additions and 95 deletions.
diff --git a/src/coreclr/minipal/Unix/CMakeLists.txt b/src/coreclr/minipal/Unix/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(SOURCES
     doublemapping.cpp
     dn-u16.cpp
+    ${CLR_SRC_NATIVE_DIR}/minipal/time.c
 )
 
 if(NOT CLR_CROSS_COMPONENTS_BUILD)

diff --git a/src/coreclr/minipal/Windows/CMakeLists.txt b/src/coreclr/minipal/Windows/CMakeLists.txt
@@ -2,6 +2,7 @@ set(SOURCES
     doublemapping.cpp
     dn-u16.cpp
     ${CLR_SRC_NATIVE_DIR}/minipal/utf8.c
+    ${CLR_SRC_NATIVE_DIR}/minipal/time.c
 )
 
 if(NOT CLR_CROSS_COMPONENTS_BUILD)

diff --git a/src/coreclr/nativeaot/Runtime/CMakeLists.txt b/src/coreclr/nativeaot/Runtime/CMakeLists.txt
@@ -51,6 +51,7 @@ set(COMMON_RUNTIME_SOURCES
     ${GC_DIR}/softwarewritewatch.cpp
 
     ${CLR_SRC_NATIVE_DIR}/minipal/cpufeatures.c
+    ${CLR_SRC_NATIVE_DIR}/minipal/time.c
 )
 
 set(SERVER_GC_SOURCES

diff --git a/src/coreclr/nativeaot/Runtime/threadstore.cpp b/src/coreclr/nativeaot/Runtime/threadstore.cpp
@@ -22,6 +22,7 @@
 #include "RuntimeInstance.h"
 #include "TargetPtrs.h"
 #include "yieldprocessornormalized.h"
+#include <minipal/time.h>
 
 #include "slist.inl"
 
@@ -224,30 +225,6 @@ void ThreadStore::UnlockThreadStore()
     m_Lock.Leave();
 }
 
-// exponential spinwait with an approximate time limit for waiting in microsecond range.
-// when iteration == -1, only usecLimit is used
-void SpinWait(int iteration, int usecLimit)
-{
-    int64_t startTicks = PalQueryPerformanceCounter();
-    int64_t ticksPerSecond = PalQueryPerformanceFrequency();
-    int64_t endTicks = startTicks + (usecLimit * ticksPerSecond) / 1000000;
-
-    int l = iteration >= 0 ? min(iteration, 30): 30;
-    for (int i = 0; i < l; i++)
-    {
-        for (int j = 0; j < (1 << i); j++)
-        {
-            System_YieldProcessor();
-        }
-
-        int64_t currentTicks = PalQueryPerformanceCounter();
-        if (currentTicks > endTicks)
-        {
-            break;
-        }
-    }
-}
-
 void ThreadStore::SuspendAllThreads(bool waitForGCEvent)
 {
     Thread * pThisThread = GetCurrentThreadIfAvailable();
@@ -265,16 +242,14 @@ void ThreadStore::SuspendAllThreads(bool waitForGCEvent)
     // reason for this is that we essentially implement Dekker's algorithm, which requires write ordering.
     PalFlushProcessWriteBuffers();
 
-    int retries = 0;
-    int prevRemaining = 0;
-    int remaining = 0;
-    bool observeOnly = false;
+    int prevRemaining = INT32_MAX;
+    bool observeOnly = true;
+    uint32_t rehijackDelay = 8;
+    uint32_t usecsSinceYield = 0;
 
     while(true)
     {
-        prevRemaining = remaining;
-        remaining = 0;
-
+        int remaining = 0;
         FOREACH_THREAD(pTargetThread)
         {
             if (pTargetThread == pThisThread)
@@ -293,30 +268,42 @@ void ThreadStore::SuspendAllThreads(bool waitForGCEvent)
         }
         END_FOREACH_THREAD
 
-        if (!remaining)
+        if (remaining == 0)
             break;
 
         // if we see progress or have just done a hijacking pass
         // do not hijack in the next iteration
         if (remaining < prevRemaining || !observeOnly)
         {
             // 5 usec delay, then check for more progress
-            SpinWait(-1, 5);
+            minipal_microdelay(5, &usecsSinceYield);
             observeOnly = true;
         }
         else
         {
-            SpinWait(retries++, 100);
+            minipal_microdelay(rehijackDelay, &usecsSinceYield);
             observeOnly = false;
 
-            // make sure our spining is not starving other threads, but not too often,
-            // this can cause a 1-15 msec delay, depending on OS, and that is a lot while
-            // very rarely needed, since threads are supposed to be releasing their CPUs
-            if ((retries & 127) == 0)
+            // double up rehijack delay in case we are rehjacking too often
+            // up to 100 usec, as that should be enough to make progress.
+            if (rehijackDelay < 100)
             {
-                PalSwitchToThread();
+                rehijackDelay *= 2;
             }
         }
+
+        prevRemaining = remaining;
+
+        // If we see 1 msec of uninterrupted wait, it is a concern.
+        // Since we are stopping threads, there should be free cores to run on. Perhaps
+        // some thread that we need to stop needs to run on the same core as ours.
+        // Let's yield the timeslice to make sure such threads can run.
+        // We will not do this often though, since this can introduce arbitrary delays.
+        if (usecsSinceYield > 1000)
+        {
+            PalSwitchToThread();
+            usecsSinceYield = 0;
+        }
     }
 
 #if defined(TARGET_ARM) || defined(TARGET_ARM64)

diff --git a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkUnix.cpp b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkUnix.cpp
@@ -1082,7 +1082,6 @@ REDHAWK_PALEXPORT void REDHAWK_PALAPI PalHijack(HANDLE hThread, _In_opt_ void* p
     // stack overflow too. Those are held in the sigsegv_handler with blocked signals until
     // the process exits.
     // ESRCH may happen on some OSes when the thread is exiting.
-    // The thread should leave cooperative mode, but we could have seen it in its earlier state.
     if ((status == EAGAIN)
      || (status == ESRCH)
 #ifdef __APPLE__

diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp
@@ -935,7 +935,13 @@ PAL_ERROR InjectActivationInternal(CorUnix::CPalThread* pThread)
     }
 #endif
 
-    if ((status != 0) && (status != EAGAIN))
+    // ESRCH may happen on some OSes when the thread is exiting.
+    if (status == EAGAIN || status == ESRCH)
+    {
+        return ERROR_CANCELLED;
+    }
+
+    if (status != 0)
     {
         // Failure to send the signal is fatal. There are only two cases when sending
         // the signal can fail. First, if the signal ID is invalid and second,

diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp
@@ -14,6 +14,7 @@
 
 #include "finalizerthread.h"
 #include "dbginterface.h"
+#include <minipal/time.h>
 
 #define HIJACK_NONINTERRUPTIBLE_THREADS
 
@@ -2188,6 +2189,8 @@ void Thread::RareDisablePreemptiveGC()
 #if defined(FEATURE_HIJACK) && !defined(TARGET_UNIX)
             ResetThreadState(Thread::TS_GCSuspendRedirected);
 #endif
+            // make sure this is cleared - in case a signal is lost or somehow we did not act on it
+            m_hasPendingActivation = false;
 
             DWORD status = GCHeapUtilities::GetGCHeap()->WaitUntilGCComplete();
             if (status != S_OK)
@@ -3207,44 +3210,6 @@ COR_PRF_SUSPEND_REASON GCSuspendReasonToProfSuspendReason(ThreadSuspend::SUSPEND
 }
 #endif // PROFILING_SUPPORTED
 
-static int64_t QueryPerformanceCounter()
-{
-    LARGE_INTEGER ts;
-    QueryPerformanceCounter(&ts);
-    return ts.QuadPart;
-}
-
-static int64_t QueryPerformanceFrequency()
-{
-    LARGE_INTEGER ts;
-    QueryPerformanceFrequency(&ts);
-    return ts.QuadPart;
-}
-
-// exponential spinwait with an approximate time limit for waiting in microsecond range.
-// when iteration == -1, only usecLimit is used
-void SpinWait(int iteration, int usecLimit)
-{
-    int64_t startTicks = QueryPerformanceCounter();
-    int64_t ticksPerSecond = QueryPerformanceFrequency();
-    int64_t endTicks = startTicks + (usecLimit * ticksPerSecond) / 1000000;
-
-    int l = iteration >= 0 ? min(iteration, 30): 30;
-    for (int i = 0; i < l; i++)
-    {
-        for (int j = 0; j < (1 << i); j++)
-        {
-            System_YieldProcessor();
-        }
-
-        int64_t currentTicks = QueryPerformanceCounter();
-        if (currentTicks > endTicks)
-        {
-            break;
-        }
-    }
-}
-
 //************************************************************************************
 //
 // SuspendRuntime is responsible for ensuring that all managed threads reach a
@@ -3335,16 +3300,14 @@ void ThreadSuspend::SuspendAllThreads()
     // See VSW 475315 and 488918 for details.
     ::FlushProcessWriteBuffers();
 
-    int retries = 0;
-    int prevRemaining = 0;
-    int remaining = 0;
-    bool observeOnly = false;
+    int prevRemaining = INT32_MAX;
+    bool observeOnly = true;
+    uint32_t rehijackDelay = 8;
+    uint32_t usecsSinceYield = 0;
 
     while(true)
     {
-        prevRemaining = remaining;
-        remaining = 0;
-
+        int remaining = 0;
         Thread* pTargetThread = NULL;
         while ((pTargetThread = ThreadStore::GetThreadList(pTargetThread)) != NULL)
         {
@@ -3361,30 +3324,42 @@ void ThreadSuspend::SuspendAllThreads()
             }
         }
 
-        if (!remaining)
+        if (remaining == 0)
             break;
 
         // if we see progress or have just done a hijacking pass
         // do not hijack in the next iteration
         if (remaining < prevRemaining || !observeOnly)
         {
             // 5 usec delay, then check for more progress
-            SpinWait(-1, 5);
+            minipal_microdelay(5, &usecsSinceYield);
             observeOnly = true;
         }
         else
         {
-            SpinWait(retries++, 100);
+            minipal_microdelay(rehijackDelay, &usecsSinceYield);
             observeOnly = false;
 
-            // make sure our spining is not starving other threads, but not too often,
-            // this can cause a 1-15 msec delay, depending on OS, and that is a lot while
-            // very rarely needed, since threads are supposed to be releasing their CPUs
-            if ((retries & 127) == 0)
+            // double up rehijack delay in case we are rehjacking too often
+            // up to 100 usec, as that should be enough to make progress.
+            if (rehijackDelay < 100)
             {
-                SwitchToThread();
+                rehijackDelay *= 2;
             }
         }
+
+        prevRemaining = remaining;
+
+        // If we see 1 msec of uninterrupted wait, it is a concern.
+        // Since we are stopping threads, there should be free cores to run on. Perhaps
+        // some thread that we need to stop needs to run on the same core as ours.
+        // Let's yield the timeslice to make sure such threads can run.
+        // We will not do this often though, since this can introduce arbitrary delays.
+        if (usecsSinceYield > 1000)
+        {
+            SwitchToThread();
+            usecsSinceYield = 0;
+        }
     }
 
 #if defined(TARGET_ARM) || defined(TARGET_ARM64)
@@ -5937,7 +5912,13 @@ bool Thread::InjectActivation(ActivationReason reason)
         if (hThread != INVALID_HANDLE_VALUE)
         {
             m_hasPendingActivation = true;
-            return ::PAL_InjectActivation(hThread);
+            BOOL success = ::PAL_InjectActivation(hThread);
+            if (!success)
+            {
+                m_hasPendingActivation = false;
+            }
+
+            return success;
         }
     }
 

diff --git a/src/native/minipal/configure.cmake b/src/native/minipal/configure.cmake
@@ -8,4 +8,9 @@ check_function_exists(sysctlbyname HAVE_SYSCTLBYNAME)
 check_symbol_exists(arc4random_buf "stdlib.h" HAVE_ARC4RANDOM_BUF)
 check_symbol_exists(O_CLOEXEC fcntl.h HAVE_O_CLOEXEC)
 
+check_symbol_exists(
+    clock_gettime_nsec_np
+    time.h
+    HAVE_CLOCK_GETTIME_NSEC_NP)
+
 configure_file(${CMAKE_CURRENT_LIST_DIR}/minipalconfig.h.in ${CMAKE_CURRENT_BINARY_DIR}/minipalconfig.h)
diff --git a/src/native/minipal/minipalconfig.h.in b/src/native/minipal/minipalconfig.h.in
@@ -5,5 +5,6 @@
 #cmakedefine01 HAVE_AUXV_HWCAP_H
 #cmakedefine01 HAVE_O_CLOEXEC
 #cmakedefine01 HAVE_SYSCTLBYNAME
+#cmakedefine01 HAVE_CLOCK_GETTIME_NSEC_NP
 
 #endif