[PATH v2] cgroup: add cgroup_favordynmods= command-line option

[PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Luiz Capitulino]

We have a need of using favordynmods with cgroup v1, which doesn't support
changing mount flags during remount. Enabling CONFIG_FAVOR_DYNMODS at
build-time is not an option because we want to be able to selectively
enable it for certain systems.

This commit addresses this by introducing the cgroup_favordynmods=
command-line option. This option works for both cgroup v1 and v2 and
also allows for disabling favorynmods when the kernel built with
CONFIG_FAVOR_DYNMODS=y.

Signed-off-by: Luiz Capitulino <luizcap-vV1OtcyAfmbQT0dZR+AlfA@public.gmane.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 kernel/cgroup/cgroup.c                          | 14 +++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

o v2
 - Use __ro_after_init [Waiman]

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..8b744d39d393 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -580,6 +580,10 @@
 			named mounts. Specifying both "all" and "named" disables
 			all v1 hierarchies.
 
+	cgroup_favordynmods= [KNL] Enable or Disable favordynmods.
+			Format: { "true" | "false" }
+			Defaults to the value of CONFIG_CGROUP_FAVOR_DYNMODS.
+
 	cgroup.memory=	[KNL] Pass options to the cgroup memory controller.
 			Format: <string>
 			nosocket -- Disable socket memory accounting.
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1fb7f562289d..2b7d74304606 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -207,6 +207,8 @@ static u16 have_exit_callback __read_mostly;
 static u16 have_release_callback __read_mostly;
 static u16 have_canfork_callback __read_mostly;
 
+static bool have_favordynmods __ro_after_init = IS_ENABLED(CONFIG_CGROUP_FAVOR_DYNMODS);
+
 /* cgroup namespace for init task */
 struct cgroup_namespace init_cgroup_ns = {
 	.ns.count	= REFCOUNT_INIT(2),
@@ -2243,9 +2245,9 @@ static int cgroup_init_fs_context(struct fs_context *fc)
 	fc->user_ns = get_user_ns(ctx->ns->user_ns);
 	fc->global = true;
 
-#ifdef CONFIG_CGROUP_FAVOR_DYNMODS
-	ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
-#endif
+	if (have_favordynmods)
+		ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
+
 	return 0;
 }
 
@@ -6764,6 +6766,12 @@ static int __init enable_cgroup_debug(char *str)
 }
 __setup("cgroup_debug", enable_cgroup_debug);
 
