Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
Hi Michal, On 2021/1/15 18:08, Michal Koutný wrote: > On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou > wrote: >> Yeah, this will select all enabled controllers, but which doesn't the >> behavior we want. > I see what the issue is now. > >> See above. Just the mount behavior isn't what we what. > I agree this a bug and your I find your approach correct > > Reviewed-by: Michal Koutný I have sent the v3, which updates the description and add Fixes. [v3]: https://lore.kernel.org/patchwork/patch/1365535/ If it is ok for you to add Reviewed-by there. Thanks, Chen Zhou > >> The behavior was changed since commit f5dfb5315d34 ("cgroup: take >> options parsing into ->parse_monolithic()"), will add this as Fixes. > Thanks. > > Michal
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou wrote: > Yeah, this will select all enabled controllers, but which doesn't the > behavior we want. I see what the issue is now. > See above. Just the mount behavior isn't what we what. I agree this a bug and your I find your approach correct Reviewed-by: Michal Koutný > The behavior was changed since commit f5dfb5315d34 ("cgroup: take > options parsing into ->parse_monolithic()"), will add this as Fixes. Thanks. Michal signature.asc Description: Digital signature
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
On 2021/1/15 11:17, Tejun Heo wrote: > Hello, > > On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou wrote: >> Yeah, this will select all enabled controllers, but which doesn't the >> behavior we want. >> I think the case should return error with information "Disabled controller >> xx" rather than >> attaching all the other enabled controllers. >> >> For example, boot with cgroup_no_v1=cpu, and then mount with >> "mount -t cgroup -o cpu cpu /sys/fs/cgroup/cpu", then all enabled >> controllers will >> be attached expect cpu. > Okay, that explanation actually makes sense. Can you please update the > description to include what's broken and how it's being fixed? It really > isn't clear what the patch is trying to achieve from the current > description. Ok, will update the description. > > Thanks. >
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
Hello, On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou wrote: > Yeah, this will select all enabled controllers, but which doesn't the > behavior we want. > I think the case should return error with information "Disabled controller > xx" rather than > attaching all the other enabled controllers. > > For example, boot with cgroup_no_v1=cpu, and then mount with > "mount -t cgroup -o cpu cpu /sys/fs/cgroup/cpu", then all enabled controllers > will > be attached expect cpu. Okay, that explanation actually makes sense. Can you please update the description to include what's broken and how it's being fixed? It really isn't clear what the patch is trying to achieve from the current description. Thanks. -- tejun
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
On 2021/1/15 0:54, Michal Koutný wrote: > On Thu, Jan 14, 2021 at 10:08:19PM +0800, chenzhou > wrote: >> In this case, at the beginning of function check_cgroupfs_options(), the mask >> ctx->subsys_mask will be 0. And if we mount without 'none' and 'name=' >> options, >> then in check_cgroupfs_options(), the flag ctx->all_ss will be set, that is, >> select all the subsystems. > But even then, the line > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cgroup/cgroup-v1.c?h=v5.11-rc3#n1012 > would select only 'enabled' controllers, wouldn't it? Yeah, this will select all enabled controllers, but which doesn't the behavior we want. I think the case should return error with information "Disabled controller xx" rather than attaching all the other enabled controllers. For example, boot with cgroup_no_v1=cpu, and then mount with "mount -t cgroup -o cpu cpu /sys/fs/cgroup/cpu", then all enabled controllers will be attached expect cpu. > > It's possible I missed something but if this means that cgroup_no_v1= > doesn't hold to its expectations, I'd suggest adding proper Fixes: tag > to the patch. See above. Just the mount behavior isn't what we what. The behavior was changed since commit f5dfb5315d34 ("cgroup: take options parsing into ->parse_monolithic()"), will add this as Fixes. Thanks, Chen Zhou > > Thanks, > Michal
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
On Thu, Jan 14, 2021 at 10:08:19PM +0800, chenzhou wrote: > In this case, at the beginning of function check_cgroupfs_options(), the mask > ctx->subsys_mask will be 0. And if we mount without 'none' and 'name=' > options, > then in check_cgroupfs_options(), the flag ctx->all_ss will be set, that is, > select all the subsystems. But even then, the line https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cgroup/cgroup-v1.c?h=v5.11-rc3#n1012 would select only 'enabled' controllers, wouldn't it? It's possible I missed something but if this means that cgroup_no_v1= doesn't hold to its expectations, I'd suggest adding proper Fixes: tag to the patch. Thanks, Michal signature.asc Description: Digital signature
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
Hi Michal, On 2021/1/14 21:12, Michal Koutný wrote: > Hello Chen. > > On Fri, Dec 18, 2020 at 02:17:55PM +0800, Chen Zhou > wrote: >> When mounting a cgroup hierarchy with disabled controller in cgroup v1, >> all available controllers will be attached. > Not sure if I understand the situation -- have you observed a v1 > controller attached to a hierarchy while specifying cgroup_no_v1= kernel > cmdline arg? Yeah, this is the situation. In this case, at the beginning of function check_cgroupfs_options(), the mask ctx->subsys_mask will be 0. And if we mount without 'none' and 'name=' options, then in check_cgroupfs_options(), the flag ctx->all_ss will be set, that is, select all the subsystems. Thanks, Chen Zhou > > AFAICS, the disabled controllers are honored thanks to > check_cgroupfs_options(). > > Michal
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
Hello Chen. On Fri, Dec 18, 2020 at 02:17:55PM +0800, Chen Zhou wrote: > When mounting a cgroup hierarchy with disabled controller in cgroup v1, > all available controllers will be attached. Not sure if I understand the situation -- have you observed a v1 controller attached to a hierarchy while specifying cgroup_no_v1= kernel cmdline arg? AFAICS, the disabled controllers are honored thanks to check_cgroupfs_options(). Michal signature.asc Description: Digital signature
Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
On 2020/12/18 14:17, Chen Zhou wrote: > When mounting a cgroup hierarchy with disabled controller in cgroup v1, > all available controllers will be attached. > > Add disabled controller check in cgroup1_parse_param() and return directly > if the specified controller is disabled. > > Signed-off-by: Chen Zhou > --- > Changes in v2: > - Fix line over 80 characters warning. > --- > kernel/cgroup/cgroup-v1.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c > index 191c329e482a..5190c42fea8b 100644 > --- a/kernel/cgroup/cgroup-v1.c > +++ b/kernel/cgroup/cgroup-v1.c > @@ -915,6 +915,9 @@ int cgroup1_parse_param(struct fs_context *fc, struct > fs_parameter *param) > for_each_subsys(ss, i) { > if (strcmp(param->key, ss->legacy_name)) > continue; > + if (!cgroup_ssid_enabled(i) || cgroup1_ssid_disabled(i)) > + return invalfc(fc, "Disabled controller '%s'", > +param->key); > ctx->subsys_mask |= (1 << i); > return 0; > } Reviewed-by: Zefan Li
[PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()
When mounting a cgroup hierarchy with disabled controller in cgroup v1, all available controllers will be attached. Add disabled controller check in cgroup1_parse_param() and return directly if the specified controller is disabled. Signed-off-by: Chen Zhou --- Changes in v2: - Fix line over 80 characters warning. --- kernel/cgroup/cgroup-v1.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c index 191c329e482a..5190c42fea8b 100644 --- a/kernel/cgroup/cgroup-v1.c +++ b/kernel/cgroup/cgroup-v1.c @@ -915,6 +915,9 @@ int cgroup1_parse_param(struct fs_context *fc, struct fs_parameter *param) for_each_subsys(ss, i) { if (strcmp(param->key, ss->legacy_name)) continue; + if (!cgroup_ssid_enabled(i) || cgroup1_ssid_disabled(i)) + return invalfc(fc, "Disabled controller '%s'", + param->key); ctx->subsys_mask |= (1 << i); return 0; } -- 2.20.1