[v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

[PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by T.J. Mercier 8 months, 1 week ago

Android has mounted the v1 cpuset controller using filesystem type
"cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
behavior where the controller name is not added as a prefix for cgroupfs
files. [2]

Later, a problem was discovered where cpu hotplug onlining did not
affect the cpuset/cpus files, which Android carried an out-of-tree patch
to address for a while. An attempt was made to upstream this patch, but
the recommendation was to use the "cpuset_v2_mode" mount option
instead. [3]

An effort was made to do so, but this fails with "cgroup: Unknown
parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
update the special cased cpuset_mount(), and only the cgroup (v1)
filesystem type was updated.

Add parameter parsing to the cpuset filesystem type so that
cpuset_v2_mode works like the cgroup filesystem type:

$ mkdir /dev/cpuset
$ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
$ mount|grep cpuset
none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)

[1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
[2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
[3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/

Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 27f08aa17b56..cf30ff2e7d60 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
 };
 
 #ifdef CONFIG_CPUSETS_V1
+enum cpuset_param {
+	Opt_cpuset_v2_mode,
+};
+
+const struct fs_parameter_spec cpuset_fs_parameters[] = {
+	fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
+	{}
+};
+
+static int cpuset_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+	struct cgroup_fs_context *ctx = cgroup_fc2context(fc);
+	struct fs_parse_result result;
+	int opt;
+
+	opt = fs_parse(fc, cpuset_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
+	case Opt_cpuset_v2_mode:
+		ctx->flags |= CGRP_ROOT_CPUSET_V2_MODE;
+		return 0;
+	}
+	return -EINVAL;
+}
+
 static const struct fs_context_operations cpuset_fs_context_ops = {
 	.get_tree	= cgroup1_get_tree,
 	.free		= cgroup_fs_context_free,
+	.parse_param	= cpuset_parse_param,
 };
 
 /*
@@ -2392,6 +2420,7 @@ static int cpuset_init_fs_context(struct fs_context *fc)
 static struct file_system_type cpuset_fs_type = {
 	.name			= "cpuset",
 	.init_fs_context	= cpuset_init_fs_context,
+	.parameters		= cpuset_fs_parameters,
 	.fs_flags		= FS_USERNS_MOUNT,
 };
 #endif
-- 
2.49.0.777.g153de2bbd5-goog

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by Michal Koutný 8 months, 1 week ago

On Tue, Apr 15, 2025 at 11:53:07PM +0000, "T.J. Mercier" <tjmercier@google.com> wrote:
...
>  kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Acked-by: Michal Koutný <mkoutny@suse.com>

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by Kamalesh Babulal 8 months, 1 week ago

Hi,

On 4/16/25 5:23 AM, T.J. Mercier wrote:
> Android has mounted the v1 cpuset controller using filesystem type
> "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
> behavior where the controller name is not added as a prefix for cgroupfs
> files. [2]
> 
> Later, a problem was discovered where cpu hotplug onlining did not
> affect the cpuset/cpus files, which Android carried an out-of-tree patch
> to address for a while. An attempt was made to upstream this patch, but
> the recommendation was to use the "cpuset_v2_mode" mount option
> instead. [3]
> 
> An effort was made to do so, but this fails with "cgroup: Unknown
> parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
> mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
> update the special cased cpuset_mount(), and only the cgroup (v1)
> filesystem type was updated.
> 
> Add parameter parsing to the cpuset filesystem type so that
> cpuset_v2_mode works like the cgroup filesystem type:
> 
> $ mkdir /dev/cpuset
> $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
> $ mount|grep cpuset
> none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
> 
> [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
> [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
> [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
> 
> Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
> Signed-off-by: T.J. Mercier <tjmercier@google.com>

The patch looks good to me, please feel free to add

Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>

One nit below:

> ---
>  kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 27f08aa17b56..cf30ff2e7d60 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
>  };
>  
>  #ifdef CONFIG_CPUSETS_V1
> +enum cpuset_param {
> +	Opt_cpuset_v2_mode,
> +};
> +
> +const struct fs_parameter_spec cpuset_fs_parameters[] = {
> +	fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
> +	{}
> +};

A minor optimization you may want to convert the cpuset_fs_parameters into
a static const.

-- 
Cheers,
Kamalesh

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by T.J. Mercier 8 months ago

On Wed, Apr 16, 2025 at 2:19 AM Kamalesh Babulal
<kamalesh.babulal@oracle.com> wrote:
>
> Hi,
>
> On 4/16/25 5:23 AM, T.J. Mercier wrote:
> > Android has mounted the v1 cpuset controller using filesystem type
> > "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
> > behavior where the controller name is not added as a prefix for cgroupfs
> > files. [2]
> >
> > Later, a problem was discovered where cpu hotplug onlining did not
> > affect the cpuset/cpus files, which Android carried an out-of-tree patch
> > to address for a while. An attempt was made to upstream this patch, but
> > the recommendation was to use the "cpuset_v2_mode" mount option
> > instead. [3]
> >
> > An effort was made to do so, but this fails with "cgroup: Unknown
> > parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
> > mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
> > update the special cased cpuset_mount(), and only the cgroup (v1)
> > filesystem type was updated.
> >
> > Add parameter parsing to the cpuset filesystem type so that
> > cpuset_v2_mode works like the cgroup filesystem type:
> >
> > $ mkdir /dev/cpuset
> > $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
> > $ mount|grep cpuset
> > none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
> >
> > [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
> > [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
> > [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
> >
> > Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
>
> The patch looks good to me, please feel free to add
>
> Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
>
> One nit below:
>
> > ---
> >  kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index 27f08aa17b56..cf30ff2e7d60 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
> >  };
> >
> >  #ifdef CONFIG_CPUSETS_V1
> > +enum cpuset_param {
> > +     Opt_cpuset_v2_mode,
> > +};
> > +
> > +const struct fs_parameter_spec cpuset_fs_parameters[] = {
> > +     fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
> > +     {}
> > +};
>
> A minor optimization you may want to convert the cpuset_fs_parameters into
> a static const.

Thanks, I copied from cgroup1_fs_parameters since that's where
cpuset_v2_mode lives, which doesn't have the static currently
(cgroup2_fs_parameters does). Let me update cpuset_fs_parameters in
v3, and add a second patch for cgroup1_fs_parameters.

> --
> Cheers,
> Kamalesh
>

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by Waiman Long 8 months ago

On 4/16/25 1:55 PM, T.J. Mercier wrote:
> On Wed, Apr 16, 2025 at 2:19 AM Kamalesh Babulal
> <kamalesh.babulal@oracle.com> wrote:
>> Hi,
>>
>> On 4/16/25 5:23 AM, T.J. Mercier wrote:
>>> Android has mounted the v1 cpuset controller using filesystem type
>>> "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
>>> behavior where the controller name is not added as a prefix for cgroupfs
>>> files. [2]
>>>
>>> Later, a problem was discovered where cpu hotplug onlining did not
>>> affect the cpuset/cpus files, which Android carried an out-of-tree patch
>>> to address for a while. An attempt was made to upstream this patch, but
>>> the recommendation was to use the "cpuset_v2_mode" mount option
>>> instead. [3]
>>>
>>> An effort was made to do so, but this fails with "cgroup: Unknown
>>> parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
>>> mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
>>> update the special cased cpuset_mount(), and only the cgroup (v1)
>>> filesystem type was updated.
>>>
>>> Add parameter parsing to the cpuset filesystem type so that
>>> cpuset_v2_mode works like the cgroup filesystem type:
>>>
>>> $ mkdir /dev/cpuset
>>> $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
>>> $ mount|grep cpuset
>>> none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
>>>
>>> [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
>>> [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
>>> [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
>>>
>>> Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
>>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
>> The patch looks good to me, please feel free to add
>>
>> Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
>>
>> One nit below:
>>
>>> ---
>>>   kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>>> index 27f08aa17b56..cf30ff2e7d60 100644
>>> --- a/kernel/cgroup/cgroup.c
>>> +++ b/kernel/cgroup/cgroup.c
>>> @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
>>>   };
>>>
>>>   #ifdef CONFIG_CPUSETS_V1
>>> +enum cpuset_param {
>>> +     Opt_cpuset_v2_mode,
>>> +};
>>> +
>>> +const struct fs_parameter_spec cpuset_fs_parameters[] = {
>>> +     fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
>>> +     {}
>>> +};
>> A minor optimization you may want to convert the cpuset_fs_parameters into
>> a static const.
> Thanks, I copied from cgroup1_fs_parameters since that's where
> cpuset_v2_mode lives, which doesn't have the static currently
> (cgroup2_fs_parameters does). Let me update cpuset_fs_parameters in
> v3, and add a second patch for cgroup1_fs_parameters.

Besides not exposing the structure outside the current file or maybe a 
tiny bit of linker speedup, is there other performance benefit by adding 
"static"?

Regards,
Longman

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by T.J. Mercier 8 months ago

On Wed, Apr 16, 2025 at 11:05 AM Waiman Long <llong@redhat.com> wrote:
>
> On 4/16/25 1:55 PM, T.J. Mercier wrote:
> > On Wed, Apr 16, 2025 at 2:19 AM Kamalesh Babulal
> > <kamalesh.babulal@oracle.com> wrote:
> >> Hi,
> >>
> >> On 4/16/25 5:23 AM, T.J. Mercier wrote:
> >>> Android has mounted the v1 cpuset controller using filesystem type
> >>> "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
> >>> behavior where the controller name is not added as a prefix for cgroupfs
> >>> files. [2]
> >>>
> >>> Later, a problem was discovered where cpu hotplug onlining did not
> >>> affect the cpuset/cpus files, which Android carried an out-of-tree patch
> >>> to address for a while. An attempt was made to upstream this patch, but
> >>> the recommendation was to use the "cpuset_v2_mode" mount option
> >>> instead. [3]
> >>>
> >>> An effort was made to do so, but this fails with "cgroup: Unknown
> >>> parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
> >>> mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
> >>> update the special cased cpuset_mount(), and only the cgroup (v1)
> >>> filesystem type was updated.
> >>>
> >>> Add parameter parsing to the cpuset filesystem type so that
> >>> cpuset_v2_mode works like the cgroup filesystem type:
> >>>
> >>> $ mkdir /dev/cpuset
> >>> $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
> >>> $ mount|grep cpuset
> >>> none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
> >>>
> >>> [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
> >>> [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
> >>> [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
> >>>
> >>> Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
> >>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> >> The patch looks good to me, please feel free to add
> >>
> >> Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
> >>
> >> One nit below:
> >>
> >>> ---
> >>>   kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
> >>>   1 file changed, 29 insertions(+)
> >>>
> >>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> >>> index 27f08aa17b56..cf30ff2e7d60 100644
> >>> --- a/kernel/cgroup/cgroup.c
> >>> +++ b/kernel/cgroup/cgroup.c
> >>> @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
> >>>   };
> >>>
> >>>   #ifdef CONFIG_CPUSETS_V1
> >>> +enum cpuset_param {
> >>> +     Opt_cpuset_v2_mode,
> >>> +};
> >>> +
> >>> +const struct fs_parameter_spec cpuset_fs_parameters[] = {
> >>> +     fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
> >>> +     {}
> >>> +};
> >> A minor optimization you may want to convert the cpuset_fs_parameters into
> >> a static const.
> > Thanks, I copied from cgroup1_fs_parameters since that's where
> > cpuset_v2_mode lives, which doesn't have the static currently
> > (cgroup2_fs_parameters does). Let me update cpuset_fs_parameters in
> > v3, and add a second patch for cgroup1_fs_parameters.
>
> Besides not exposing the structure outside the current file or maybe a
> tiny bit of linker speedup, is there other performance benefit by adding
> "static"?
>
> Regards,
> Longman
>

I thought it might decrease the text size a tiny bit, but it doesn't
because the symbol isn't exported and I guess the compiler knows to
just inline.

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by Waiman Long 8 months ago

On 4/16/25 2:27 PM, T.J. Mercier wrote:
> On Wed, Apr 16, 2025 at 11:05 AM Waiman Long <llong@redhat.com> wrote:
>> On 4/16/25 1:55 PM, T.J. Mercier wrote:
>>> On Wed, Apr 16, 2025 at 2:19 AM Kamalesh Babulal
>>> <kamalesh.babulal@oracle.com> wrote:
>>>> Hi,
>>>>
>>>> On 4/16/25 5:23 AM, T.J. Mercier wrote:
>>>>> Android has mounted the v1 cpuset controller using filesystem type
>>>>> "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
>>>>> behavior where the controller name is not added as a prefix for cgroupfs
>>>>> files. [2]
>>>>>
>>>>> Later, a problem was discovered where cpu hotplug onlining did not
>>>>> affect the cpuset/cpus files, which Android carried an out-of-tree patch
>>>>> to address for a while. An attempt was made to upstream this patch, but
>>>>> the recommendation was to use the "cpuset_v2_mode" mount option
>>>>> instead. [3]
>>>>>
>>>>> An effort was made to do so, but this fails with "cgroup: Unknown
>>>>> parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
>>>>> mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
>>>>> update the special cased cpuset_mount(), and only the cgroup (v1)
>>>>> filesystem type was updated.
>>>>>
>>>>> Add parameter parsing to the cpuset filesystem type so that
>>>>> cpuset_v2_mode works like the cgroup filesystem type:
>>>>>
>>>>> $ mkdir /dev/cpuset
>>>>> $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
>>>>> $ mount|grep cpuset
>>>>> none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
>>>>>
>>>>> [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
>>>>> [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
>>>>> [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
>>>>>
>>>>> Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
>>>>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
>>>> The patch looks good to me, please feel free to add
>>>>
>>>> Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
>>>>
>>>> One nit below:
>>>>
>>>>> ---
>>>>>    kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
>>>>>    1 file changed, 29 insertions(+)
>>>>>
>>>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>>>>> index 27f08aa17b56..cf30ff2e7d60 100644
>>>>> --- a/kernel/cgroup/cgroup.c
>>>>> +++ b/kernel/cgroup/cgroup.c
>>>>> @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
>>>>>    };
>>>>>
>>>>>    #ifdef CONFIG_CPUSETS_V1
>>>>> +enum cpuset_param {
>>>>> +     Opt_cpuset_v2_mode,
>>>>> +};
>>>>> +
>>>>> +const struct fs_parameter_spec cpuset_fs_parameters[] = {
>>>>> +     fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
>>>>> +     {}
>>>>> +};
>>>> A minor optimization you may want to convert the cpuset_fs_parameters into
>>>> a static const.
>>> Thanks, I copied from cgroup1_fs_parameters since that's where
>>> cpuset_v2_mode lives, which doesn't have the static currently
>>> (cgroup2_fs_parameters does). Let me update cpuset_fs_parameters in
>>> v3, and add a second patch for cgroup1_fs_parameters.
>> Besides not exposing the structure outside the current file or maybe a
>> tiny bit of linker speedup, is there other performance benefit by adding
>> "static"?
>>
>> Regards,
>> Longman
>>
> I thought it might decrease the text size a tiny bit, but it doesn't
> because the symbol isn't exported and I guess the compiler knows to
> just inline.
>
Since the structure already have a "const" modifier, I doubt there is 
any further optimization that the compiler can do whether the symbol is 
visible externally or not. Anyway, I am not objecting to v3 with static 
modifier added.

Cheers,
Longman

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by T.J. Mercier 8 months ago

On Wed, Apr 16, 2025 at 10:55 AM T.J. Mercier <tjmercier@google.com> wrote:
>
> On Wed, Apr 16, 2025 at 2:19 AM Kamalesh Babulal
> <kamalesh.babulal@oracle.com> wrote:
> >
> > Hi,
> >
> > On 4/16/25 5:23 AM, T.J. Mercier wrote:
> > > Android has mounted the v1 cpuset controller using filesystem type
> > > "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
> > > behavior where the controller name is not added as a prefix for cgroupfs
> > > files. [2]
> > >
> > > Later, a problem was discovered where cpu hotplug onlining did not
> > > affect the cpuset/cpus files, which Android carried an out-of-tree patch
> > > to address for a while. An attempt was made to upstream this patch, but
> > > the recommendation was to use the "cpuset_v2_mode" mount option
> > > instead. [3]
> > >
> > > An effort was made to do so, but this fails with "cgroup: Unknown
> > > parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
> > > mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
> > > update the special cased cpuset_mount(), and only the cgroup (v1)
> > > filesystem type was updated.
> > >
> > > Add parameter parsing to the cpuset filesystem type so that
> > > cpuset_v2_mode works like the cgroup filesystem type:
> > >
> > > $ mkdir /dev/cpuset
> > > $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
> > > $ mount|grep cpuset
> > > none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
> > >
> > > [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
> > > [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
> > > [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
> > >
> > > Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
> > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> >
> > The patch looks good to me, please feel free to add
> >
> > Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
> >
> > One nit below:
> >
> > > ---
> > >  kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > index 27f08aa17b56..cf30ff2e7d60 100644
> > > --- a/kernel/cgroup/cgroup.c
> > > +++ b/kernel/cgroup/cgroup.c
> > > @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
> > >  };
> > >
> > >  #ifdef CONFIG_CPUSETS_V1
> > > +enum cpuset_param {
> > > +     Opt_cpuset_v2_mode,
> > > +};
> > > +
> > > +const struct fs_parameter_spec cpuset_fs_parameters[] = {
> > > +     fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
> > > +     {}
> > > +};
> >
> > A minor optimization you may want to convert the cpuset_fs_parameters into
> > a static const.
>
> Thanks, I copied from cgroup1_fs_parameters since that's where
> cpuset_v2_mode lives, which doesn't have the static currently
> (cgroup2_fs_parameters does). Let me update cpuset_fs_parameters in
> v3, and add a second patch for cgroup1_fs_parameters.

Ah nevermind, cgroup1_fs_parameters needs to be accessible from
cgroup.c. So just the v3 update then.
>
> > --
> > Cheers,
> > Kamalesh
> >

Re: [PATCH v2] cgroup/cpuset-v1: Add missing support for cpuset_v2_mode

Posted by Waiman Long 8 months, 1 week ago

On 4/15/25 7:53 PM, T.J. Mercier wrote:
> Android has mounted the v1 cpuset controller using filesystem type
> "cpuset" (not "cgroup") since 2015 [1], and depends on the resulting
> behavior where the controller name is not added as a prefix for cgroupfs
> files. [2]
>
> Later, a problem was discovered where cpu hotplug onlining did not
> affect the cpuset/cpus files, which Android carried an out-of-tree patch
> to address for a while. An attempt was made to upstream this patch, but
> the recommendation was to use the "cpuset_v2_mode" mount option
> instead. [3]
>
> An effort was made to do so, but this fails with "cgroup: Unknown
> parameter 'cpuset_v2_mode'" because commit e1cba4b85daa ("cgroup: Add
> mount flag to enable cpuset to use v2 behavior in v1 cgroup") did not
> update the special cased cpuset_mount(), and only the cgroup (v1)
> filesystem type was updated.
>
> Add parameter parsing to the cpuset filesystem type so that
> cpuset_v2_mode works like the cgroup filesystem type:
>
> $ mkdir /dev/cpuset
> $ mount -t cpuset -ocpuset_v2_mode none /dev/cpuset
> $ mount|grep cpuset
> none on /dev/cpuset type cgroup (rw,relatime,cpuset,noprefix,cpuset_v2_mode,release_agent=/sbin/cpuset_release_agent)
>
> [1] https://cs.android.com/android/_/android/platform/system/core/+/b769c8d24fd7be96f8968aa4c80b669525b930d3
> [2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libprocessgroup/setup/cgroup_map_write.cpp;drc=2dac5d89a0f024a2d0cc46a80ba4ee13472f1681;l=192
> [3] https://lore.kernel.org/lkml/f795f8be-a184-408a-0b5a-553d26061385@redhat.com/T/
>
> Fixes: e1cba4b85daa ("cgroup: Add mount flag to enable cpuset to use v2 behavior in v1 cgroup")
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> ---
>   kernel/cgroup/cgroup.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 27f08aa17b56..cf30ff2e7d60 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2353,9 +2353,37 @@ static struct file_system_type cgroup2_fs_type = {
>   };
>   
>   #ifdef CONFIG_CPUSETS_V1
> +enum cpuset_param {
> +	Opt_cpuset_v2_mode,
> +};
> +
> +const struct fs_parameter_spec cpuset_fs_parameters[] = {
> +	fsparam_flag  ("cpuset_v2_mode", Opt_cpuset_v2_mode),
> +	{}
> +};
> +
> +static int cpuset_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +	struct cgroup_fs_context *ctx = cgroup_fc2context(fc);
> +	struct fs_parse_result result;
> +	int opt;
> +
> +	opt = fs_parse(fc, cpuset_fs_parameters, param, &result);
> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_cpuset_v2_mode:
> +		ctx->flags |= CGRP_ROOT_CPUSET_V2_MODE;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
>   static const struct fs_context_operations cpuset_fs_context_ops = {
>   	.get_tree	= cgroup1_get_tree,
>   	.free		= cgroup_fs_context_free,
> +	.parse_param	= cpuset_parse_param,
>   };
>   
>   /*
> @@ -2392,6 +2420,7 @@ static int cpuset_init_fs_context(struct fs_context *fc)
>   static struct file_system_type cpuset_fs_type = {
>   	.name			= "cpuset",
>   	.init_fs_context	= cpuset_init_fs_context,
> +	.parameters		= cpuset_fs_parameters,
>   	.fs_flags		= FS_USERNS_MOUNT,
>   };
>   #endif

LGTM

Acked-by: Waiman Long <longman@redhat.com>