-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Suppress wake-up preemption on Linux in LowLevelLifoSemaphore (parallel to Windows SetThreadPriorityBoost) #129395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
e3fc42a
5bbca72
9739f35
5a789aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Runtime.InteropServices; | ||
|
|
||
| internal static partial class Interop | ||
| { | ||
| internal static partial class Sys | ||
| { | ||
| [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_SuppressWakePreemption")] | ||
| internal static unsafe partial int SuppressWakePreemption(int* previousPolicy, int* previousPriority); | ||
|
|
||
| [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_RestoreWakePreemption")] | ||
| internal static partial int RestoreWakePreemption(int previousPolicy, int previousPriority); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace System.Threading | ||
| { | ||
| internal sealed partial class LowLevelLifoSemaphore | ||
| { | ||
| private readonly struct WakePreemptionScope | ||
| { | ||
| #if TARGET_LINUX | ||
|
VSadov marked this conversation as resolved.
|
||
| internal readonly int PreviousPolicy; | ||
| internal readonly int PreviousPriority; | ||
|
|
||
| internal WakePreemptionScope(int previousPolicy, int previousPriority) | ||
| { | ||
| PreviousPolicy = previousPolicy; | ||
| PreviousPriority = previousPriority; | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| private static WakePreemptionScope SuppressWakePreemption() | ||
| { | ||
| #if TARGET_WINDOWS | ||
| // GetCurrentThread() returns a pseudo-handle (-2) that is valid | ||
| // only on the calling thread and does not need to be closed. | ||
| Interop.Kernel32.SetThreadPriorityBoost(Interop.Kernel32.GetCurrentThread(), bDisablePriorityBoost: true); | ||
| return default; | ||
| #elif TARGET_LINUX | ||
|
VSadov marked this conversation as resolved.
|
||
| int previousPolicy = -1; | ||
| int previousPriority = 0; | ||
| unsafe | ||
| { | ||
| Interop.Sys.SuppressWakePreemption(&previousPolicy, &previousPriority); | ||
| } | ||
|
|
||
| return new WakePreemptionScope(previousPolicy, previousPriority); | ||
| #else | ||
| return default; | ||
| #endif | ||
| } | ||
|
|
||
| private static void RestoreWakePreemption(WakePreemptionScope scope) | ||
| { | ||
| #if TARGET_WINDOWS | ||
| Interop.Kernel32.SetThreadPriorityBoost(Interop.Kernel32.GetCurrentThread(), bDisablePriorityBoost: false); | ||
| #elif TARGET_LINUX | ||
|
VSadov marked this conversation as resolved.
|
||
| if (scope.PreviousPolicy != -1) | ||
| { | ||
| Interop.Sys.RestoreWakePreemption(scope.PreviousPolicy, scope.PreviousPriority); | ||
| } | ||
| #else | ||
| _ = scope; | ||
| #endif | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,15 +320,14 @@ private bool Block(int timeoutMs) | |
|
|
||
| if (blockerNode != null) | ||
| { | ||
| #if TARGET_WINDOWS | ||
| // Disable the priority boost that Windows would normally apply | ||
| // Suppress the wake-up preemption that the OS would normally apply | ||
| // when this thread is unblocked. The semaphore is used to park workers. | ||
| // A transient priority boost on wake provides no benefit here and can | ||
| // result in woken thread preempting already working threads. | ||
| // GetCurrentThread() returns a pseudo-handle (-2) that is valid | ||
| // only on the calling thread and does not need to be closed. | ||
| Interop.Kernel32.SetThreadPriorityBoost(Interop.Kernel32.GetCurrentThread(), bDisablePriorityBoost: true); | ||
| #endif | ||
| // A transient wake-up advantage provides no benefit here and can result in | ||
| // the woken thread preempting already-working threads. On Windows this | ||
| // disables SetThreadPriorityBoost. On Linux this temporarily switches the | ||
| // thread to SCHED_BATCH, which sched(7) documents as applying "a small | ||
| // scheduling penalty with respect to wakeup behavior". | ||
|
|
||
| WakePreemptionScope wakePreemptionScope = SuppressWakePreemption(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are threadpool threads under control of the runtime. We do not need to optimize/anticipate cases where these syscalls do not work since not working is both unexpected and benign. @copilot Add an assert to make sure that the syscalls work in common/typical scenarios.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
|
|
||
| try | ||
| { | ||
|
|
@@ -349,10 +348,7 @@ private bool Block(int timeoutMs) | |
| } | ||
| finally | ||
| { | ||
| #if TARGET_WINDOWS | ||
| // restore the default. | ||
| Interop.Kernel32.SetThreadPriorityBoost(Interop.Kernel32.GetCurrentThread(), bDisablePriorityBoost: false); | ||
| #endif | ||
| RestoreWakePreemption(wakePreemptionScope); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot these are threadpool threads. The user is not expected to change varioaus parameters like scheduler or priority from defaults. Even if user does so, it would be ok and perhaps even a mildly good thing if suppressing/restoring would revert thread state to defaults.
I do not think we need to capture and revert to the original state. We should revert to default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified as requested.
RestoreWakePreemptionnow unconditionally setsSCHED_OTHER(priority 0) instead of capturing and restoring the previous state. The native API is now parameterless — suppress setsSCHED_BATCH, restore setsSCHED_OTHER.WakePreemptionScopeon Linux carries only abool Suppressedto guard the restore call. Commit:b7de4e3.