sched, cgroup: Don't reject lower cpu.max on ancestors
Tejun Heo
maintainerPATCH 1/1 sched, cgroup: Don't reject lower cpu.max on ancestors
While adding cgroup2 interface for the cpu controller, 0d5936344f30 ("sched: Implement interface for cgroup unified hierarchy") forgot to update input validation and left it to reject cpu.max config if any descendant has set a higher value.
cgroup2 officially supports delegation and a descendant must not be able to restrict what its ancestors can configure. For absolute limits such as cpu.max and memory.max, this means that the config at each level should only act as the upper limit at that level and shouldn't interfere with what other cgroups can configure.
This patch updates config validation on cgroup2 so that the cpu controller follows the same convention.
--- kernel/sched/core.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
kernel/sched/core.c +10 -5
@@ -6610,13 +6610,18 @@ static int tg_cfs_schedulable_down(struct task_group *tg, void *data) parent_quota = parent_b->hierarchical_quota;/* -* Ensure max(child_quota) <= parent_quota, inherit when no +* Ensure max(child_quota) <= parent_quota. On cgroup2, +* always take the min. On cgroup1, only inherit when no * limit is set: */ -if (quota ==RUNTIME_INF ) - quota = parent_quota; -else if (parent_quota !=RUNTIME_INF && quota > parent_quota) -return -EINVAL ; +if (cgroup_subsys_on_dfl (cpu_cgrp_subsys)) { + quota =min (quota, parent_quota); + }else { +if (quota ==RUNTIME_INF ) + quota = parent_quota; +else if (parent_quota !=RUNTIME_INF && quota > parent_quota) +return -EINVAL ; + } } cfs_b->hierarchical_quota = quota;
Tejun Heo
maintainerHello,
Quoted 11 lines · Tejun Heo
While adding cgroup2 interface for the cpu controller,
0d5936344f30("sched: Implement interface for cgroup unified hierarchy") forgot to update input validation and left it to reject cpu.max config if any descendant has set a higher value.cgroup2 officially supports delegation and a descendant must not be able to restrict what its ancestors can configure. For absolute limits such as cpu.max and memory.max, this means that the config at each level should only act as the upper limit at that level and shouldn't interfere with what other cgroups can configure.
Quoted 5 lines
This patch updates config validation on cgroup2 so that the cpu controller follows the same convention.
Signed-off-by Tejun Heo <[email protected]>Fixes 0d5936344f30 ("sched: Implement interface for cgroup unified hierarchy")
Peter, can you please take a look? I can route it through cgroup tree if that's preferable.
Thanks.
Signature · 1 line
tejun
Peter Zijlstra
maintainerQuoted 10 lines · Tejun Heo
While adding cgroup2 interface for the cpu controller,
0d5936344f30("sched: Implement interface for cgroup unified hierarchy") forgot to update input validation and left it to reject cpu.max config if any descendant has set a higher value.cgroup2 officially supports delegation and a descendant must not be able to restrict what its ancestors can configure. For absolute limits such as cpu.max and memory.max, this means that the config at each level should only act as the upper limit at that level and shouldn't interfere with what other cgroups can configure.
*blink* what?
afaiu the existing code does exactly the opposite, it forces the descendants to configure less than the parent allows.
You're taking out an error condition and silently allowing descentant misconfiguration. How does that make sense?
Tejun Heo
maintainerHello,
Quoted 5 lines · Peter Zijlstra
afaiu the existing code does exactly the opposite, it forces the descendants to configure less than the parent allows.
You're taking out an error condition and silently allowing descentant misconfiguration. How does that make sense?
Well, they're upper limits, not strict allocations. The current behavior implemented by cpu isn't either a strict allocation or upper limits. It disallows a child from having a value higher than the parent (allocation-ish) but the sum of the children is allowed to exceed the parent's (limit-ish).
The combination is rather arbitrary and makes it impossible to delegate safely (a delegatee can block the delegator from reducing the amount resource allocated to the delegatee) while not really protecting against overcommit from descendants either.
We had this sort of input validations in different controllers all in their own ways. In most cases, these aren't well thought out and we can't support things like delegation without aligning controller behaviors.
Thanks.
Signature · 1 line
tejun
Quoted 6 lines · Tejun Heo
Hello,
Quoted 3 lines · Peter Zijlstra
afaiu the existing code does exactly the opposite, it forces the descendants to configure less than the parent allows.
Quoted 8 lines
Quoted 2 lines
You're taking out an error condition and silently allowing descentant misconfiguration. How does that make sense?
Well, they're upper limits, not strict allocations. The current behavior implemented by cpu isn't either a strict allocation or upper limits. It disallows a child from having a value higher than the parent (allocation-ish) but the sum of the children is allowed to exceed the parent's (limit-ish).
True; but its still weird to have the parent 'promise' something and then retract that 'promise' later.
Quoted 9 lines
The combination is rather arbitrary and makes it impossible to delegate safely (a delegatee can block the delegator from reducing the amount resource allocated to the delegatee) while not really protecting against overcommit from descendants either.
We had this sort of input validations in different controllers all in their own ways. In most cases, these aren't well thought out and we can't support things like delegation without aligning controller behaviors.
I suppose..
Tejun Heo
maintainerHello,
Quoted 8 lines · Peter Zijlstra
Quoted 5 lines
Well, they're upper limits, not strict allocations. The current behavior implemented by cpu isn't either a strict allocation or upper limits. It disallows a child from having a value higher than the parent (allocation-ish) but the sum of the children is allowed to exceed the parent's (limit-ish).
True; but its still weird to have the parent 'promise' something and then retract that 'promise' later.
Yeah, depending on how you look at it, it can feel weird. It's just that viewing these absolute resource limits (cpu.max, memory.{high,max}, io.max, pids.max) as upper bounds seems to be the best abstraction in terms of capturing what they do and making uses of them in a robust way.
Quoted 8 lines
Quoted 4 lines
We had this sort of input validations in different controllers all in their own ways. In most cases, these aren't well thought out and we can't support things like delegation without aligning controller behaviors.
I suppose..
Will route it through cgroup fixes branch in a week or so.
Thanks a lot.
Signature · 1 line
tejun
Quoted 11 lines · Tejun Heo
While adding cgroup2 interface for the cpu controller,
0d5936344f30("sched: Implement interface for cgroup unified hierarchy") forgot to update input validation and left it to reject cpu.max config if any descendant has set a higher value.cgroup2 officially supports delegation and a descendant must not be able to restrict what its ancestors can configure. For absolute limits such as cpu.max and memory.max, this means that the config at each level should only act as the upper limit at that level and shouldn't interfere with what other cgroups can configure.
Quoted 5 lines
This patch updates config validation on cgroup2 so that the cpu controller follows the same convention.
Signed-off-by Tejun Heo <[email protected]>Fixes 0d5936344f30 ("sched: Implement interface for cgroup unified hierarchy")
Applied to cgroup/for-4.16-fixes.
Thanks.
Signature · 1 line
tejun