Remove 1024 CPUs limit#5343
Conversation
kolyshkin
force-pushed
the
cpu-aff-dyn
branch
2 times, most recently
from
82c976c to
97ca6d1
Compare
|
Cc @askervin |
| return nil, fmt.Errorf("no members found in set %q", str) | ||
| } | ||
|
|
||
| s := unix.NewCPUSet(maxCPU + 1) |
There was a problem hiding this comment.
TIL! But I see that's something you contributed (golang/sys@690c91f)
I should probably check if we have similar assumptions elsewhere (assuming 1024 is the limit)
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
second commit is missing a DCO sign-off; only has
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| - The `cpuAffinity` and NUMA `memoryPolicy` settings are no longer limited to | ||
| 1024 CPUs/nodes, as runc now uses a dynamically-sized CPU mask. (#5343) |
There was a problem hiding this comment.
Should we mention the new (arbitrary) limit, or too high to reasonably be concerned about for users?
There was a problem hiding this comment.
I think it's too high, and the error message is clear so once someone will bump into it it's an easy fix.
AFAIK the kernel supports 8192 CPUs by default (unless specifically configured to handle more CPUs).
There was a problem hiding this comment.
For more context, see the comment added:
// maxCPUSetCPU is the highest CPU/NUMA node ID that [ToCPUSet] accepts. It is
// an arbitrary sanity limit, used to avoid allocating an unreasonably large
// mask for a bogus input.
const maxCPUSetCPU = 64 * 1024So it's more like a sanity check than a real limit. We can raise it higher if you have concerns, or remove it altogether.
|
|
||
| // SchedSetaffinity wraps sched_setaffinity syscall without unix.CPUSet size limitation. | ||
| func SchedSetaffinity(pid int, buf []byte) error { | ||
| err := retryOnEINTR(func() error { |
There was a problem hiding this comment.
I see you mentioned it;
note we're losing retry-on-eintr but I don't think it's a problem
Is this something we should still add in golang.org/x/sys ?
There was a problem hiding this comment.
By definition, x/sys/unix is a low-level thing so I guess they will not accept it.
I also think (but not sure) that sched_setaffinity is safe wrt EINTR, as it's not a blocking call (like a syscall doing I/O).
There was a problem hiding this comment.
I mean, in principle almost any syscall can EINTR? It seems like we could just replace the function we're wrapping here (i.e., just replace unix.Syscall with the new unix.SchedSetAffinityDynamic) but I don't really mind too much and we were already calling into unix before...
rata
left a comment
rata
left a comment
There was a problem hiding this comment.
LGTM. Left a nit, but I'm fine if it's not fixed as part of this PR :)
Can you fix the DCO, Kir? |
This was introduced in https://go-review.googlesource.com/c/sys/+/735380 to overcome the 1024 CPUs limit. Let's change our wrapper to use unix.SchedSetaffinityDynamic. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
ToCPUSet was limited to unix.CPUSet (a fixed-size mask capped at 1024 CPUs). Switch CPUAffinity, LinuxMemoryPolicy.Nodes, ToCPUSet, and SetMempolicy to unix.CPUSetDynamic, which can represent arbitrary CPU and NUMA node IDs. ToCPUSet now parses the input in a first pass to find the maximum value, then allocates a mask large enough to hold it (out-of-range Set calls on a dynamic mask are silently ignored). An arbitrary sanity cap is kept to avoid huge allocations on bogus input. This is backward compatible with reading older state.json: both the old *unix.CPUSet (a fixed array) and the new CPUSetDynamic (a slice) marshal to and from the same JSON array-of-numbers representation. Update TestToCPUSet accordingly, adding coverage for values beyond the old non-dynamic limit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
Addressed all review comments; PTAL @cyphar @rata @thaJeztah |
4 participants
Replace the fixed size unix.CPUMask with CPUMaskDynamic (see CL 735380).
Modify ToCPUSet to return a dynamic set.
This is backward compatible with reading older state.json: both the old *unix.CPUSet (fixed array) and the new CPUSetDynamic (slice) marshal to/from the same JSON array-of-numbers representation.