Some refactoring of thread suspension. (#34666)

* m_dwForbidSuspendThread

* Remove some fibers stuff

* g_TrapReturningThreads

* g_pGCSuspendEvent

* m_pThreadAttemptingSuspendForGC

* refactor SuspendRuntime into just one loop

* Suspend iteration after updating hijacks should "observeOnly" - no point to update hijacks again right away.

* avoid retrying to suspend threads that are already in redirected state.

* PR feedback

* "premptive" - common typo apparently

* missing profiler notification on suspend end and `DisableStressHeap` stuff

* indentation and infinite loop style

* Removed GTF_DONT_CSE flag on `g_TrapReturningThreads` load in GCPoll and added comment explaining why.

* reverted an accidentally removed assert

* removed SWITCHOUT_HANDLE_VALUE entirely - not different from INVALID_HANDLE_VALUE now

* some cleanups and more comments when non-GC suspension yields to GC

* Few tweaks for the concerns discussed in the PR

* On Server GC `pCurThread` is NULL.
Need to fix an ASSERT to account for the possibility of NULL.

src/coreclr/src/debug/di/rsthread.cpp

@@ -603,7 +603,7 @@ bool CordbThread::OwnsFrame(CordbFrame * pFrame)
 // This routine is a internal helper function for ICorDebugThread2::GetTaskId.
 //
 // Arguments:
-//    pHandle - return thread handle here after fetching from the left side. Can return SWITCHOUT_HANDLE_VALUE.
+//    pHandle - return thread handle here after fetching from the left side.
 //
 // Return Value:
 //    hr - It can fail with CORDBG_E_THREAD_NOT_SCHEDULED.
@@ -629,12 +629,6 @@ void CordbThread::RefreshHandle(HANDLE * phThread)
     IDacDbiInterface * pDAC = GetProcess()->GetDAC();
     HANDLE hThread = pDAC->GetThreadHandle(m_vmThreadToken);
 
-    if (hThread == SWITCHOUT_HANDLE_VALUE)
-    {
-        *phThread = SWITCHOUT_HANDLE_VALUE;
-        ThrowHR(CORDBG_E_THREAD_NOT_SCHEDULED);
-    }
-
     _ASSERTE(hThread != INVALID_HANDLE_VALUE);
     PREFAST_ASSUME(hThread != NULL);
 

src/coreclr/src/inc/cor.h

@@ -143,8 +143,6 @@ typedef UNALIGNED void const *UVCP_CONSTANT;
 #define TARGET_MAIN_CLR_DLL_NAME_W    MAKE_TARGET_DLLNAME_W(MAIN_CLR_MODULE_NAME_W)
 #define TARGET_MAIN_CLR_DLL_NAME_A    MAKE_TARGET_DLLNAME_A(MAIN_CLR_MODULE_NAME_A)
 
-#define SWITCHOUT_HANDLE_VALUE ((HANDLE)(LONG_PTR)-2)
-
 //*****************************************************************************
 //*****************************************************************************
 //

src/coreclr/src/jit/flowgraph.cpp

@@ -4016,8 +4016,11 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
             value = gtNewIndOfIconHandleNode(TYP_INT, (size_t)addrTrap, GTF_ICON_GLOBAL_PTR, false);
         }
 
-        // Treat the reading of g_TrapReturningThreads as volatile.
-        value->gtFlags |= GTF_IND_VOLATILE;
+        // NOTE: in c++ an equivalent load is done via LoadWithoutBarrier() to ensure that the
+        // program order is preserved. (not hoisted out of a loop or cached in a local, for example)
+        //
+        // Here we introduce the read really late after all major optimizations are done, and the location
+        // is formally unknown, so noone could optimize the load, thus no special flags are needed.
 
         // Compare for equal to zero
         GenTree* trapRelop = gtNewOperNode(GT_EQ, TYP_INT, value, gtNewIconNode(0, TYP_INT));

src/coreclr/src/vm/gccover.cpp

