On 07/23/2017 08:18 AM, Tejun Heo wrote: > cgroup_enable_threaded() checks that the cgroup doesn't have any tasks > or children and fails the operation if so. This test is unnecessary > because the first part is already checked by > cgroup_can_be_thread_root() and the latter is unnecessary. The latter > actually cause a behavioral oddity. Please consider the following > hierarchy. All cgroups are domains. > > A > / \ > B C > \ > D > > If B is made threaded, C and D becomes invalid domains. Due to the no > children restriction, threaded mode can't be enabled on C. For C and > D, the only thing the user can do is removal. > > There is no reason for this restriction. Remove it. > > Signed-off-by: Tejun Heo <[email protected]> > Cc: Waiman Long <[email protected]> > --- > Hello, > > So, the only thing inconsistent there was the extra restriction when > enabling threaded mode when all the necessary conditions are already > checked by when verifying the parent's domain. > > Thanks. > > Documentation/cgroup-v2.txt | 5 +++-- > kernel/cgroup/cgroup.c | 7 ------- > 2 files changed, 3 insertions(+), 9 deletions(-) > > --- a/Documentation/cgroup-v2.txt > +++ b/Documentation/cgroup-v2.txt > @@ -274,8 +274,9 @@ thread mode, the following conditions mu > - As the cgroup will join the parent's resource domain. The parent > must either be a valid (threaded) domain or a threaded cgroup. > > -- The cgroup must be empty. No enabled controllers, child cgroups or > - processes. > +- When the parent is an unthreaded domain, it must not have any domain > + controllers enabled or populated domain children. The root is > + exempt from this requirement. > > Topology-wise, a cgroup can be in an invalid state. Please consider > the following toplogy:: > --- a/kernel/cgroup/cgroup.c > +++ b/kernel/cgroup/cgroup.c > @@ -3147,13 +3147,6 @@ static int cgroup_enable_threaded(struct > return -EOPNOTSUPP; > > /* > - * Allow enabling thread mode only on empty cgroups to avoid > - * implicit migrations and recursive operations. > - */ > - if (cgroup_has_tasks(cgrp) || css_has_online_children(&cgrp->self)) > - return -EBUSY; > - > - /* > * The following shouldn't cause actual migrations and should > * always succeed. > */
Acked-by: Waiman Long <[email protected]> While you are at it, the one comment that I have with cgroup_enable_threaded() is that it does not check to see if the cgroup parent exists. I thought for a while the first time I saw it, but then realized that cgroup.type wasn't show in root. I think a comment about this will make the code less puzzling to causal reader. Cheers, Longman