+static int __init cgroup_favordynmods_setup(char *str)
+{
+	return (kstrtobool(str, &have_favordynmods) == 0);
+}
+__setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
+
 /**
  * css_tryget_online_from_dir - get corresponding css from a cgroup dentry
  * @dentry: directory dentry of interest
-- 
2.40.1

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Waiman Long]

On 9/5/23 20:57, Luiz Capitulino wrote:
> We have a need of using favordynmods with cgroup v1, which doesn't support
> changing mount flags during remount. Enabling CONFIG_FAVOR_DYNMODS at
> build-time is not an option because we want to be able to selectively
> enable it for certain systems.
>
> This commit addresses this by introducing the cgroup_favordynmods=
> command-line option. This option works for both cgroup v1 and v2 and
> also allows for disabling favorynmods when the kernel built with
> CONFIG_FAVOR_DYNMODS=y.
>
> Signed-off-by: Luiz Capitulino <luizcap-vV1OtcyAfmbQT0dZR+AlfA@public.gmane.org>
> ---
>   Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>   kernel/cgroup/cgroup.c                          | 14 +++++++++++---
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> o v2
>   - Use __ro_after_init [Waiman]
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..8b744d39d393 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -580,6 +580,10 @@
>   			named mounts. Specifying both "all" and "named" disables
>   			all v1 hierarchies.
>   
> +	cgroup_favordynmods= [KNL] Enable or Disable favordynmods.
> +			Format: { "true" | "false" }
> +			Defaults to the value of CONFIG_CGROUP_FAVOR_DYNMODS.
> +
>   	cgroup.memory=	[KNL] Pass options to the cgroup memory controller.
>   			Format: <string>
>   			nosocket -- Disable socket memory accounting.
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 1fb7f562289d..2b7d74304606 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -207,6 +207,8 @@ static u16 have_exit_callback __read_mostly;
>   static u16 have_release_callback __read_mostly;
>   static u16 have_canfork_callback __read_mostly;
>   
> +static bool have_favordynmods __ro_after_init = IS_ENABLED(CONFIG_CGROUP_FAVOR_DYNMODS);
> +
>   /* cgroup namespace for init task */
>   struct cgroup_namespace init_cgroup_ns = {
>   	.ns.count	= REFCOUNT_INIT(2),
> @@ -2243,9 +2245,9 @@ static int cgroup_init_fs_context(struct fs_context *fc)
>   	fc->user_ns = get_user_ns(ctx->ns->user_ns);
>   	fc->global = true;
>   
> -#ifdef CONFIG_CGROUP_FAVOR_DYNMODS
> -	ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
> -#endif
> +	if (have_favordynmods)
> +		ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
> +
>   	return 0;
>   }
>   
> @@ -6764,6 +6766,12 @@ static int __init enable_cgroup_debug(char *str)
>   }
>   __setup("cgroup_debug", enable_cgroup_debug);
>   
> +static int __init cgroup_favordynmods_setup(char *str)
> +{
> +	return (kstrtobool(str, &have_favordynmods) == 0);
> +}
> +__setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
> +
>   /**
>    * css_tryget_online_from_dir - get corresponding css from a cgroup dentry
>    * @dentry: directory dentry of interest
Reviewed-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Kamalesh Babulal]

On 9/6/23 06:27, Luiz Capitulino wrote:
[...]
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 1fb7f562289d..2b7d74304606 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -207,6 +207,8 @@ static u16 have_exit_callback __read_mostly;
>  static u16 have_release_callback __read_mostly;
>  static u16 have_canfork_callback __read_mostly;
>  
> +static bool have_favordynmods __ro_after_init = IS_ENABLED(CONFIG_CGROUP_FAVOR_DYNMODS);
> +
>  /* cgroup namespace for init task */
>  struct cgroup_namespace init_cgroup_ns = {
>  	.ns.count	= REFCOUNT_INIT(2),
> @@ -2243,9 +2245,9 @@ static int cgroup_init_fs_context(struct fs_context *fc)
>  	fc->user_ns = get_user_ns(ctx->ns->user_ns);
>  	fc->global = true;
>  
> -#ifdef CONFIG_CGROUP_FAVOR_DYNMODS
> -	ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
> -#endif
> +	if (have_favordynmods)
> +		ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
> +
>  	return 0;
>  }
>  
> @@ -6764,6 +6766,12 @@ static int __init enable_cgroup_debug(char *str)
>  }
>  __setup("cgroup_debug", enable_cgroup_debug);
>  
> +static int __init cgroup_favordynmods_setup(char *str)
> +{
> +	return (kstrtobool(str, &have_favordynmods) == 0);
> +}
> +__setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
> +
>  /**
>   * css_tryget_online_from_dir - get corresponding css from a cgroup dentry
>   * @dentry: directory dentry of interest

Consider a case where the kernel is compiled with
CONFIG_CGROUP_FAVOR_DYNMODS=n and kernel command line is passed with
cgroup_favordynmods=true, this would set the have_favordynmods to true.
In cgroup_favordynmods_setup(), should it return 0 with a pr_warn(),
when CONFIG_CGROUP_FAVOR_DYNMODS=n in the above case, or is this
expected behavior?

-- 
Thanks,
Kamalesh

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Waiman Long]

On 9/6/23 02:58, Kamalesh Babulal wrote:
> On 9/6/23 06:27, Luiz Capitulino wrote:
> [...]
>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>> index 1fb7f562289d..2b7d74304606 100644
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
>> @@ -207,6 +207,8 @@ static u16 have_exit_callback __read_mostly;
>>   static u16 have_release_callback __read_mostly;
>>   static u16 have_canfork_callback __read_mostly;
>>   
>> +static bool have_favordynmods __ro_after_init = IS_ENABLED(CONFIG_CGROUP_FAVOR_DYNMODS);
>> +
>>   /* cgroup namespace for init task */
>>   struct cgroup_namespace init_cgroup_ns = {
>>   	.ns.count	= REFCOUNT_INIT(2),
>> @@ -2243,9 +2245,9 @@ static int cgroup_init_fs_context(struct fs_context *fc)
>>   	fc->user_ns = get_user_ns(ctx->ns->user_ns);
>>   	fc->global = true;
>>   
>> -#ifdef CONFIG_CGROUP_FAVOR_DYNMODS
>> -	ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
>> -#endif
>> +	if (have_favordynmods)
>> +		ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
>> +
>>   	return 0;
>>   }
>>   
>> @@ -6764,6 +6766,12 @@ static int __init enable_cgroup_debug(char *str)
>>   }
>>   __setup("cgroup_debug", enable_cgroup_debug);
>>   
>> +static int __init cgroup_favordynmods_setup(char *str)
>> +{
>> +	return (kstrtobool(str, &have_favordynmods) == 0);
>> +}
>> +__setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
>> +
>>   /**
>>    * css_tryget_online_from_dir - get corresponding css from a cgroup dentry
>>    * @dentry: directory dentry of interest
> Consider a case where the kernel is compiled with
> CONFIG_CGROUP_FAVOR_DYNMODS=n and kernel command line is passed with
> cgroup_favordynmods=true, this would set the have_favordynmods to true.
> In cgroup_favordynmods_setup(), should it return 0 with a pr_warn(),
> when CONFIG_CGROUP_FAVOR_DYNMODS=n in the above case, or is this
> expected behavior?

According to the documentation of __setup:

/*
  * NOTE: __setup functions return values:
  * @fn returns 1 (or non-zero) if the option argument is "handled"
  * and returns 0 if the option argument is "not handled".
  */

