agent: correct CPUShares and CPUWeight value #8341
Conversation
|
/test |
|
/test |
amshinde
added
the
backport
|
Can we merge this? @bergwolf |
|
/test |
|
Hello, can we get this merged? |
| // Minimum value of CPUShares should be 2, see https://github.com/systemd/systemd/blob/d19434fbf81db04d03c8cffa87821f754a86635b/src/basic/cgroup-util.h#L122 | ||
| if shares >= 2 { | ||
| properties.push(("CPUShares", Value::U64(shares))); | ||
| } |
There was a problem hiding this comment.
So this is basically ignoring an invalid value received from the runtime... not correcting anything unlike the title seems to suggest. Is it appropriate to continue execution ? FWIW the memory subsystem bails out when receiving an invalid value. See https://github.com/kata-containers/kata-containers/blob/main/src/agent/rustjail/src/cgroups/systemd/subsystem/memory.rs#L42 .
There was a problem hiding this comment.
What's the default value of cpuShares? 100?
There was a problem hiding this comment.
What's the default value of cpuShares? 100?
See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/sched/sched.h#n409
/*
* A weight of 0 or 1 can cause arithmetics problems.
* A weight of a cfs_rq is the sum of weights of which entities
* are queued on this cfs_rq, so a weight of a entity should not be
* too large, so as the shares value of a task group.
* (The default weight is 1024 - so there's no practical
* limitation from this.)
*/
#define MIN_SHARES (1UL << 1)
#define MAX_SHARES (1UL << 18)
You can :
- convert runtime input of 0 to 1024
- bail out if value is not in the MIN_SHARES...MAX_SHARES range
| @@ -81,7 +84,10 @@ impl Cpu { | |||
| ) -> Result<()> { | |||
| if let Some(shares) = cpu_resources.shares { | |||
| let weight = shares_to_weight(shares); | |||
| properties.push(("CPUWeight", Value::U64(weight))); | |||
| // CPUWeigth can not be 0. See https://github.com/systemd/systemd/blob/d19434fbf81db04d03c8cffa87821f754a86635b/src/basic/cgroup-util.h#L91 | |||
There was a problem hiding this comment.
AFAICT shares_to_weight() already takes care of never returning 0.
It even converts an input of 0 shares to 100, which is the default value for weight.
There was a problem hiding this comment.
Similarly to cpu shares, cpu weight has a validity range.
All weights are in the range [1, 10000] with the default at 100. This
allows symmetric multiplicative biases in both directions at fine
enough granularity while staying in the intuitive range.
Maybe bail out if value is out of range, which happens if passed cpu shares is > 262144, for consistency.
jongwu
force-pushed
the
fix_cpushares
branch
from
b6de068
to
4e80896
Compare
|
Thanks @gkurz -, Updated. |
jongwu
force-pushed
the
fix_cpushares
branch
from
4e80896
to
2cd33de
Compare
If cgroup driver is systemd, CPUShares, for cgroup v1, should be at least 2 [1] and CPUWeight for cgroup v2, should be at least 1 [2]. Fixes: kata-containers#8340 Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> [1] https://github.com/systemd/systemd/blob/d19434fbf81db04d03c8cffa87821f754a86635b/src/basic/cgroup-util.h#L122 [2] https://github.com/systemd/systemd/blob/d19434fbf81db04d03c8cffa87821f754a86635b/src/basic/cgroup-util.h#L91
jongwu
force-pushed
the
fix_cpushares
branch
from
2cd33de
to
58e88d9
Compare
katacontainersbot
added
size/small
|
/test |
If cgroup driver is systemd, CPUShares, for cgroup v1, should be at least 2 [1] and CPUWeight for cgroup v2, should be at least 1 [2].
Fixes: #8340
Signed-off-by: Jianyong Wu jianyong.wu@arm.com
[1] https://github.com/systemd/systemd/blob/d19434fbf81db04d03c8cffa87821f754a86635b/src/basic/cgroup-util.h#L122
[2] https://github.com/systemd/systemd/blob/d19434fbf81db04d03c8cffa87821f754a86635b/src/basic/cgroup-util.h#L91