cg/sd: support CPUWeight=idle (aka cpu.idle=1) #3788
Conversation
kolyshkin
force-pushed
the
systemd-cpu-idle
branch
6 times, most recently
from
b8d81f3
to
f679172
Compare
And Fedora? |
F37 on my laptop has systemd v231 |
👀 I was expecting CentOS 9 Stream to be more conservative than Fedora. |
Good question. I wish I knew. I just checked, the latest F37 update has systemd 251.13-6.fc37, in CS9 it is definitely v252 |
kolyshkin
force-pushed
the
systemd-cpu-idle
branch
from
f679172
to
8adc1f3
Compare
|
It is really hard to say how to handle this properly. The kernel allows to set That means, for example, that running For example, this is what we see when changing cpu-period (see #3786), which appears to be unrelated to cpu weight and cpu idle. So, maybe we should make systemd cgroup v2 driver to deny setting some CPU-related parameters if cpu.idle was/is set to 1. |
kolyshkin
force-pushed
the
systemd-cpu-idle
branch
from
8adc1f3
to
cbcf4b9
Compare
|
Oh my, finally the CI is green again. If we merge this PR this week, we will be able to brag that it only took us a month to fix the CI (in main branch, that is). |
libcontainer/cgroups/systemd/v2.go
Outdated
| @@ -64,8 +64,7 @@ func unifiedResToSystemdProps(cm *dbusConnManager, res map[string]string) (props | |||
| if strings.Contains(k, "/") { | |||
| return nil, fmt.Errorf("unified resource %q must be a file name (no slashes)", k) | |||
| } | |||
| sk := strings.SplitN(k, ".", 2) | |||
| if len(sk) != 2 { | |||
| if strings.IndexByte(k, '.') < 2 { | |||
There was a problem hiding this comment.
why less than 2? < 1 should work, right? (Assuming there is a hypothetical one character controller in the future..)
There was a problem hiding this comment.
You're right, and I have a correct description in the commit message, but not in the code :)
Fixed
In code that checks that the resource name is in the for Using strings.SplitN is an overkill in this case, resulting in allocations and thus garbage to collect. Using strings.IndexByte and checking that result is not less than 1 (meaning there is a period, and it is not the first character) is sufficient here. Fixes: 0cb8bf6 Signed-off-by: Kir Kolyshkin <[email protected]>
Systemd v252 (available in CentOS Stream 9 in our CI) added support for setting cpu.idle (see [1]). The way it works is: - if CPUWeight == 0, cpu.idle is set to 1; - if CPUWeight != 0, cpu.idle is set to 0. This behavior breaks the existing test case, as described in [2]. To fix, skip the last check in the test case in case a newer systemd is used. Fixes: opencontainers#3786 [1] systemd/systemd#23299 [2] opencontainers#3786 Signed-off-by: Kir Kolyshkin <[email protected]>
Values other than 1 or 0 are ignored by the kernel, see sched_group_set_idle() in kernel/sched/fair.c If the added test case ever fails, it means that the kernel now accepts values other than 0 or 1, and runc needs to adopt to that. Signed-off-by: Kir Kolyshkin <[email protected]>
Systemd v252 (available in CentOS Stream 9 in our CI) added support for setting cpu.idle (see [1]). The way it works is: - if CPUWeight == 0, cpu.idle is set to 1; - if CPUWeight != 0, cpu.idle is set to 0. This commit implements setting cpu.idle in systemd cgroup driver via a unit property. In case CPUIdle is set to non-zero value, the driver sets adds CPUWeight=0 property, which will result in systemd setting cpu.idle to 1. Unfortunately, there's no way to set cpu.idle to 0 without also changing the CPUWeight value, so the driver doesn't do anything if CPUIdle is explicitly set to 0. This case is handled by the fs driver which is always used as a followup to setting systemd unit properties. Also, handle cpu.idle set via unified map. In case it is set to non-zero value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll get two different CPUWeight properties set). Add a unit test for new values in unified map, and an integration test case. [1] systemd/systemd#23299 [2] opencontainers#3786 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
systemd-cpu-idle
branch
from
cbcf4b9
to
ed9651b
Compare
Systemd v252 added support for setting cgroup v2 cpu.idle in systemd/systemd#23299
The way it works is
This PR implements support for setting
cpu.idlevia systemd cgroupv2 driver. In case CPUIdle is set to non-zero value, the driver sets
CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.
Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.
Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).
Add a unit test for new values in unified map, and an integration test case.
Modify docs to show the newly supported property.
Fixes: #3786