So the return value should tell whether the input parameter is a 
recognizable true or false value, not whether it is true or false. 
kstrtobool returns 0 if it is a recognizable T/F value or -EINVAL 
otherwise. So the check is correct. I did double check that before I 
ack'ed the patch.

Cheers,
Longman

Re: [External] : Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Kamalesh Babulal]

On 9/6/23 18:29, Waiman Long wrote:
> On 9/6/23 02:58, Kamalesh Babulal wrote:
>> On 9/6/23 06:27, Luiz Capitulino wrote:
>> [...]
>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>>> index 1fb7f562289d..2b7d74304606 100644
>>> --- a/kernel/cgroup/cgroup.c
>>> +++ b/kernel/cgroup/cgroup.c
>>> @@ -207,6 +207,8 @@ static u16 have_exit_callback __read_mostly;
>>>   static u16 have_release_callback __read_mostly;
>>>   static u16 have_canfork_callback __read_mostly;
>>>   +static bool have_favordynmods __ro_after_init = IS_ENABLED(CONFIG_CGROUP_FAVOR_DYNMODS);
>>> +
>>>   /* cgroup namespace for init task */
>>>   struct cgroup_namespace init_cgroup_ns = {
>>>       .ns.count    = REFCOUNT_INIT(2),
>>> @@ -2243,9 +2245,9 @@ static int cgroup_init_fs_context(struct fs_context *fc)
>>>       fc->user_ns = get_user_ns(ctx->ns->user_ns);
>>>       fc->global = true;
>>>   -#ifdef CONFIG_CGROUP_FAVOR_DYNMODS
>>> -    ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
>>> -#endif
>>> +    if (have_favordynmods)
>>> +        ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
>>> +
>>>       return 0;
>>>   }
>>>   @@ -6764,6 +6766,12 @@ static int __init enable_cgroup_debug(char *str)
>>>   }
>>>   __setup("cgroup_debug", enable_cgroup_debug);
>>>   +static int __init cgroup_favordynmods_setup(char *str)
>>> +{
>>> +    return (kstrtobool(str, &have_favordynmods) == 0);
>>> +}
>>> +__setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
>>> +
>>>   /**
>>>    * css_tryget_online_from_dir - get corresponding css from a cgroup dentry
>>>    * @dentry: directory dentry of interest
>> Consider a case where the kernel is compiled with
>> CONFIG_CGROUP_FAVOR_DYNMODS=n and kernel command line is passed with
>> cgroup_favordynmods=true, this would set the have_favordynmods to true.
>> In cgroup_favordynmods_setup(), should it return 0 with a pr_warn(),
>> when CONFIG_CGROUP_FAVOR_DYNMODS=n in the above case, or is this
>> expected behavior?
> 
> According to the documentation of __setup:
> 
> /*
>  * NOTE: __setup functions return values:
>  * @fn returns 1 (or non-zero) if the option argument is "handled"
>  * and returns 0 if the option argument is "not handled".
>  */
> 
> So the return value should tell whether the input parameter is a recognizable true or false value, not whether it is true or false. kstrtobool returns 0 if it is a recognizable T/F value or -EINVAL otherwise. So the check is correct. I did double check that before I ack'ed the patch.
> 

