diff mbox series

cgroup: In cgroup_no_v1() check v1 controllers only

Message ID 20230920102513.55772-1-kamalesh.babulal@oracle.com
State New
Headers
Series cgroup: In cgroup_no_v1() check v1 controllers only |

Commit Message

Kamalesh Babulal Sept. 20, 2023, 10:25 a.m. UTC 
  cgroup v1 or v2 or both controller names can be passed as arguments to
the 'cgroup_no_v1' kernel parameter, though most of the controller's
names are the same for both cgroup versions. This can be confusing when
both versions are used interchangeably, i.e., passing cgroup_no_v1=io

$ sudo dmesg |grep cgroup
...
cgroup: Disabling io control group subsystem in v1 mounts
cgroup: Disabled controller 'blkio'

Restrict the check to ss->legacy_name only, when parsing the controller
names passed to cgroup_no_v1 parameter and also, use ss->legacy_name in
pr_info(), that prints the controller disabling information.

Signed-off-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
---
 kernel/cgroup/cgroup-v1.c | 3 +--
 kernel/cgroup/cgroup.c    | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)


base-commit: 2cf0f715623872823a72e451243bbf555d10d032

Comments

Tejun Heo Oct. 4, 2023, 7:17 p.m. UTC | #1 
On Wed, Sep 20, 2023 at 03:55:12PM +0530, Kamalesh Babulal wrote:
> cgroup v1 or v2 or both controller names can be passed as arguments to
> the 'cgroup_no_v1' kernel parameter, though most of the controller's
> names are the same for both cgroup versions. This can be confusing when
> both versions are used interchangeably, i.e., passing cgroup_no_v1=io
> 
> $ sudo dmesg |grep cgroup
> ...
> cgroup: Disabling io control group subsystem in v1 mounts
> cgroup: Disabled controller 'blkio'

So, making the printed names consistent makes sense but I'm not sure about
not matching "io" anymore. That's gonna break users who already use them,
right?

Thanks.
Kamalesh Babulal Oct. 5, 2023, 11:21 a.m. UTC | #2 
On 10/5/23 00:47, Tejun Heo wrote:
> On Wed, Sep 20, 2023 at 03:55:12PM +0530, Kamalesh Babulal wrote:
>> cgroup v1 or v2 or both controller names can be passed as arguments to
>> the 'cgroup_no_v1' kernel parameter, though most of the controller's
>> names are the same for both cgroup versions. This can be confusing when
>> both versions are used interchangeably, i.e., passing cgroup_no_v1=io
>>
>> $ sudo dmesg |grep cgroup
>> ...
>> cgroup: Disabling io control group subsystem in v1 mounts
>> cgroup: Disabled controller 'blkio'
> 
> So, making the printed names consistent makes sense but I'm not sure about
> not matching "io" anymore. That's gonna break users who already use them,
> right?

Agreed, users might pass "io" in place of "blkio", I was thinking
in terms of cgroup v1 users, for whom it is "blkio" controller but
as an argument, "cgroup_no_v1" makes the controllers passed to it,
available under cgroup v2, where the controller is named "io",
which the user may use interchangeably.

Shall I re-send the patch to print the controller legacy_name only?
Tejun Heo Oct. 5, 2023, 6:12 p.m. UTC | #3 
On Thu, Oct 05, 2023 at 04:51:52PM +0530, Kamalesh Babulal wrote:
> 
> 
> On 10/5/23 00:47, Tejun Heo wrote:
> > On Wed, Sep 20, 2023 at 03:55:12PM +0530, Kamalesh Babulal wrote:
> >> cgroup v1 or v2 or both controller names can be passed as arguments to
> >> the 'cgroup_no_v1' kernel parameter, though most of the controller's
> >> names are the same for both cgroup versions. This can be confusing when
> >> both versions are used interchangeably, i.e., passing cgroup_no_v1=io
> >>
> >> $ sudo dmesg |grep cgroup
> >> ...
> >> cgroup: Disabling io control group subsystem in v1 mounts
> >> cgroup: Disabled controller 'blkio'
> > 
> > So, making the printed names consistent makes sense but I'm not sure about
> > not matching "io" anymore. That's gonna break users who already use them,
> > right?
> 
> Agreed, users might pass "io" in place of "blkio", I was thinking
> in terms of cgroup v1 users, for whom it is "blkio" controller but
> as an argument, "cgroup_no_v1" makes the controllers passed to it,
> available under cgroup v2, where the controller is named "io",
> which the user may use interchangeably.
> 
> Shall I re-send the patch to print the controller legacy_name only?

Yes, let's do that.

Thanks.
diff mbox series

Patch

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index c487ffef6652..942872b83bf2 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -1297,8 +1297,7 @@  static int __init cgroup_no_v1(char *str)
 		}
 
 		for_each_subsys(ss, i) {
-			if (strcmp(token, ss->name) &&
-			    strcmp(token, ss->legacy_name))
+			if (strcmp(token, ss->legacy_name))
 				continue;
 
 			cgroup_no_v1_mask |= 1 << i;
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1fb7f562289d..622fb53a806b 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6121,7 +6121,7 @@  int __init cgroup_init(void)
 
 		if (cgroup1_ssid_disabled(ssid))
 			pr_info("Disabling %s control group subsystem in v1 mounts\n",
-				ss->name);
+				ss->legacy_name);
 
 		cgrp_dfl_root.subsys_mask |= 1 << ss->id;