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

[Kamalesh Babulal <kamalesh.babulal@oracle.com>]

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