Apologies for not being clear in the previous email. It was in two parts,  
where the first one was more of a question, where if a kernel is compiled
with CONFIG_CGROUP_FAVOR_DYNMODS config option disabled and the user    
attempts to pass cgroup_favordynmods=true in the kernel command line.   
                                                                        
In this scenario, the have_favordynmods is set to true regardless of    
the CONFIG_CGROUP_FAVOR_DYNMODS config option being enabled/disabled in 
the kernel. This allows the user to set CGRP_ROOT_FAVOR_DYNMODS flag    
without enabling the CONFIG_CGROUP_FAVOR_DYNMODS kernel config.         
                                                                        
Shouldn't the cgroup_favordynmods kernel parameter be valid only when   
the kernel is compiled with CONFIG_CGROUP_FAVOR_DYNMODS=y and allows the 
user to only disable it in the kernel command line instead of allowing  
them to set/unset have_favordynmods when CONFIG_CGROUP_FAVOR_DYNMODS is 
disabled.                                                               
                                                                        
If the above assumption is right, that's where the second part was of   
email, where I was suggesting the restriction by using ifdef guards in  
cgroup_favordynmods_setup(), something like:

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 2b7d74304606..5c7d1a0b1dbe 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6768,7 +6768,11 @@ __setup("cgroup_debug", enable_cgroup_debug);
 
 static int __init cgroup_favordynmods_setup(char *str)
 {
+#ifdef CGROUP_FAVOR_DYNMODS
        return (kstrtobool(str, &have_favordynmods) == 0);
+#endif
+       pr_warn("Favor Dynmods not supported\n");
+       return 0;
 }
 __setup("cgroup_favordynmods=", cgroup_favordynmods_setup);

-- 
Thanks,
Kamalesh

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On Wed, Sep 06, 2023 at 12:57:12AM +0000, Luiz Capitulino <luizcap@amazon.com> wrote:
> We have a need of using favordynmods with cgroup v1, which doesn't support
> changing mount flags during remount. Enabling CONFIG_FAVOR_DYNMODS at
> build-time is not an option because we want to be able to selectively
> enable it for certain systems.

Could this be implemented by a utility that would read /proc/cmdline
(while kernel ignores the arg) and remount respective hierarchies
accordingly? Or what do I miss?

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Luiz Capitulino]

On 2023-09-07 11:06, Michal Koutný wrote:
> On Wed, Sep 06, 2023 at 12:57:12AM +0000, Luiz Capitulino <luizcap-vV1OtcyAfmbQT0dZR+AlfA@public.gmane.org> wrote:
>> We have a need of using favordynmods with cgroup v1, which doesn't support
>> changing mount flags during remount. Enabling CONFIG_FAVOR_DYNMODS at
>> build-time is not an option because we want to be able to selectively
>> enable it for certain systems.
> 
> Could this be implemented by a utility that would read /proc/cmdline
> (while kernel ignores the arg) and remount respective hierarchies
> accordingly? Or what do I miss?

Yeah, this works for cgroup v2 but my understanding is that cgroup v1
doesn't support changing flags in remount, take a look at
cgroup1_reconfigure().

- Luiz

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Thu, Sep 07, 2023 at 11:16:41AM -0400, Luiz Capitulino <luizcap@amazon.com> wrote:
> Yeah, this works for cgroup v2 but my understanding is that cgroup v1
> doesn't support changing flags in remount, take a look at
> cgroup1_reconfigure().

Ah, didn't notice.
Alhtough -- there seems to be a deeper issue -- the mount option doesn't
have a per-root semantics. There is only a single
cgroup_threadgroup_rwsem afterall.

Even with your cmdline option, you may loose the behavior after
unmounting any of the v1 hierarchies (cgroup_destroy_root()
unconditionally disables it).

Or you could still achieve the result by mounting cgroup2 hierarchy with
favordynmods. (And unmount it, default root is not ever released.)

Maybe it would be the best to have this controllable only via v2
hierarchy (as it's the only documented).
(And maybe v1s should not show the option at all.)

My 0.02€,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Luiz Capitulino]

