From bd3e6f8e9c95b572d25a3261e6b7dae889e8fa73 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 9 Jun 2022 04:53:58 -0700 Subject: [PATCH 1/2] Modify thread pool thread counting to be a bit more defensive - An unexpected underflow in one or more thread counts can lead to a large number of threads to be created continually - Prevented underflows in changes to thread counts, such that following an unexpected underflow, subsequent paired increments and decrements would avoid repeating the underflow - Verified by creating an unexpected underflow in the debugger --- .../PortableThreadPool.ThreadCounts.cs | 51 ++++++++----------- .../PortableThreadPool.WorkerThread.cs | 28 +++++++--- 2 files changed, 41 insertions(+), 38 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs index 3242f165aff9a..26b6ab0cf0ac4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs @@ -31,61 +31,52 @@ private void SetInt16Value(short value, byte shift) => /// public short NumProcessingWork { - get => GetInt16Value(NumProcessingWorkShift); + get + { + short value = GetInt16Value(NumProcessingWorkShift); + Debug.Assert(value >= 0); + return value; + } set { Debug.Assert(value >= 0); - SetInt16Value(value, NumProcessingWorkShift); + SetInt16Value(Math.Max((short)0, value), NumProcessingWorkShift); } } - public void SubtractNumProcessingWork(short value) - { - Debug.Assert(value >= 0); - Debug.Assert(value <= NumProcessingWork); - - _data -= (ulong)(ushort)value << NumProcessingWorkShift; - } - - public void InterlockedDecrementNumProcessingWork() - { - Debug.Assert(NumProcessingWorkShift == 0); - - ThreadCounts counts = new ThreadCounts(Interlocked.Decrement(ref _data)); - Debug.Assert(counts.NumProcessingWork >= 0); - } - /// /// Number of thread pool threads that currently exist. /// public short NumExistingThreads { - get => GetInt16Value(NumExistingThreadsShift); + get + { + short value = GetInt16Value(NumExistingThreadsShift); + Debug.Assert(value >= 0); + return value; + } set { Debug.Assert(value >= 0); - SetInt16Value(value, NumExistingThreadsShift); + SetInt16Value(Math.Max((short)0, value), NumExistingThreadsShift); } } - public void SubtractNumExistingThreads(short value) - { - Debug.Assert(value >= 0); - Debug.Assert(value <= NumExistingThreads); - - _data -= (ulong)(ushort)value << NumExistingThreadsShift; - } - /// /// Max possible thread pool threads we want to have. /// public short NumThreadsGoal { - get => GetInt16Value(NumThreadsGoalShift); + get + { + short value = GetInt16Value(NumThreadsGoalShift); + Debug.Assert(value > 0); + return value; + } set { Debug.Assert(value > 0); - SetInt16Value(value, NumThreadsGoalShift); + SetInt16Value(Math.Max((short)1, value), NumThreadsGoalShift); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs index 7a79c9980447c..9042917a53b9a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs @@ -128,16 +128,14 @@ private static void WorkerThreadStart() // Since this thread is currently registered as an existing thread, if more work comes in meanwhile, // this thread would be expected to satisfy the new work. Ensure that NumExistingThreads is not // decreased below NumProcessingWork, as that would be indicative of such a case. - short numExistingThreads = counts.NumExistingThreads; - if (numExistingThreads <= counts.NumProcessingWork) + if (counts.NumExistingThreads <= counts.NumProcessingWork) { // In this case, enough work came in that this thread should not time out and should go back to work. break; } ThreadCounts newCounts = counts; - newCounts.SubtractNumExistingThreads(1); - short newNumExistingThreads = (short)(numExistingThreads - 1); + short newNumExistingThreads = --newCounts.NumExistingThreads; short newNumThreadsGoal = Math.Max( threadPoolInstance.MinThreadsGoal, @@ -173,7 +171,21 @@ private static void WorkerThreadStart() /// private static void RemoveWorkingWorker(PortableThreadPool threadPoolInstance) { - threadPoolInstance._separated.counts.InterlockedDecrementNumProcessingWork(); + ThreadCounts counts = threadPoolInstance._separated.counts; + while (true) + { + ThreadCounts newCounts = counts; + newCounts.NumProcessingWork--; + + ThreadCounts countsBeforeUpdate = + threadPoolInstance._separated.counts.InterlockedCompareExchange(newCounts, counts); + if (countsBeforeUpdate == counts) + { + break; + } + + counts = countsBeforeUpdate; + } // It's possible that we decided we had thread requests just before a request came in, // but reduced the worker count *after* the request came in. In this case, we might @@ -235,8 +247,8 @@ internal static void MaybeAddWorkingWorker(PortableThreadPool threadPoolInstance while (true) { ThreadCounts newCounts = counts; - newCounts.SubtractNumProcessingWork((short)toCreate); - newCounts.SubtractNumExistingThreads((short)toCreate); + newCounts.NumProcessingWork -= (short)toCreate; + newCounts.NumExistingThreads -= (short)toCreate; ThreadCounts oldCounts = threadPoolInstance._separated.counts.InterlockedCompareExchange(newCounts, counts); if (oldCounts == counts) @@ -273,7 +285,7 @@ internal static bool ShouldStopProcessingWorkNow(PortableThreadPool threadPoolIn } ThreadCounts newCounts = counts; - newCounts.SubtractNumProcessingWork(1); + newCounts.NumProcessingWork--; ThreadCounts oldCounts = threadPoolInstance._separated.counts.InterlockedCompareExchange(newCounts, counts); From d7f8cfc4047a1736aeb15d4b6402e5a4fb7fda40 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 9 Jun 2022 09:36:27 -0700 Subject: [PATCH 2/2] Address feedback --- .../src/System/Threading/PortableThreadPool.WorkerThread.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs index 9042917a53b9a..3d2dee0e495fa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs @@ -171,6 +171,8 @@ private static void WorkerThreadStart() /// private static void RemoveWorkingWorker(PortableThreadPool threadPoolInstance) { + // A compare-exchange loop is used instead of Interlocked.Decrement or Interlocked.Add to defensively prevent + // NumProcessingWork from underflowing. See the setter for NumProcessingWork. ThreadCounts counts = threadPoolInstance._separated.counts; while (true) {