@@ -1266,7 +1266,7 @@ void RemoveGcCoverageInterrupt(TADDR instrPtr, BYTE * savedInstrPtr, GCCoverageI
 
 // A managed thread (T) can race with the GC as follows:
 // 1)	At the first safepoint, we notice that T is in preemptive mode during the call for GCStress
-//      So, it is put it in cooperative mode for the purpose of GCStress(fPremptiveGcDisabledForGcStress)
+//      So, it is put it in cooperative mode for the purpose of GCStress(fPreemptiveGcDisabledForGcStress)
 // 2)	We DoGCStress(). Start off background GC in a different thread.
 // 3)	Then the thread T is put back to preemptive mode (because that's where it was).
 //      Thread T continues execution along with the GC thread.

src/coreclr/src/vm/proftoeeinterfaceimpl.cpp

@@ -9159,7 +9159,7 @@ HRESULT ProfToEEInterfaceImpl::RequestReJIT(ULONG       cFunctions,   // in
         // Yay!
         NOTHROW;
 
-        // When we suspend the runtime we drop into premptive mode
+        // When we suspend the runtime we drop into preemptive mode
         GC_TRIGGERS;
 
         // Yay!

src/coreclr/src/vm/threads.cpp

@@ -388,7 +388,7 @@ BOOL Thread::Alert ()
     BOOL fRetVal = FALSE;
     {
         HANDLE handle = GetThreadHandle();
-        if (handle != INVALID_HANDLE_VALUE && handle != SWITCHOUT_HANDLE_VALUE)
+        if (handle != INVALID_HANDLE_VALUE)
         {
             fRetVal = ::QueueUserAPC(UserInterruptAPC, handle, APC_Code);
         }
@@ -422,7 +422,7 @@ DWORD Thread::JoinEx(DWORD timeout, WaitMode mode)
         mode = (WaitMode)(mode & ~WaitMode_InDeadlock);
 
         HANDLE handle = GetThreadHandle();
-        if (handle == INVALID_HANDLE_VALUE || handle == SWITCHOUT_HANDLE_VALUE) {
+        if (handle == INVALID_HANDLE_VALUE) {
             return WAIT_FAILED;
         }
         if (pCurThread) {
@@ -572,8 +572,7 @@ DWORD Thread::StartThread()
     m_Creater.Clear();
 #endif
 
-    _ASSERTE (GetThreadHandle() != INVALID_HANDLE_VALUE &&
-                GetThreadHandle() != SWITCHOUT_HANDLE_VALUE);
+    _ASSERTE (GetThreadHandle() != INVALID_HANDLE_VALUE);
     dwRetVal = ::ResumeThread(GetThreadHandle());
 
 
@@ -1000,7 +999,7 @@ HRESULT Thread::DetachThread(BOOL fDLLThreadDetach)
     }
 
     HANDLE hThread = GetThreadHandle();
-    SetThreadHandle (SWITCHOUT_HANDLE_VALUE);
+    SetThreadHandle (INVALID_HANDLE_VALUE);
     while (m_dwThreadHandleBeingUsed > 0)
     {
         // Another thread is using the handle now.
@@ -5182,15 +5181,29 @@ void ThreadStore::InitThreadStore()
 // additional semantics well beyond a normal lock.
 DEBUG_NOINLINE void ThreadStore::Enter()
 {
-    WRAPPER_NO_CONTRACT;
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
+        // we must be in preemptive mode while taking this lock
+        // if suspension is in progress, the lock is taken, and there is no way to suspend us once we block
+        MODE_PREEMPTIVE;
+    }
+    CONTRACTL_END;
+
     ANNOTATION_SPECIAL_HOLDER_CALLER_NEEDS_DYNAMIC_CONTRACT;
     CHECK_ONE_STORE();
     m_Crst.Enter();
 }
 
 DEBUG_NOINLINE void ThreadStore::Leave()
 {
-    WRAPPER_NO_CONTRACT;
+    CONTRACTL {
+        NOTHROW;
+        GC_NOTRIGGER;
+    }
+    CONTRACTL_END;
+
     ANNOTATION_SPECIAL_HOLDER_CALLER_NEEDS_DYNAMIC_CONTRACT;
     CHECK_ONE_STORE();
     m_Crst.Leave();

src/coreclr/src/vm/threads.h

@@ -1097,7 +1097,11 @@ class Thread
         TS_Unknown                = 0x00000000,    // threads are initialized this way
 
         TS_AbortRequested         = 0x00000001,    // Abort the thread
-        TS_GCSuspendPending       = 0x00000002,    // waiting to get to safe spot for GC
+
+        TS_GCSuspendPending       = 0x00000002,    // ThreadSuspend::SuspendRuntime watches this thread to leave coop mode.
+        TS_GCSuspendRedirected    = 0x00000004,    // ThreadSuspend::SuspendRuntime has redirected the thread to suspention routine.
+        TS_GCSuspendFlags         = TS_GCSuspendPending | TS_GCSuspendRedirected, // used to track suspension progress. Only SuspendRuntime writes/resets these.
+
         TS_DebugSuspendPending    = 0x00000008,    // Is the debugger suspending threads?
         TS_GCOnTransitions        = 0x00000010,    // Force a GC on stub transitions (GCStress only)
 
@@ -1656,9 +1660,12 @@ class Thread
     EEThreadId m_Creater;
 #endif
 
-    // After we suspend a thread, we may need to call EEJitManager::JitCodeToMethodInfo
-    // or StressLog which may waits on a spinlock.  It is unsafe to suspend a thread while it
-    // is in this state.
+    // A thread may forbid its own suspension. For example when holding certain locks.
+    // The state is a counter to allow nested forbids.
+    // The state is only modified by the current thread.
+    // Other threads may read this state, but must mind the races.
+    // It must be assumed that a running thread (or one that may start running) may change this state at any time.
+    // One exception: SuspendThread can read this "reliably" from other threads by temporarily suspending them, reading, and releasing if != 0.
     Volatile<LONG> m_dwForbidSuspendThread;
 
 public:
@@ -1684,7 +1691,8 @@ class Thread
             STRESS_LOG2(LF_SYNC, LL_INFO100000, "Set forbid suspend [%d] for thread %p.\n", pThread->m_dwForbidSuspendThread.Load(), pThread);
             }
 #endif
-            FastInterlockIncrement(&pThread->m_dwForbidSuspendThread);
+            // modified only by the current thread, so ++ is ok
+            pThread->m_dwForbidSuspendThread.RawValue()++;
         }
 #endif //!DACCESS_COMPILE
     }
@@ -1703,8 +1711,10 @@ class Thread
         Thread * pThread = GetThreadNULLOk();
         if (pThread)
         {
-            _ASSERTE (pThread->m_dwForbidSuspendThread != (LONG)0);
-            FastInterlockDecrement(&pThread->m_dwForbidSuspendThread);
+            _ASSERTE (pThread->m_dwForbidSuspendThread >= (LONG)0);
+
+            // modified only by the current thread, so -- is ok
+            pThread->m_dwForbidSuspendThread.RawValue()--;
 #ifdef _DEBUG
             {
                 //DEBUG_ONLY;
@@ -1715,9 +1725,11 @@ class Thread
 #endif //!DACCESS_COMPILE
     }
 
+    // Returns the state of m_dwForbidSuspendThread as it was at the time of the call.
+    // It may asynchronously change if there are no additional guarantees (i.e. it is the current thread or the thread is suspended)
     bool IsInForbidSuspendRegion()
     {
-        return m_dwForbidSuspendThread != (LONG)0;
+        return m_dwForbidSuspendThread.LoadWithoutBarrier() != (LONG)0;
     }
 
     typedef StateHolder<Thread::IncForbidSuspendThread, Thread::DecForbidSuspendThread> ForbidSuspendThreadHolder;
@@ -2397,10 +2409,6 @@ class Thread
         // that either we are not allowed to call into the host, or we ran
         // out of memory.
         STR_NoStressLog,
-
-        // The EE thread is currently switched out.  This can only happen
-        // if we are hosted and the host schedules EE threads on fibers.
-        STR_SwitchedOut,
     };
 
 #if defined(FEATURE_HIJACK) && defined(TARGET_UNIX)
@@ -3506,7 +3514,6 @@ class Thread
             CounterHolder handleHolder(&m_dwThreadHandleBeingUsed);
             HANDLE handle = m_ThreadHandle;
             _ASSERTE ( handle == INVALID_HANDLE_VALUE
-                || handle == SWITCHOUT_HANDLE_VALUE
                 || m_OSThreadId == 0
                 || m_OSThreadId == 0xbaadf00d
                 || ::MatchThreadHandleToOsId(handle, (DWORD)m_OSThreadId) );
@@ -3522,7 +3529,6 @@ class Thread
         LIMITED_METHOD_CONTRACT;
 #if defined(_DEBUG)
         _ASSERTE ( h == INVALID_HANDLE_VALUE
-            || h == SWITCHOUT_HANDLE_VALUE
             || m_OSThreadId == 0
             || m_OSThreadId == 0xbaadf00d
             || ::MatchThreadHandleToOsId(h, (DWORD)m_OSThreadId) );
@@ -3934,7 +3940,7 @@ class Thread
             UnhijackThread();
 #endif // FEATURE_HIJACK
 
-        ResetThreadState(TS_GCSuspendPending);
+        _ASSERTE(!HasThreadStateOpportunistic(Thread::TS_GCSuspendPending));
     }
 
     static LPVOID GetStaticFieldAddress(FieldDesc *pFD);