[Resending, looks like I'm having issues with my mail server]

On 2023-09-07 05:51, Kamalesh Babulal wrote:

> 
> On 9/6/23 18:29, Waiman Long wrote:
>> On 9/6/23 02:58, Kamalesh Babulal wrote:
>>> On 9/6/23 06:27, Luiz Capitulino wrote:
>>> [...]
>>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>>>> index 1fb7f562289d..2b7d74304606 100644
>>>> --- a/kernel/cgroup/cgroup.c
>>>> +++ b/kernel/cgroup/cgroup.c
>>>> @@ -207,6 +207,8 @@ static u16 have_exit_callback __read_mostly;
>>>>    static u16 have_release_callback __read_mostly;
>>>>    static u16 have_canfork_callback __read_mostly;
>>>>    +static bool have_favordynmods __ro_after_init = IS_ENABLED(CONFIG_CGROUP_FAVOR_DYNMODS);
>>>> +
>>>>    /* cgroup namespace for init task */
>>>>    struct cgroup_namespace init_cgroup_ns = {
>>>>        .ns.count    = REFCOUNT_INIT(2),
>>>> @@ -2243,9 +2245,9 @@ static int cgroup_init_fs_context(struct fs_context *fc)
>>>>        fc->user_ns = get_user_ns(ctx->ns->user_ns);
>>>>        fc->global = true;
>>>>    -#ifdef CONFIG_CGROUP_FAVOR_DYNMODS
>>>> -    ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
>>>> -#endif
>>>> +    if (have_favordynmods)
>>>> +        ctx->flags |= CGRP_ROOT_FAVOR_DYNMODS;
>>>> +
>>>>        return 0;
>>>>    }
>>>>    @@ -6764,6 +6766,12 @@ static int __init enable_cgroup_debug(char *str)
>>>>    }
>>>>    __setup("cgroup_debug", enable_cgroup_debug);
>>>>    +static int __init cgroup_favordynmods_setup(char *str)
>>>> +{
>>>> +    return (kstrtobool(str, &have_favordynmods) == 0);
>>>> +}
>>>> +__setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
>>>> +
>>>>    /**
>>>>     * css_tryget_online_from_dir - get corresponding css from a cgroupdentry
>>>>     * @dentry: directory dentry of interest
>>> Consider a case where the kernel is compiled with
>>> CONFIG_CGROUP_FAVOR_DYNMODS=n and kernel command line is passed with
>>> cgroup_favordynmods=true, this would set the have_favordynmods to true.
>>> In cgroup_favordynmods_setup(), should it return 0 with a pr_warn(),
>>> when CONFIG_CGROUP_FAVOR_DYNMODS=n in the above case, or is this
>>> expected behavior?
>>
>> According to the documentation of __setup:
>>
>> /*
>>   * NOTE: __setup functions return values:
>>   * @fn returns 1 (or non-zero) if the option argument is "handled"
>>   * and returns 0 if the option argument is "not handled".
>>   */
>>
>> So the return value should tell whether the input parameter is a recognizable true or false value, not whether it is true or false. kstrtobool returns 0 if it is a recognizable T/F value or -EINVAL otherwise. So the check is correct. I did double check that before I ack'ed the patch.
>>
> 
> Apologies for not being clear in the previous email. It was in two parts,
> where the first one was more of a question, where if a kernel is compiled
> with CONFIG_CGROUP_FAVOR_DYNMODS config option disabled and the user
> attempts to pass cgroup_favordynmods=true in the kernel command line.
> 
> In this scenario, the have_favordynmods is set to true regardless of
> the CONFIG_CGROUP_FAVOR_DYNMODS config option being enabled/disabled in
> the kernel. This allows the user to set CGRP_ROOT_FAVOR_DYNMODS flag
> without enabling the CONFIG_CGROUP_FAVOR_DYNMODS kernel config.

Correct, that's exactly the goal of this patch: to give users the
option to enable/disable favordynmods at boot-time regardless of
CONFIG_FAVOR_DYNMODS.

This is especially useful with cgroup v1 where remounting with
favordynmods is not supported.


> Shouldn't the cgroup_favordynmods kernel parameter be valid only when
> the kernel is compiled with CONFIG_CGROUP_FAVOR_DYNMODS=y and allows the
> user to only disable it in the kernel command line instead of allowing
> them to set/unset have_favordynmods when CONFIG_CGROUP_FAVOR_DYNMODS is
> disabled.

