| Message ID | 20210512201946.2949351-1-shakeelb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cgroup: disable controllers at parse time | expand |
On Wed, May 12, 2021 at 1:19 PM Shakeel Butt <shakeelb@google.com> wrote: > > This patch effectively reverts the commit a3e72739b7a7 ("cgroup: fix > too early usage of static_branch_disable()"). The commit 6041186a3258 > ("init: initialize jump labels before command line option parsing") has > moved the jump_label_init() before parse_args() which has made the > commit a3e72739b7a7 unnecessary. On the other hand there are > consequences of disabling the controllers later as there are subsystems > doing the controller checks for different decisions. One such incident > is reported [1] regarding the memory controller and its impact on memory > reclaim code. > > [1] https://lore.kernel.org/linux-mm/921e53f3-4b13-aab8-4a9e-e83ff15371e4@nec.com > > Signed-off-by: Shakeel Butt <shakeelb@google.com> > Reported-by: NOMURA JUNICHI(野村 淳一) <junichi.nomura@nec.com> Nomura, I think you have already tested this patch, so, can you please add your tested-by tag? Tejun, any comments or concerns? Yang, do you think we should add Fixes tag to make sure this patch lands in 5.13 where your shrinker patches landed? > --- > kernel/cgroup/cgroup.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > index e049edd66776..e7a9a2998245 100644 > --- a/kernel/cgroup/cgroup.c > +++ b/kernel/cgroup/cgroup.c > @@ -5634,8 +5634,6 @@ int __init cgroup_init_early(void) > return 0; > } > > -static u16 cgroup_disable_mask __initdata; > - > /** > * cgroup_init - cgroup initialization > * > @@ -5694,12 +5692,8 @@ int __init cgroup_init(void) > * disabled flag and cftype registration needs kmalloc, > * both of which aren't available during early_init. > */ > - if (cgroup_disable_mask & (1 << ssid)) { > - static_branch_disable(cgroup_subsys_enabled_key[ssid]); > - printk(KERN_INFO "Disabling %s control group subsystem\n", > - ss->name); > + if (!cgroup_ssid_enabled(ssid)) > continue; > - } > > if (cgroup1_ssid_disabled(ssid)) > printk(KERN_INFO "Disabling %s control group subsystem in v1 mounts\n", > @@ -6214,7 +6208,10 @@ static int __init cgroup_disable(char *str) > if (strcmp(token, ss->name) && > strcmp(token, ss->legacy_name)) > continue; > - cgroup_disable_mask |= 1 << i; > + > + static_branch_disable(cgroup_subsys_enabled_key[i]); > + pr_info("Disabling %s control group subsystem\n", > + ss->name); > } > } > return 1; > -- > 2.31.1.607.g51e8a6a459-goog >
On Tue, May 18, 2021 at 11:13 AM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, May 12, 2021 at 1:19 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > This patch effectively reverts the commit a3e72739b7a7 ("cgroup: fix > > too early usage of static_branch_disable()"). The commit 6041186a3258 > > ("init: initialize jump labels before command line option parsing") has > > moved the jump_label_init() before parse_args() which has made the > > commit a3e72739b7a7 unnecessary. On the other hand there are > > consequences of disabling the controllers later as there are subsystems > > doing the controller checks for different decisions. One such incident > > is reported [1] regarding the memory controller and its impact on memory > > reclaim code. > > > > [1] https://lore.kernel.org/linux-mm/921e53f3-4b13-aab8-4a9e-e83ff15371e4@nec.com > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com> > > Reported-by: NOMURA JUNICHI(野村 淳一) <junichi.nomura@nec.com> > > Nomura, I think you have already tested this patch, so, can you please > add your tested-by tag? > > Tejun, any comments or concerns? > > Yang, do you think we should add Fixes tag to make sure this patch > lands in 5.13 where your shrinker patches landed? Yes, I think we should, please do that. > > > --- > > kernel/cgroup/cgroup.c | 13 +++++-------- > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > > index e049edd66776..e7a9a2998245 100644 > > --- a/kernel/cgroup/cgroup.c > > +++ b/kernel/cgroup/cgroup.c > > @@ -5634,8 +5634,6 @@ int __init cgroup_init_early(void) > > return 0; > > } > > > > -static u16 cgroup_disable_mask __initdata; > > - > > /** > > * cgroup_init - cgroup initialization > > * > > @@ -5694,12 +5692,8 @@ int __init cgroup_init(void) > > * disabled flag and cftype registration needs kmalloc, > > * both of which aren't available during early_init. > > */ > > - if (cgroup_disable_mask & (1 << ssid)) { > > - static_branch_disable(cgroup_subsys_enabled_key[ssid]); > > - printk(KERN_INFO "Disabling %s control group subsystem\n", > > - ss->name); > > + if (!cgroup_ssid_enabled(ssid)) > > continue; > > - } > > > > if (cgroup1_ssid_disabled(ssid)) > > printk(KERN_INFO "Disabling %s control group subsystem in v1 mounts\n", > > @@ -6214,7 +6208,10 @@ static int __init cgroup_disable(char *str) > > if (strcmp(token, ss->name) && > > strcmp(token, ss->legacy_name)) > > continue; > > - cgroup_disable_mask |= 1 << i; > > + > > + static_branch_disable(cgroup_subsys_enabled_key[i]); > > + pr_info("Disabling %s control group subsystem\n", > > + ss->name); > > } > > } > > return 1; > > -- > > 2.31.1.607.g51e8a6a459-goog > >
On 2021/05/19 4:54, Yang Shi wrote: > On Tue, May 18, 2021 at 11:13 AM Shakeel Butt <shakeelb@google.com> wrote: >> >> On Wed, May 12, 2021 at 1:19 PM Shakeel Butt <shakeelb@google.com> wrote: >>> >>> This patch effectively reverts the commit a3e72739b7a7 ("cgroup: fix >>> too early usage of static_branch_disable()"). The commit 6041186a3258 >>> ("init: initialize jump labels before command line option parsing") has >>> moved the jump_label_init() before parse_args() which has made the >>> commit a3e72739b7a7 unnecessary. On the other hand there are >>> consequences of disabling the controllers later as there are subsystems >>> doing the controller checks for different decisions. One such incident >>> is reported [1] regarding the memory controller and its impact on memory >>> reclaim code. >>> >>> [1] https://lore.kernel.org/linux-mm/921e53f3-4b13-aab8-4a9e-e83ff15371e4@nec.com >>> >>> Signed-off-by: Shakeel Butt <shakeelb@google.com> >>> Reported-by: NOMURA JUNICHI(野村 淳一) <junichi.nomura@nec.com> >> >> Nomura, I think you have already tested this patch, so, can you please >> add your tested-by tag? Sure, I have confirmed the problem still occurs with v5.13-rc2 and it disappeared with your patch. So, Tested-by: Jun'ichi Nomura <junichi.nomura@nec.com>
On Wed, May 12, 2021 at 01:19:46PM -0700, Shakeel Butt wrote: > This patch effectively reverts the commit a3e72739b7a7 ("cgroup: fix > too early usage of static_branch_disable()"). The commit 6041186a3258 > ("init: initialize jump labels before command line option parsing") has > moved the jump_label_init() before parse_args() which has made the > commit a3e72739b7a7 unnecessary. On the other hand there are > consequences of disabling the controllers later as there are subsystems > doing the controller checks for different decisions. One such incident > is reported [1] regarding the memory controller and its impact on memory > reclaim code. > > [1] https://lore.kernel.org/linux-mm/921e53f3-4b13-aab8-4a9e-e83ff15371e4@nec.com > > Signed-off-by: Shakeel Butt <shakeelb@google.com> > Reported-by: NOMURA JUNICHI(野村 淳一) <junichi.nomura@nec.com> Applied to cgroup/for-5.13-fixes. Thanks.
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index e049edd66776..e7a9a2998245 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -5634,8 +5634,6 @@ int __init cgroup_init_early(void) return 0; } -static u16 cgroup_disable_mask __initdata; - /** * cgroup_init - cgroup initialization * @@ -5694,12 +5692,8 @@ int __init cgroup_init(void) * disabled flag and cftype registration needs kmalloc, * both of which aren't available during early_init. */ - if (cgroup_disable_mask & (1 << ssid)) { - static_branch_disable(cgroup_subsys_enabled_key[ssid]); - printk(KERN_INFO "Disabling %s control group subsystem\n", - ss->name); + if (!cgroup_ssid_enabled(ssid)) continue; - } if (cgroup1_ssid_disabled(ssid)) printk(KERN_INFO "Disabling %s control group subsystem in v1 mounts\n", @@ -6214,7 +6208,10 @@ static int __init cgroup_disable(char *str) if (strcmp(token, ss->name) && strcmp(token, ss->legacy_name)) continue; - cgroup_disable_mask |= 1 << i; + + static_branch_disable(cgroup_subsys_enabled_key[i]); + pr_info("Disabling %s control group subsystem\n", + ss->name); } } return 1;
This patch effectively reverts the commit a3e72739b7a7 ("cgroup: fix too early usage of static_branch_disable()"). The commit 6041186a3258 ("init: initialize jump labels before command line option parsing") has moved the jump_label_init() before parse_args() which has made the commit a3e72739b7a7 unnecessary. On the other hand there are consequences of disabling the controllers later as there are subsystems doing the controller checks for different decisions. One such incident is reported [1] regarding the memory controller and its impact on memory reclaim code. [1] https://lore.kernel.org/linux-mm/921e53f3-4b13-aab8-4a9e-e83ff15371e4@nec.com Signed-off-by: Shakeel Butt <shakeelb@google.com> Reported-by: NOMURA JUNICHI(野村 淳一) <junichi.nomura@nec.com> --- kernel/cgroup/cgroup.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)