Remove 1024 CPUs limit#5343
Conversation
This was introduced in https://go-review.googlesource.com/c/sys/+/735380 to overcome the 1024 CPUs limit. Let's use it and drop our own wrapper (note we're losing retry-on-eintr but I don't think it's a problem). 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>
|
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).
|
|
||
| // 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...
| @@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
There was a problem hiding this comment.
I need to forward-port the 1.5 changelog, these entries are being mixed with stuff we already released.
| maxCPU := 0 | ||
| for _, cpu := range cpus { | ||
| if cpu > maxCPU { | ||
| maxCPU = cpu | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Aren't we on a new enough Go version to be able to just use max here?
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 :)
| if ret >= maxCPU { | ||
| return 0, fmt.Errorf("values larger than %d are not supported", maxCPU-1) | ||
| if ret > maxCPUSetCPU { |
There was a problem hiding this comment.
We are losing the "=" here. I guess because we use the "+1" below as the maxCPUSetCPU is a valid CPU to set. But just asking to make sure I'm following correctly
| // First pass: parse all ranges and find the maximum CPU value, so the | ||
| // resulting mask can be allocated large enough to hold it. | ||
| type cpuRange struct{ start, end int } | ||
| var ( | ||
| ranges []cpuRange | ||
| maxCPU int | ||
| ) |
There was a problem hiding this comment.
nit: I'd separate the parsing in another function from this one. Not very related to this PR, but if you don't mind doing it... Otherwise it's fine, we can tackle it in another PR
Can you fix the DCO, Kir? |
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.