config-linux: add idle option for container cgroup #1136
Conversation
wineway
requested review from
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
mrunalp,
tianon and
vbatts
as code owners
Signed-off-by: wineway <[email protected]>
|
LGTM |
|
Looking at torvalds/linux@3040003, I think this was even in 5.15. 😅 |
| @@ -361,6 +361,7 @@ The following parameters can be specified to set up the controller: | |||
| * **`realtimePeriod`** *(uint64, OPTIONAL)* - same as **`period`** but applies to realtime scheduler only | |||
| * **`cpus`** *(string, OPTIONAL)* - list of CPUs the container will run in | |||
| * **`mems`** *(string, OPTIONAL)* - list of Memory Nodes the container will run in | |||
| * **`idle`** *(int64, OPTIONAL)* - cgroups are configured with minimum weight, 0: default behavior, 1: SCHED_IDLE. | |||
There was a problem hiding this comment.
I'm getting mixed signals on the actual "type" of this value from torvalds/linux@3040003#diff-1b49efb1a709264f46f4ac518b965e9620acc2f5b928209547313b8dfc9f1286R400 (int) vs torvalds/linux@3040003#diff-1b49efb1a709264f46f4ac518b965e9620acc2f5b928209547313b8dfc9f1286R511 (long) -- it doesn't make a huge difference since the only values are 0 and 1, but I think any future changes/improvements will benefit if we match the kernel's type size from the get go. 😅
Any ideas which one is "canonical" ? (unfortunately I'm not very familiar with the kernel source to be able to parse this diff better than the two conflicting lines I found 🙈)
There was a problem hiding this comment.
Doest it support magic values like -1 ? (wondering if it would make it easier to use to use an uintXX if it only supports 0 and 1
There was a problem hiding this comment.
The text around it implies that it might someday? It does not currently. Currently it supports 0 and 1. What I'd like to do is make sure the type we choose matches the type the kernel uses, but I guess int64 fits both of C's int and long on all platforms I'm aware of, so I guess it should be plenty. 👍
| @@ -361,6 +361,7 @@ The following parameters can be specified to set up the controller: | |||
| * **`realtimePeriod`** *(uint64, OPTIONAL)* - same as **`period`** but applies to realtime scheduler only | |||
| * **`cpus`** *(string, OPTIONAL)* - list of CPUs the container will run in | |||
| * **`mems`** *(string, OPTIONAL)* - list of Memory Nodes the container will run in | |||
| * **`idle`** *(int64, OPTIONAL)* - cgroups are configured with minimum weight, 0: default behavior, 1: SCHED_IDLE. | |||
There was a problem hiding this comment.
Doest it support magic values like -1 ? (wondering if it would make it easier to use to use an uintXX if it only supports 0 and 1
specs-go/config.go
Outdated
| @@ -330,6 +330,8 @@ type LinuxCPU struct { | |||
| Cpus string `json:"cpus,omitempty"` | |||
| // List of memory nodes in the cpuset. Default is to use any available memory node. | |||
| Mems string `json:"mems,omitempty"` | |||
| // cgroups are configured with minimum weight, 0: default behavior, 1: SCHED_IDLE. Default 0 | |||
| Idle *int64 `json:"idle,omitempty"` | |||
There was a problem hiding this comment.
Do we want const / enums for these values (for easier consumption)?
There was a problem hiding this comment.
Also, if 0 is the default behavior, it may not be needed to define it as a pointer
@tianon yes, updated issue & pr comment. |
Signed-off-by: wineway <[email protected]>
| @@ -361,6 +361,7 @@ The following parameters can be specified to set up the controller: | |||
| * **`realtimePeriod`** *(uint64, OPTIONAL)* - same as **`period`** but applies to realtime scheduler only | |||
| * **`cpus`** *(string, OPTIONAL)* - list of CPUs the container will run in | |||
| * **`mems`** *(string, OPTIONAL)* - list of Memory Nodes the container will run in | |||
| * **`idle`** *(int64, OPTIONAL)* - cgroups are configured with minimum weight, 0: default behavior, 1: SCHED_IDLE. | |||
There was a problem hiding this comment.
The text around it implies that it might someday? It does not currently. Currently it supports 0 and 1. What I'd like to do is make sure the type we choose matches the type the kernel uses, but I guess int64 fits both of C's int and long on all platforms I'm aware of, so I guess it should be plenty. 👍
|
the PR doesn't update the json schema, opened another one: #1145 |
See: opencontainers/runtime-spec#1136 Signed-off-by: Aditya R <[email protected]>
See: opencontainers/runtime-spec#1136 Signed-off-by: Aditya R <[email protected]>
sched: Cgroup SCHED_IDLE support is introduced in Linux 5.15
we can mark a container SCHED_IDLE with this feature which means its sched weight are configured to the minimum
see also #1135