This was my first idea as well, but since we'd allow for enabling why
not allow for disabling as well? Besides, the resulting code is
fairly simple.

> If the above assumption is right, that's where the second part was of
> email, where I was suggesting the restriction by using ifdef guards in
> cgroup_favordynmods_setup(), something like:
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 2b7d74304606..5c7d1a0b1dbe 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -6768,7 +6768,11 @@ __setup("cgroup_debug", enable_cgroup_debug);
> 
>   static int __init cgroup_favordynmods_setup(char *str)
>   {
> +#ifdef CGROUP_FAVOR_DYNMODS
>          return (kstrtobool(str, &have_favordynmods) == 0);
> +#endif
> +       pr_warn("Favor Dynmods not supported\n");
> +       return 0;
>   }

Why should we do this? What's the benefit for the user?

>   __setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
> 
> --
> Thanks,
> Kamalesh

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Luiz Capitulino]

[Resending, looks like I'm having issues with my mail server]

On 2023-09-07 11:57, Michal Koutný wrote:
> On Thu, Sep 07, 2023 at 11:16:41AM -0400, Luiz Capitulino <luizcap@amazon.com> wrote:
>> Yeah, this works for cgroup v2 but my understanding is that cgroup v1
>> doesn't support changing flags in remount, take a look at
>> cgroup1_reconfigure().
> 
> Ah, didn't notice.
> Alhtough -- there seems to be a deeper issue -- the mount option doesn't
> have a per-root semantics. There is only a single
> cgroup_threadgroup_rwsem afterall.
> 
> Even with your cmdline option, you may loose the behavior after
> unmounting any of the v1 hierarchies (cgroup_destroy_root()
> unconditionally disables it).

Good point. I haven't checked this in detail yet, but if
CONFIG_CGROUP_FAVOR_DYNMODS has the same behavior then I wouldn't worry
much about this. Also, I don't know how common it is to unmount and
mount a cgroup hierarchies (if it's not so common then it's even
less important).

We could also investigate on how to make the flag stick as a follow
up work on this.

> 
> Or you could still achieve the result by mounting cgroup2 hierarchy with
> favordynmods. (And unmount it, default root is not ever released.)
> 
> Maybe it would be the best to have this controllable only via v2
> hierarchy (as it's the only documented).
> (And maybe v1s should not show the option at all.)

The main motivation for this patch is really v1 since we can
simply remount v2 with favordynmods enabled (although we do
find this very useful for v2 as well).

Another crazy idea (based on your suggestion to allow only this
controllable in v2), would be to make favordynmods enabled by
default in v1 w/ the rationale that new behavior changes affect
only v2.

- Luiz

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Kamalesh Babulal]

On 9/8/23 00:17, Luiz Capitulino wrote:

[...]

>>>> Consider a case where the kernel is compiled with
>>>> CONFIG_CGROUP_FAVOR_DYNMODS=n and kernel command line is passed with
>>>> cgroup_favordynmods=true, this would set the have_favordynmods to true.
>>>> In cgroup_favordynmods_setup(), should it return 0 with a pr_warn(),
>>>> when CONFIG_CGROUP_FAVOR_DYNMODS=n in the above case, or is this
>>>> expected behavior?
>>>
>>> According to the documentation of __setup:
>>>
>>> /*
>>>   * NOTE: __setup functions return values:
>>>   * @fn returns 1 (or non-zero) if the option argument is "handled"
>>>   * and returns 0 if the option argument is "not handled".
>>>   */
>>>
>>> So the return value should tell whether the input parameter is a recognizable true or false value, not whether it is true or false. kstrtobool returns 0 if it is a recognizable T/F value or -EINVAL otherwise. So the check is correct. I did double check that before I ack'ed the patch.
>>>
>>
>> Apologies for not being clear in the previous email. It was in two parts,
>> where the first one was more of a question, where if a kernel is compiled
>> with CONFIG_CGROUP_FAVOR_DYNMODS config option disabled and the user
>> attempts to pass cgroup_favordynmods=true in the kernel command line.
>>
>> In this scenario, the have_favordynmods is set to true regardless of
>> the CONFIG_CGROUP_FAVOR_DYNMODS config option being enabled/disabled in
>> the kernel. This allows the user to set CGRP_ROOT_FAVOR_DYNMODS flag
>> without enabling the CONFIG_CGROUP_FAVOR_DYNMODS kernel config.
> 
> Correct, that's exactly the goal of this patch: to give users the
> option to enable/disable favordynmods at boot-time regardless of
> CONFIG_FAVOR_DYNMODS.
> 
> This is especially useful with cgroup v1 where remounting with
> favordynmods is not supported.

Thank you so much for explaining. I understand the idea of the
patch better now.

>> Shouldn't the cgroup_favordynmods kernel parameter be valid only when
>> the kernel is compiled with CONFIG_CGROUP_FAVOR_DYNMODS=y and allows the
>> user to only disable it in the kernel command line instead of allowing
>> them to set/unset have_favordynmods when CONFIG_CGROUP_FAVOR_DYNMODS is
>> disabled.
> 
> This was my first idea as well, but since we'd allow for enabling why
> not allow for disabling as well? Besides, the resulting code is
> fairly simple.

Agreed, If it's independent of CONFIG_CGROUP_FAVOR_DYNMODS config
option, providing both enable and disable, is useful.

>> If the above assumption is right, that's where the second part was of
>> email, where I was suggesting the restriction by using ifdef guards in
>> cgroup_favordynmods_setup(), something like:
>>
>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>> index 2b7d74304606..5c7d1a0b1dbe 100644
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
>> @@ -6768,7 +6768,11 @@ __setup("cgroup_debug", enable_cgroup_debug);
>>
>>   static int __init cgroup_favordynmods_setup(char *str)
>>   {
>> +#ifdef CGROUP_FAVOR_DYNMODS
>>          return (kstrtobool(str, &have_favordynmods) == 0);
>> +#endif
>> +       pr_warn("Favor Dynmods not supported\n");
>> +       return 0;
>>   }
> 
> Why should we do this? What's the benefit for the user?

This code was constructed on the idea of have_favordynmods, should
be available only when the kernel is compiled with CONFIG_CGROUP_FAVOR_DYNMODS
and it's of no benefit.
 
>>   __setup("cgroup_favordynmods=", cgroup_favordynmods_setup);
>>

-- 
Thanks,
Kamalesh

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Tejun Heo]

