From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: Cgroup v2 bug: "domain invalid" node can't be converted to "threaded" Date: Thu, 4 Oct 2018 11:20:17 -0700 Message-ID: <20181004182017.GK270328@devbig004.ftw2.facebook.com> References: <20181002210741.GJ270328@devbig004.ftw2.facebook.com> <0cf4f9de-70d2-cf94-12d2-283759638e20@gmail.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9pskbhcFdCYR+O0197LXRxHX3xRpKTYxebBKC2f8iQg=; b=V2i6+QJobPY/bjEjCTvpU2v+tHjxiaslwIuJeCO4QRbFsQeV9Vf3fv9rSEvRBg7KJx j1b+LXGp+wTyo0TGTsxcQzXezM59pVWA7dkjQy5e+ZWQG/wsg42enBdcLel6xb0WnQJA ChdtIVYpGbCTD4AlsRuUeRlZrxOiLbfzd9ATakDrozkxKsCPPO1PDddjF/PtWKL0529P ql61Uij+ASr76FX9rX5nVFa7s2ScHJ06cdoaKcypphlLYOL2BMIm3kluFsEz94KoPECK 2KMYrnYtZb9L7lm6gbabKQRDcuCBNP/uxyHkvDlgCfitIWohJTfYTs/KYcHuebALYkv3 SMlQ== Content-Disposition: inline In-Reply-To: <0cf4f9de-70d2-cf94-12d2-283759638e20@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Michael Kerrisk (man-pages)" Cc: lkml , "open list:CONTROL GROUP (CGROUP)" , "Serge E. Hallyn" , Amin Jamali , Joao De Almeida Pereira Hello, On Thu, Oct 04, 2018 at 08:14:55PM +0200, Michael Kerrisk (man-pages) wrote: > On 10/02/2018 11:07 PM, Tejun Heo wrote: > > Hello, Michael. > > > > Great catch. Can you please verify whether the following patch fixes > > the issue? > > > > Thanks. > Against which kernel version should this apply? I get these build > errors on kernel 4.18: lol, sorry about that. The header change ended up in the wrong patch. Updated patch follows. Thanks. ------ 8< ------ Subject: cgroup: Fix dom_cgrp propagation when enabling threaded mode A cgroup which is already a threaded domain may be converted into a threaded cgroup if the prerequisite conditions are met. When this happens, all threaded descendant should also have their ->dom_cgrp updated to the new threaded domain cgroup. Unfortunately, this propagation was missing leading to the following failure. # cd /sys/fs/cgroup/unified # cat cgroup.subtree_control # show that no controllers are enabled # mkdir -p mycgrp/a/b/c # echo threaded > mycgrp/a/b/cgroup.type At this point, the hierarchy looks as follows: mycgrp [d] a [dt] b [t] c [inv] Now let's make node "a" threaded (and thus "mycgrp" s made "domain threaded"): # echo threaded > mycgrp/a/cgroup.type By this point, we now have a hierarchy that looks as follows: mycgrp [dt] a [t] b [t] c [inv] But, when we try to convert the node "c" from "domain invalid" to "threaded", we get ENOTSUP on the write(): # echo threaded > mycgrp/a/b/c/cgroup.type sh: echo: write error: Operation not supported This patch fixes the problem by * Moving the opencoded ->dom_cgrp save and restoration in cgroup_enable_threaded() into cgroup_{save|restore}_control() so that mulitple cgroups can be handled. * Updating all threaded descendants' ->dom_cgrp to point to the new dom_cgrp when enabling threaded mode. Signed-off-by: Tejun Heo Reported-by: "Michael Kerrisk (man-pages)" Link: https://lore.kernel.org/r/CAKgNAkhHYCMn74TCNiMJ=ccLd7DcmXSbvw3CbZ1YREeG7iJM5g@mail.gmail.com Fixes: 454000adaa2a ("cgroup: introduce cgroup->dom_cgrp and threaded css_set handling") Cc: stable@vger.kernel.org # v4.14+ --- include/linux/cgroup-defs.h | 1 + kernel/cgroup/cgroup.c | 25 ++++++++++++++++--------- 2 files changed, 17 insertions(+), 9 deletions(-) --- a/include/linux/cgroup-defs.h +++ b/include/linux/cgroup-defs.h @@ -412,6 +412,7 @@ struct cgroup { * specific task are charged to the dom_cgrp. */ struct cgroup *dom_cgrp; + struct cgroup *old_dom_cgrp; /* used while enabling threaded */ /* per-cpu recursive resource statistics */ struct cgroup_rstat_cpu __percpu *rstat_cpu; --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -2836,11 +2836,12 @@ restart: } /** - * cgroup_save_control - save control masks of a subtree + * cgroup_save_control - save control masks and dom_cgrp of a subtree * @cgrp: root of the target subtree * - * Save ->subtree_control and ->subtree_ss_mask to the respective old_ - * prefixed fields for @cgrp's subtree including @cgrp itself. + * Save ->subtree_control, ->subtree_ss_mask and ->dom_cgrp to the + * respective old_ prefixed fields for @cgrp's subtree including @cgrp + * itself. */ static void cgroup_save_control(struct cgroup *cgrp) { @@ -2850,6 +2851,7 @@ static void cgroup_save_control(struct c cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) { dsct->old_subtree_control = dsct->subtree_control; dsct->old_subtree_ss_mask = dsct->subtree_ss_mask; + dsct->old_dom_cgrp = dsct->dom_cgrp; } } @@ -2875,11 +2877,12 @@ static void cgroup_propagate_control(str } /** - * cgroup_restore_control - restore control masks of a subtree + * cgroup_restore_control - restore control masks and dom_cgrp of a subtree * @cgrp: root of the target subtree * - * Restore ->subtree_control and ->subtree_ss_mask from the respective old_ - * prefixed fields for @cgrp's subtree including @cgrp itself. + * Restore ->subtree_control, ->subtree_ss_mask and ->dom_cgrp from the + * respective old_ prefixed fields for @cgrp's subtree including @cgrp + * itself. */ static void cgroup_restore_control(struct cgroup *cgrp) { @@ -2889,6 +2892,7 @@ static void cgroup_restore_control(struc cgroup_for_each_live_descendant_post(dsct, d_css, cgrp) { dsct->subtree_control = dsct->old_subtree_control; dsct->subtree_ss_mask = dsct->old_subtree_ss_mask; + dsct->dom_cgrp = dsct->old_dom_cgrp; } } @@ -3196,6 +3200,8 @@ static int cgroup_enable_threaded(struct { struct cgroup *parent = cgroup_parent(cgrp); struct cgroup *dom_cgrp = parent->dom_cgrp; + struct cgroup *dsct; + struct cgroup_subsys_state *d_css; int ret; lockdep_assert_held(&cgroup_mutex); @@ -3225,12 +3231,13 @@ static int cgroup_enable_threaded(struct */ cgroup_save_control(cgrp); - cgrp->dom_cgrp = dom_cgrp; + cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) + if (dsct == cgrp || cgroup_is_threaded(dsct)) + dsct->dom_cgrp = dom_cgrp; + ret = cgroup_apply_control(cgrp); if (!ret) parent->nr_threaded_children++; - else - cgrp->dom_cgrp = cgrp; cgroup_finalize_control(cgrp, ret); return ret;