Re: [PATCH -next] cgroup: Use descriptor table to unify mount flag management

[Chen Ridong <chenridong@huaweicloud.com>]

On 2025/12/2 17:24, Michal Koutný wrote:
> Hi Ridong.
> 
> On Wed, Nov 26, 2025 at 02:08:25AM +0000, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> From: Chen Ridong <chenridong@huawei.com>
>>
>> The cgroup2 mount flags (e.g. nsdelegate, favordynmods) were previously
>> handled via scattered switch-case and conditional checks across
>> parameter parsing, flag application, and option display paths. This
>> leads to redundant code and increased maintenance cost when adding/removing
>> flags.
>>
>> Introduce a `cgroup_mount_flag_desc` descriptor table to centralize the
>> mapping between flag bits, names, and apply functions. Refactor the
>> relevant paths to use this table for unified management:
>> 1. cgroup2_parse_param: Replace switch-case with table lookup
>> 2. apply_cgroup_root_flags: Replace multiple conditionals with table
>>    iteration
>> 3. cgroup_show_options: Replace hardcoded seq_puts with table-driven output
>>
>> No functional change intended, and the mount option output format remains
>> compatible with the original implementation.
> 
> At first I thought this is worthy but then I ran into the possible
> (semantic) overlap with the cgroup2_fs_parameters array (the string
> `name`s are duplicated in both :-/), I didn't figure out a way how to
> make such an polymorphic array in C (like when cgroup_mount_flag_desc
> would be a class that inherits from fs_parameter_spec and you could pass
> the array of the formers to consumers (fs_parse()) of latters).
> 
> So I'm wondering whether there exists some way to avoid possible
> divergence between definitions of the two arrays...
> 

Hi Michal,

Thank you for your thoughtful feedback.

I understand your concern about the semantic overlap between the two arrays and the potential for
divergence. I initially tried to find a way to merge them into a single polymorphic array, but given
the constraints of C and the existing fs_parameter_spec structure (which we cannot easily modify for
this purpose), I haven't found a clean way to achieve that.

However, to address the maintenance issue, I've come up with an alternative approach using a macro
that allows us to define mount flags in just one place. The idea is to introduce a macro list
CGROUP2_MOUNT_FLAG_LIST that expands into both the fs_parameter_spec array and the new
cgroup_mount_flag_desc table. This way, when adding a new mount flag, we only need to extend this
single macro list.

While we still end up with two separate arrays, the macro ensures that any addition or modification
only needs to be made in one place—the CGROUP2_MOUNT_FLAG_LIST. This should prevent the divergence
you mentioned.

What do you think about this approach? If you have any suggestions for further improvement, I'd be
happy to incorporate them.

Below is a simplified diff showing the concept:

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index e717208cfb18..bd81b15dc3bd 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1985,26 +1985,53 @@ int cgroup_show_path(struct seq_file *sf, struct kernfs_node *kf_node,
 	return len;
 }

+#define CGROUP2_MOUNT_FLAG_LIST(_)					\
+	_(nsdelegate,		CGRP_ROOT_NS_DELEGATE,	apply_cgroup_root_flag) \
+	_(favordynmods,	CGRP_ROOT_FAVOR_DYNMODS,	apply_cgroup_favor_flag) \
+	_(memory_localevents,	CGRP_ROOT_MEMORY_LOCAL_EVENTS, apply_cgroup_root_flag) \
+	_(memory_recursiveprot,	CGRP_ROOT_MEMORY_RECURSIVE_PROT, apply_cgroup_root_flag) \
+	_(memory_hugetlb_accounting, CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING, apply_cgroup_root_flag) \
+	_(pids_localevents,	CGRP_ROOT_PIDS_LOCAL_EVENTS, apply_cgroup_root_flag)
+
 enum cgroup2_param {
-	Opt_nsdelegate,
-	Opt_favordynmods,
-	Opt_memory_localevents,
-	Opt_memory_recursiveprot,
-	Opt_memory_hugetlb_accounting,
-	Opt_pids_localevents,
+#define CGROUP2_PARAM_ENUM(name, ...) Opt_##name,
+	CGROUP2_MOUNT_FLAG_LIST(CGROUP2_PARAM_ENUM)
+#undef CGROUP2_PARAM_ENUM
 	nr__cgroup2_params
 };

+struct cgroup_mount_flag_desc {
+	enum cgroup_root_flag flag;
+	const char *name;
+	void (*apply)(enum cgroup_root_flag flag, bool enable);
+};
+
+static void apply_cgroup_root_flag(enum cgroup_root_flag flag, bool enable)
+{
+	if (enable)
+		cgrp_dfl_root.flags |= flag;
+	else
+		cgrp_dfl_root.flags &= ~flag;
+}
+
+static void apply_cgroup_favor_flag(enum cgroup_root_flag flag, bool enable)
+{
+	cgroup_favor_dynmods(&cgrp_dfl_root, enable);
+}
+
 static const struct fs_parameter_spec cgroup2_fs_parameters[] = {
-	fsparam_flag("nsdelegate",		Opt_nsdelegate),
-	fsparam_flag("favordynmods",		Opt_favordynmods),
-	fsparam_flag("memory_localevents",	Opt_memory_localevents),
-	fsparam_flag("memory_recursiveprot",	Opt_memory_recursiveprot),
-	fsparam_flag("memory_hugetlb_accounting", Opt_memory_hugetlb_accounting),
-	fsparam_flag("pids_localevents",	Opt_pids_localevents),
+#define CGROUP2_PARAM_SPEC(name, ...) fsparam_flag(#name, Opt_##name),
+	CGROUP2_MOUNT_FLAG_LIST(CGROUP2_PARAM_SPEC)
+#undef CGROUP2_PARAM_SPEC
 	{}
 };

+static const struct cgroup_mount_flag_desc cgroup2_mount_flags[] = {
+#define CGROUP2_FLAG_DESC(name, flag, apply)[Opt_##name] = { flag, #name, apply },
+	CGROUP2_MOUNT_FLAG_LIST(CGROUP2_FLAG_DESC)
+#undef CGROUP2_FLAG_DESC
+};
+
...

-- 
Best regards,
Ridong