Fix cpu affinity set size to have minimal size - #133497
Conversation
There is a problem when the /sys/devices/system/cpu/possible reports less CPUs than the kernel keeps as `nr_cpu_ids`. My recent change has made the CPU set size dynamic based on the possible CPU count. In the problematic case, kernel returns failure from the sched_getaffinity syscall, since it requires the passed in CPU set size to be >= `nr_cpu_ids`. This change fixes it by always using CPU_SETSIZE as the minimum size allocated, which is the default size when the CPU set is not allocated dynamically and which was used before my recent fix. Close dotnet#133449
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly scoped and consistent across affected Unix implementations, with only a minor whitespace nit noted.
Pull request overview
This PR adjusts several Unix CPU-affinity code paths to ensure the cpu_set_t buffer passed to sched_getaffinity/sched_setaffinity is never smaller than CPU_SETSIZE, avoiding failures when the kernel’s nr_cpu_ids exceeds the CPU count inferred from /sys/devices/system/cpu/possible.
Changes:
- Allocate
cpu_set_twithmax(configuredCpuCount, CPU_SETSIZE)to guarantee a minimum affinity mask size. - Reuse the computed
cpuSetSizeconsistently when callingsched_setaffinity/CPU_COUNT_S. - Apply the same sizing logic across CoreCLR PAL, CoreCLR GC Unix env, and NativeAOT PAL to keep behavior consistent.
File summaries
| File | Description |
|---|---|
| src/coreclr/pal/src/thread/thread.cpp | Ensures affinity mask allocation is at least CPU_SETSIZE and reuses cpuSetSize for sched_setaffinity. |
| src/coreclr/pal/src/misc/sysinfo.cpp | Ensures sched_getaffinity uses a mask sized to at least CPU_SETSIZE before counting CPUs. |
| src/coreclr/nativeaot/Runtime/unix/PalUnix.cpp | Aligns NativeAOT CPU-count initialization with the minimum CPU_SETSIZE affinity mask allocation. |
| src/coreclr/gc/unix/gcenv.unix.cpp | Ensures GC’s affinity probing and thread affinity setting allocate a mask sized to at least CPU_SETSIZE. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| { | ||
| // We should not get any of the errors that the sched_getaffinity can return since none | ||
| // of them applies for the current thread, so this is an unexpected kind of failure. | ||
| assert(false); |
There was a problem hiding this comment.
We have seen this happen now.
I recommend to implement a fallback here, similar to the other code branches.
There was a problem hiding this comment.
@lg2de do you mean that it happened for you with this change?
There was a problem hiding this comment.
No, I think the statement was hit on my system. No assertion was raised because of Release build. But, there is no other active code, which results into "0 CPUs".
This is why I recommend to use here the same code which is used below in the "#else // HAVE_SCHED_GETAFFINITY" branch, or similar fallback.
There was a problem hiding this comment.
🟡 Changes recommended
GC Unix changes introduce a Debug-only abort that defeats the newly-added fallback, and one new call site uses a uint32_t cpu count with APIs that expect an int, risking conversion warnings/errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/coreclr/gc/unix/gcenv.unix.cpp:911
- In SetThreadAffinity,
cpusToAllocateisuint32_t, but theCPU_ALLOC/CPU_ALLOC_SIZEfamily takes anintcpu count on Linux. Keeping this asint(as done in the other call sites in this PR) avoids signed/unsigned conversions and potential -Wconversion/-Wsign-conversion build breaks.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| // We should not get any of the errors that the sched_getaffinity can return since none | ||
| // of them applies for the current thread, so this is an unexpected kind of failure. | ||
| assert(false); | ||
| // Fallback: if sched_getaffinity fails, assume all CPUs are available. |
There is a problem when the /sys/devices/system/cpu/possible reports less CPUs than the kernel keeps as
nr_cpu_ids. My recent change has made the CPU set size dynamic based on the possible CPU count. In the problematic case, kernel returns failure from the sched_getaffinity syscall, since it requires the passed in CPU set size to be >=nr_cpu_ids.This change fixes it by always using CPU_SETSIZE as the minimum size allocated, which is the default size when the CPU set is not allocated dynamically and which was used before my recent fix.
Close #133449