It was found that the cpuset.cpus could contain CPUs that are not listed
in their parent's cpu list as shown by the command sequence below:
# echo "+cpuset" >cgroup.subtree_control
# mkdir g1
# echo 0-5 >g1/cpuset.cpus
# mkdir g1/g11
# echo "+cpuset" > g1/cgroup.subtree_control
# echo 6-11 >g1/g11/cpuset.cpus
# grep -R . g1 | grep "\.cpus"
g1/cpuset.cpus:0-5
g1/cpuset.cpus.effective:0-5
g1/g11/cpuset.cpus:6-11
g1/g11/cpuset.cpus.effective:0-5
As the intersection of g11's cpus and that of g1 is empty, the effective
cpus of g11 is just that of g1. The check in update_cpumask() is now
corrected to make sure that cpus in a child cpus must be a subset of
its parent's cpus. The error "write error: Invalid argument" will now
be reported in the above case.
Reported-by: Juri Lelli <[email protected]>
Signed-off-by: Waiman Long <[email protected]>
---
kernel/cgroup/cpuset.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 71fb2d0..ceec438 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1185,12 +1185,17 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs,
if (!*buf) {
cpumask_clear(trialcs->cpus_allowed);
} else {
+ struct cpuset *parent = parent_cs(cs);
+
retval = cpulist_parse(buf, trialcs->cpus_allowed);
if (retval < 0)
return retval;
+ /*
+ * The cpu list must be a subset of the parent.
+ */
if (!cpumask_subset(trialcs->cpus_allowed,
- top_cpuset.cpus_allowed))
+ parent->cpus_allowed))
return -EINVAL;
}
--
1.8.3.1
Hi,
On 30/05/18 09:46, Waiman Long wrote:
> It was found that the cpuset.cpus could contain CPUs that are not listed
> in their parent's cpu list as shown by the command sequence below:
>
> # echo "+cpuset" >cgroup.subtree_control
> # mkdir g1
> # echo 0-5 >g1/cpuset.cpus
> # mkdir g1/g11
> # echo "+cpuset" > g1/cgroup.subtree_control
> # echo 6-11 >g1/g11/cpuset.cpus
> # grep -R . g1 | grep "\.cpus"
> g1/cpuset.cpus:0-5
> g1/cpuset.cpus.effective:0-5
> g1/g11/cpuset.cpus:6-11
> g1/g11/cpuset.cpus.effective:0-5
>
> As the intersection of g11's cpus and that of g1 is empty, the effective
> cpus of g11 is just that of g1. The check in update_cpumask() is now
> corrected to make sure that cpus in a child cpus must be a subset of
> its parent's cpus. The error "write error: Invalid argument" will now
> be reported in the above case.
>
> Reported-by: Juri Lelli <[email protected]>
> Signed-off-by: Waiman Long <[email protected]>
Looks like it fixes the bug.
Reviewed-and-Tested-by: Juri Lelli <[email protected]>
Thanks,
- Juri
Hi Waiman,
On 2018/5/30 21:46, Waiman Long wrote:
> It was found that the cpuset.cpus could contain CPUs that are not listed
> in their parent's cpu list as shown by the command sequence below:
>
> # echo "+cpuset" >cgroup.subtree_control
> # mkdir g1
> # echo 0-5 >g1/cpuset.cpus
> # mkdir g1/g11
> # echo "+cpuset" > g1/cgroup.subtree_control
> # echo 6-11 >g1/g11/cpuset.cpus
> # grep -R . g1 | grep "\.cpus"
> g1/cpuset.cpus:0-5
> g1/cpuset.cpus.effective:0-5
> g1/g11/cpuset.cpus:6-11
> g1/g11/cpuset.cpus.effective:0-5
>
> As the intersection of g11's cpus and that of g1 is empty, the effective
> cpus of g11 is just that of g1. The check in update_cpumask() is now
> corrected to make sure that cpus in a child cpus must be a subset of
> its parent's cpus. The error "write error: Invalid argument" will now
> be reported in the above case.
>
We made the distinction between user-configured CPUs and effective CPUs
in commit 7e88291beefbb758, so actually it's not a bug.
On Thu, May 31, 2018 at 09:25:29AM +0800, Zefan Li wrote:
> Hi Waiman,
>
> On 2018/5/30 21:46, Waiman Long wrote:
> > It was found that the cpuset.cpus could contain CPUs that are not listed
> > in their parent's cpu list as shown by the command sequence below:
> >
> > # echo "+cpuset" >cgroup.subtree_control
> > # mkdir g1
> > # echo 0-5 >g1/cpuset.cpus
> > # mkdir g1/g11
> > # echo "+cpuset" > g1/cgroup.subtree_control
> > # echo 6-11 >g1/g11/cpuset.cpus
> > # grep -R . g1 | grep "\.cpus"
> > g1/cpuset.cpus:0-5
> > g1/cpuset.cpus.effective:0-5
> > g1/g11/cpuset.cpus:6-11
> > g1/g11/cpuset.cpus.effective:0-5
> >
> > As the intersection of g11's cpus and that of g1 is empty, the effective
> > cpus of g11 is just that of g1. The check in update_cpumask() is now
> > corrected to make sure that cpus in a child cpus must be a subset of
> > its parent's cpus. The error "write error: Invalid argument" will now
> > be reported in the above case.
> >
>
> We made the distinction between user-configured CPUs and effective CPUs
> in commit 7e88291beefbb758, so actually it's not a bug.
Why though; that makes no sense what so ever.
On 2018/5/31 9:25, Zefan Li wrote:
> Hi Waiman,
>
> On 2018/5/30 21:46, Waiman Long wrote:
>> It was found that the cpuset.cpus could contain CPUs that are not listed
>> in their parent's cpu list as shown by the command sequence below:
>>
>> # echo "+cpuset" >cgroup.subtree_control
>> # mkdir g1
>> # echo 0-5 >g1/cpuset.cpus
>> # mkdir g1/g11
>> # echo "+cpuset" > g1/cgroup.subtree_control
>> # echo 6-11 >g1/g11/cpuset.cpus
>> # grep -R . g1 | grep "\.cpus"
>> g1/cpuset.cpus:0-5
>> g1/cpuset.cpus.effective:0-5
>> g1/g11/cpuset.cpus:6-11
>> g1/g11/cpuset.cpus.effective:0-5
>>
>> As the intersection of g11's cpus and that of g1 is empty, the effective
>> cpus of g11 is just that of g1. The check in update_cpumask() is now
>> corrected to make sure that cpus in a child cpus must be a subset of
>> its parent's cpus. The error "write error: Invalid argument" will now
>> be reported in the above case.
>>
>
> We made the distinction between user-configured CPUs and effective CPUs
> in commit 7e88291beefbb758, so actually it's not a bug.
>
I remember the original reason is to support restoration of the original
cpu after cpu offline->online. We use user-configured CPUs to remember
if the cpu should be restored in the cpuset after it's onlined.
On Thu, May 31, 2018 at 04:12:34PM +0800, Zefan Li wrote:
> On 2018/5/31 9:25, Zefan Li wrote:
> > Hi Waiman,
> >
> > On 2018/5/30 21:46, Waiman Long wrote:
> >> It was found that the cpuset.cpus could contain CPUs that are not listed
> >> in their parent's cpu list as shown by the command sequence below:
> >>
> >> # echo "+cpuset" >cgroup.subtree_control
> >> # mkdir g1
> >> # echo 0-5 >g1/cpuset.cpus
> >> # mkdir g1/g11
> >> # echo "+cpuset" > g1/cgroup.subtree_control
> >> # echo 6-11 >g1/g11/cpuset.cpus
> >> # grep -R . g1 | grep "\.cpus"
> >> g1/cpuset.cpus:0-5
> >> g1/cpuset.cpus.effective:0-5
> >> g1/g11/cpuset.cpus:6-11
> >> g1/g11/cpuset.cpus.effective:0-5
> >>
> >> As the intersection of g11's cpus and that of g1 is empty, the effective
> >> cpus of g11 is just that of g1. The check in update_cpumask() is now
> >> corrected to make sure that cpus in a child cpus must be a subset of
> >> its parent's cpus. The error "write error: Invalid argument" will now
> >> be reported in the above case.
> >>
> >
> > We made the distinction between user-configured CPUs and effective CPUs
> > in commit 7e88291beefbb758, so actually it's not a bug.
> >
>
> I remember the original reason is to support restoration of the original
> cpu after cpu offline->online. We use user-configured CPUs to remember
> if the cpu should be restored in the cpuset after it's onlined.
AFAICT you can do that and still have the child a subset of the parent,
no?
On 31/05/18 10:26, Peter Zijlstra wrote:
> On Thu, May 31, 2018 at 04:12:34PM +0800, Zefan Li wrote:
> > On 2018/5/31 9:25, Zefan Li wrote:
> > > Hi Waiman,
> > >
> > > On 2018/5/30 21:46, Waiman Long wrote:
> > >> It was found that the cpuset.cpus could contain CPUs that are not listed
> > >> in their parent's cpu list as shown by the command sequence below:
> > >>
> > >> # echo "+cpuset" >cgroup.subtree_control
> > >> # mkdir g1
> > >> # echo 0-5 >g1/cpuset.cpus
> > >> # mkdir g1/g11
> > >> # echo "+cpuset" > g1/cgroup.subtree_control
> > >> # echo 6-11 >g1/g11/cpuset.cpus
> > >> # grep -R . g1 | grep "\.cpus"
> > >> g1/cpuset.cpus:0-5
> > >> g1/cpuset.cpus.effective:0-5
> > >> g1/g11/cpuset.cpus:6-11
> > >> g1/g11/cpuset.cpus.effective:0-5
> > >>
> > >> As the intersection of g11's cpus and that of g1 is empty, the effective
> > >> cpus of g11 is just that of g1. The check in update_cpumask() is now
> > >> corrected to make sure that cpus in a child cpus must be a subset of
> > >> its parent's cpus. The error "write error: Invalid argument" will now
> > >> be reported in the above case.
> > >>
> > >
> > > We made the distinction between user-configured CPUs and effective CPUs
> > > in commit 7e88291beefbb758, so actually it's not a bug.
> > >
> >
> > I remember the original reason is to support restoration of the original
> > cpu after cpu offline->online. We use user-configured CPUs to remember
> > if the cpu should be restored in the cpuset after it's onlined.
>
> AFAICT you can do that and still have the child a subset of the parent,
> no?
Plus this is not hotplug, but a user decision. It could make sense to
keep .cpus unmodified after hotplug events, but does it make sense to
let the user be able to choose cpus outside the parent domain?
On 2018/5/31 16:26, Peter Zijlstra wrote:
> On Thu, May 31, 2018 at 04:12:34PM +0800, Zefan Li wrote:
>> On 2018/5/31 9:25, Zefan Li wrote:
>>> Hi Waiman,
>>>
>>> On 2018/5/30 21:46, Waiman Long wrote:
>>>> It was found that the cpuset.cpus could contain CPUs that are not listed
>>>> in their parent's cpu list as shown by the command sequence below:
>>>>
>>>> # echo "+cpuset" >cgroup.subtree_control
>>>> # mkdir g1
>>>> # echo 0-5 >g1/cpuset.cpus
>>>> # mkdir g1/g11
>>>> # echo "+cpuset" > g1/cgroup.subtree_control
>>>> # echo 6-11 >g1/g11/cpuset.cpus
>>>> # grep -R . g1 | grep "\.cpus"
>>>> g1/cpuset.cpus:0-5
>>>> g1/cpuset.cpus.effective:0-5
>>>> g1/g11/cpuset.cpus:6-11
>>>> g1/g11/cpuset.cpus.effective:0-5
>>>>
>>>> As the intersection of g11's cpus and that of g1 is empty, the effective
>>>> cpus of g11 is just that of g1. The check in update_cpumask() is now
>>>> corrected to make sure that cpus in a child cpus must be a subset of
>>>> its parent's cpus. The error "write error: Invalid argument" will now
>>>> be reported in the above case.
>>>>
>>>
>>> We made the distinction between user-configured CPUs and effective CPUs
>>> in commit 7e88291beefbb758, so actually it's not a bug.
>>>
>>
>> I remember the original reason is to support restoration of the original
>> cpu after cpu offline->online. We use user-configured CPUs to remember
>> if the cpu should be restored in the cpuset after it's onlined.
>
> AFAICT you can do that and still have the child a subset of the parent,
> no?
> .
Sure. IIRC this was suggested by Tejun as he had done something similar to devcgroup.
On 05/31/2018 04:42 AM, Zefan Li wrote:
> On 2018/5/31 16:26, Peter Zijlstra wrote:
>> On Thu, May 31, 2018 at 04:12:34PM +0800, Zefan Li wrote:
>>> On 2018/5/31 9:25, Zefan Li wrote:
>>>> Hi Waiman,
>>>>
>>>> On 2018/5/30 21:46, Waiman Long wrote:
>>>>> It was found that the cpuset.cpus could contain CPUs that are not listed
>>>>> in their parent's cpu list as shown by the command sequence below:
>>>>>
>>>>> # echo "+cpuset" >cgroup.subtree_control
>>>>> # mkdir g1
>>>>> # echo 0-5 >g1/cpuset.cpus
>>>>> # mkdir g1/g11
>>>>> # echo "+cpuset" > g1/cgroup.subtree_control
>>>>> # echo 6-11 >g1/g11/cpuset.cpus
>>>>> # grep -R . g1 | grep "\.cpus"
>>>>> g1/cpuset.cpus:0-5
>>>>> g1/cpuset.cpus.effective:0-5
>>>>> g1/g11/cpuset.cpus:6-11
>>>>> g1/g11/cpuset.cpus.effective:0-5
>>>>>
>>>>> As the intersection of g11's cpus and that of g1 is empty, the effective
>>>>> cpus of g11 is just that of g1. The check in update_cpumask() is now
>>>>> corrected to make sure that cpus in a child cpus must be a subset of
>>>>> its parent's cpus. The error "write error: Invalid argument" will now
>>>>> be reported in the above case.
>>>>>
>>>> We made the distinction between user-configured CPUs and effective CPUs
>>>> in commit 7e88291beefbb758, so actually it's not a bug.
>>>>
>>> I remember the original reason is to support restoration of the original
>>> cpu after cpu offline->online. We use user-configured CPUs to remember
>>> if the cpu should be restored in the cpuset after it's onlined.
>> AFAICT you can do that and still have the child a subset of the parent,
>> no?
>> .
> Sure. IIRC this was suggested by Tejun as he had done something similar to devcgroup.
>
OK, let wait until Tejun has time to chime in. For me, it just look
weird to be able to do that.
Another corner case that is not handled is when cpus_allowed is empty.
In this case, it falls back to the parent's effective cpus. On the other
hand, it can also be argued that an empty cpus_allowed is a transient
state and a cpuset shouldn't have cpus undefined while creating children.
Cheers,
Longman
Hello,
On Thu, May 31, 2018 at 09:22:23AM -0400, Waiman Long wrote:
> >>>>> As the intersection of g11's cpus and that of g1 is empty, the effective
> >>>>> cpus of g11 is just that of g1. The check in update_cpumask() is now
> >>>>> corrected to make sure that cpus in a child cpus must be a subset of
> >>>>> its parent's cpus. The error "write error: Invalid argument" will now
> >>>>> be reported in the above case.
> >>>>>
> >>>> We made the distinction between user-configured CPUs and effective CPUs
> >>>> in commit 7e88291beefbb758, so actually it's not a bug.
> >>>>
> >>> I remember the original reason is to support restoration of the original
> >>> cpu after cpu offline->online. We use user-configured CPUs to remember
> >>> if the cpu should be restored in the cpuset after it's onlined.
> >> AFAICT you can do that and still have the child a subset of the parent,
> >> no?
> >> .
> > Sure. IIRC this was suggested by Tejun as he had done something similar to devcgroup.
> >
> OK, let wait until Tejun has time to chime in. For me, it just look
> weird to be able to do that.
>
> Another corner case that is not handled is when cpus_allowed is empty.
> In this case, it falls back to the parent's effective cpus. On the other
> hand, it can also be argued that an empty cpus_allowed is a transient
> state and a cpuset shouldn't have cpus undefined while creating children.
Tying together what's configured and what's applied may feel
attractive on the surface but it's a long term headache.
* It's inconsistent with what other controllers are doing. All the
limit resource configs declare the upper bound the specific cgroup
can consume regardless of what's actually available to it. They
limit but don't guarantee access.
* Which decouples a given cgroup's configurations from its ancestors',
which allows an ancestor to take away resources that it granted
before and then also giving it back later. No matter what you do,
if you couple configs of cgroup hierarchy, you end up restricting
what an ancestor can do to its sub-hierarchy, which can quickly
become a difficult operational headache.
So, let's please stay away from it even if that means a bit of
overhead in terms of interface.
Thanks.
--
tejun
On Thu, May 31, 2018 at 08:58:07AM -0700, Tejun Heo wrote:
> Tying together what's configured and what's applied may feel
> attractive on the surface but it's a long term headache.
>
> * It's inconsistent with what other controllers are doing. All the
> limit resource configs declare the upper bound the specific cgroup
> can consume regardless of what's actually available to it. They
> limit but don't guarantee access.
>
> * Which decouples a given cgroup's configurations from its ancestors',
> which allows an ancestor to take away resources that it granted
> before and then also giving it back later. No matter what you do,
> if you couple configs of cgroup hierarchy, you end up restricting
> what an ancestor can do to its sub-hierarchy, which can quickly
> become a difficult operational headache.
>
> So, let's please stay away from it even if that means a bit of
> overhead in terms of interface.
Urgh, that again :/
I'm still not convinced by your arguments though. The root container can
access all the sub-groups anyway and can grub around in them to take
away resources if it really wants to.
For cpuset in particular randomly restricting on the ancestor level can
create an unrecoverable trainwreck inside a container. Affinities are
not recoverable. Once a runnable task ends up with an empty set, its
affinities are reset and the smaller (empty) set is lost.
Hello,
On Thu, May 31, 2018 at 06:16:45PM +0200, Peter Zijlstra wrote:
> > So, let's please stay away from it even if that means a bit of
> > overhead in terms of interface.
>
> Urgh, that again :/
Yeah, well, it's pretty important.
> I'm still not convinced by your arguments though. The root container can
> access all the sub-groups anyway and can grub around in them to take
> away resources if it really wants to.
That's really messy and if you delegated away a subtree, you can't
walk the subtree in a race free way, not easily anyway.
> For cpuset in particular randomly restricting on the ancestor level can
> create an unrecoverable trainwreck inside a container. Affinities are
> not recoverable. Once a runnable task ends up with an empty set, its
> affinities are reset and the smaller (empty) set is lost.
Yeah, for cpuset, it's messier, but it isn't different from hotunplug
scenario, right? I think the best we can do there is putting ancestor
operation on an equal footing as hotplug ops.
Thanks.
--
tejun
On 05/31/2018 11:58 AM, Tejun Heo wrote:
> Hello,
>
> On Thu, May 31, 2018 at 09:22:23AM -0400, Waiman Long wrote:
>>>>>>> As the intersection of g11's cpus and that of g1 is empty, the effective
>>>>>>> cpus of g11 is just that of g1. The check in update_cpumask() is now
>>>>>>> corrected to make sure that cpus in a child cpus must be a subset of
>>>>>>> its parent's cpus. The error "write error: Invalid argument" will now
>>>>>>> be reported in the above case.
>>>>>>>
>>>>>> We made the distinction between user-configured CPUs and effective CPUs
>>>>>> in commit 7e88291beefbb758, so actually it's not a bug.
>>>>>>
>>>>> I remember the original reason is to support restoration of the original
>>>>> cpu after cpu offline->online. We use user-configured CPUs to remember
>>>>> if the cpu should be restored in the cpuset after it's onlined.
>>>> AFAICT you can do that and still have the child a subset of the parent,
>>>> no?
>>>> .
>>> Sure. IIRC this was suggested by Tejun as he had done something similar to devcgroup.
>>>
>> OK, let wait until Tejun has time to chime in. For me, it just look
>> weird to be able to do that.
>>
>> Another corner case that is not handled is when cpus_allowed is empty.
>> In this case, it falls back to the parent's effective cpus. On the other
>> hand, it can also be argued that an empty cpus_allowed is a transient
>> state and a cpuset shouldn't have cpus undefined while creating children.
> Tying together what's configured and what's applied may feel
> attractive on the surface but it's a long term headache.
>
> * It's inconsistent with what other controllers are doing. All the
> limit resource configs declare the upper bound the specific cgroup
> can consume regardless of what's actually available to it. They
> limit but don't guarantee access.
>
> * Which decouples a given cgroup's configurations from its ancestors',
> which allows an ancestor to take away resources that it granted
> before and then also giving it back later. No matter what you do,
> if you couple configs of cgroup hierarchy, you end up restricting
> what an ancestor can do to its sub-hierarchy, which can quickly
> become a difficult operational headache.
>
> So, let's please stay away from it even if that means a bit of
> overhead in terms of interface.
>
> Thanks.
>
I am fine with that argument. I will update the patch documentation to
include this information as I think it is important for the users to be
aware of that.
Cheers,
Longman
On Thu, May 31, 2018 at 09:19:42AM -0700, Tejun Heo wrote:
> Hello,
>
> On Thu, May 31, 2018 at 06:16:45PM +0200, Peter Zijlstra wrote:
> > > So, let's please stay away from it even if that means a bit of
> > > overhead in terms of interface.
> >
> > Urgh, that again :/
>
> Yeah, well, it's pretty important.
>
> > I'm still not convinced by your arguments though. The root container can
> > access all the sub-groups anyway and can grub around in them to take
> > away resources if it really wants to.
>
> That's really messy and if you delegated away a subtree, you can't
> walk the subtree in a race free way, not easily anyway.
Messy perhaps, but taking away resources you gave out earlier isn't
particularly nice either way around.
Not sure the races matter, if you win, the delegate can't undo it, if
you loose, you try again until you win.
It's not like cgroup stuff gets changed often, so a conflict causing you
to loose should be very rare indeed.
> > For cpuset in particular randomly restricting on the ancestor level can
> > create an unrecoverable trainwreck inside a container. Affinities are
> > not recoverable. Once a runnable task ends up with an empty set, its
> > affinities are reset and the smaller (empty) set is lost.
>
> Yeah, for cpuset, it's messier, but it isn't different from hotunplug
> scenario, right? I think the best we can do there is putting ancestor
> operation on an equal footing as hotplug ops.
Right, but hotplug is exceedingly rare, while I get the impression you
think it is perfectly fine to recind on your resource grants.
Hello, Peter.
Sorry about late reply.
On Thu, May 31, 2018 at 06:38:26PM +0200, Peter Zijlstra wrote:
> > Yeah, for cpuset, it's messier, but it isn't different from hotunplug
> > scenario, right? I think the best we can do there is putting ancestor
> > operation on an equal footing as hotplug ops.
>
> Right, but hotplug is exceedingly rare, while I get the impression you
> think it is perfectly fine to recind on your resource grants.
Well, yeah, for a trivial example, imagine dynamic workload management
where you wanna restrict what a side-loaded batch workload can do on
and off peak hours. All other controllers can do that. It'd be a
really odd design trade-off if we make that really clumsy for cpuset
especially given that we wouldn't be gaining any actual
functionalities.
Thanks.
--
tejun