[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