On Mon, Sep 18, 2023 at 10:01:13AM -0400, Luiz Capitulino wrote:
> 
> 
> On 2023-09-05 20:57, Luiz Capitulino wrote:
> > We have a need of using favordynmods with cgroup v1, which doesn't support
> > changing mount flags during remount. Enabling CONFIG_FAVOR_DYNMODS at
> > build-time is not an option because we want to be able to selectively
> > enable it for certain systems.
> > 
> > This commit addresses this by introducing the cgroup_favordynmods=
> > command-line option. This option works for both cgroup v1 and v2 and
> > also allows for disabling favorynmods when the kernel built with
> > CONFIG_FAVOR_DYNMODS=y.
> > 
> > Signed-off-by: Luiz Capitulino <luizcap@amazon.com>
> 
> Ping?

Michal raised some valid concerns. I don't really mind that it's not great
on the edges tho. Michal, what do you think?

Thanks.

-- 
tejun

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Mon, Sep 18, 2023 at 07:47:13AM -1000, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Michal raised some valid concerns. I don't really mind that it's not great
> on the edges tho. Michal, what do you think?

I'd have a few suggestions:
- reset to have_dynmods value instead of false in cgroup_destroy_root()
  (to the benefing of the users of this option, not the common default
  users) or not touch the value in cgroup_destroy_root() at all
- s/CONFIG_FAVOR_DYNMODS/CONFIG_CGROUP_FAVOR_DYNMODS/ in commit message

Those should be minor tweaks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATH v2] cgroup: add cgroup_favordynmods= command-line option

[Luiz Capitulino]

On 2023-09-19 07:00, Michal Koutný wrote:
> On Mon, Sep 18, 2023 at 07:47:13AM -1000, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> Michal raised some valid concerns. I don't really mind that it's not great
>> on the edges tho. Michal, what do you think?
> 
> I'd have a few suggestions:
> - reset to have_dynmods value instead of false in cgroup_destroy_root()
>    (to the benefing of the users of this option, not the common default
>    users) or not touch the value in cgroup_destroy_root() at all
> - s/CONFIG_FAVOR_DYNMODS/CONFIG_CGROUP_FAVOR_DYNMODS/ in commit message

Thank you for the detailed suggestions, Michal. I'll on this for v3.

- Luiz