Repository navigation
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
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
kkokosa
left a comment
kkokosa
left a comment
There was a problem hiding this comment.
LGTM. Maybe longer term, retrying sched_getaffinity with a larger mask on EINVAL would be more robust? Not sure if it is worth it.
|
/backport to 11.0 |
|
Started backporting to |
|
/backport to 10.0 |
|
Started backporting to |
|
@janvorli an error occurred while backporting to |
|
@janvorli an error occurred while backporting to |
Did you mean |
|
@lg2de yes, I've meant that. Anyways, I've created the backports manually afterwards. |
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