Re: [PATCH v6 06/18] fs/resctrl: Introduce kernel mode states for resctrl

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

nit: "for resctrl" in subject is redundant.

On 10/1/26 8:10 AM, Babu Moger wrote:
> The filesystem tracks supported kernel modes, the active mode, and the
> control and monitoring association for the assigned group.

Please let the context describe the context at the time of the patch, not how
things will work after entire series is applied.

Please use consistent terminology, this series switches between "assigned group"
and "backing group" to refer to the same group.

Please use the changelog to guide the reader to understand the architecture
and how things fit together. Many changelogs are very cryptic without explanation
of what the patch does and why.

> 
> Introduce struct resctrl_kmode_cfg and initialize the default configuration
> from the platform's capabilities.

Please check all changelogs to ensure they all are able to provide an
answer to "why". More specifically, please consider:
	https://lore.kernel.org/lkml/20250911165433.GBaML-yTUZHkywuJIe@fat_crate.local/

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
...

> ---
>  fs/resctrl/internal.h | 29 +++++++++++++++++++++++++++++
>  fs/resctrl/rdtgroup.c | 25 +++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index e62a277dee85..98f04a2a762b 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -314,6 +314,35 @@ struct mbm_state {
>  	u32	prev_bw;
>  };
>  
> +/**
> + * enum kmode_state - Control or monitoring association

What does "control or monitoring association" mean? This is becoming
an overloaded term used to mean different things and becoming confusing.
For comparison, earlier patch introduced *another*
"control and monitoring association" enum:

	enum resctrl_kernel_mode - Kernel mode control and monitoring association

> + * @KMODE_INHERIT: Inherit from the user-space task.
> + * @KMODE_ASSIGN: Use the backing group's CLOSID or RMID.
> + */
> +enum kmode_state {
> +	KMODE_INHERIT,
> +	KMODE_ASSIGN
> +};
> +
> +/**
> + * struct resctrl_kmode_cfg - Kernel mode configuration
> + * @kmode_sup:	Bitmap of supported kernel modes.
> + * @kmode_cur:	Currently selected kernel mode.

Adding this comment during a second pass ... please consider later motivations
about naming of these fields. This field is introduced as "kmode_cur" but when
considering the language used in rest of series "active" seems more appropriate.

@kmode_sup could also just be @supported (the "kmode" is redundant).

> + * @ctrl:	Control association for %RESCTRL_GLOBAL_ENABLE_PER_CPU.
> + *		Kept while %RESCTRL_INHERIT_USER is selected.

The "Kept while ..." does not seem to match implementation. Looks like
resctrl_kmode_switch() always resets it to KMODE_ASSIGN?

At this point the reader is left to decipher the architecture via cryptic
changelogs and inaccurate comments. It is very difficult to parse.

> + * @mon:	Monitoring association for %RESCTRL_GLOBAL_ENABLE_PER_CPU.
> + *		Kept while %RESCTRL_INHERIT_USER is selected.

Similar comment about "Kept ..."

> + * @rdtgrp:	Resource group backing %RESCTRL_GLOBAL_ENABLE_PER_CPU.
> + *		%NULL while that mode is not selected.
> + */
> +struct resctrl_kmode_cfg {
> +	DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES);
> +	enum resctrl_kernel_mode	kmode_cur;
> +	enum kmode_state		ctrl;
> +	enum kmode_state		mon;
> +	struct rdtgroup			*rdtgrp;
> +};
> +
>  extern struct mutex rdtgroup_mutex;
>  
>  static inline const char *rdt_kn_name(const struct kernfs_node *kn)
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 68be9b903ac6..7c39792be1fa 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -76,6 +76,9 @@ static void rdtgroup_destroy_root(void);
>  
>  struct dentry *debugfs_resctrl;
>  
> +/* Kernel mode for this system. */
> +static struct resctrl_kmode_cfg resctrl_kmode;
> +
>  /*
>   * Memory bandwidth monitoring event to use for the default CTRL_MON group
>   * and each new CTRL_MON group created by the user.  Only relevant when
> @@ -2297,6 +2300,26 @@ static void io_alloc_init(void)
>  	}
>  }
>  
> +/*
> + * Set RESCTRL_INHERIT_USER as the default when the platform supports
> + * allocation or monitoring. Otherwise leave kernel mode unsupported.

This (and why) would be useful to describe in changelog.

> + */
> +static void resctrl_kmode_init(void)
> +{
> +	if (!resctrl_arch_alloc_capable() && !resctrl_arch_mon_capable())
> +		return;
> +

This check seems unnecessary? resctrl_kmode_init() will only be called
on a system that is capable of allocation or monitoring. 
The planned AET changes will change this behavior see
https://lore.kernel.org/lkml/20260928221509.68002-16-tony.luck@intel.com/
but in that case it still seems as though initializing the kernel mode as
below always needs to be done. 

That begs the question why is resctrl_kmode_init() needed at all? Why not use
a static initializer? 

I find resctrl_kmode_init() to be potentially confusing since it does *not*
actually initialize resctrl_kmode. This is only possible to see two patches
later where get_rdt_resources() is changed so that x86 sets its supported
modes and this runs *before* resctrl_kmode_init().

If anyone decides one day "this initialization code is missing a bitmap_zero()"
then it will break PLZA.

> +	resctrl_kmode.kmode_cur = RESCTRL_INHERIT_USER;
> +	__set_bit(RESCTRL_INHERIT_USER, resctrl_kmode.kmode_sup);
> +
> +	/*
> +	 * Defaults used when RESCTRL_GLOBAL_ENABLE_PER_CPU mode is
> +	 * activated without new values.
> +	 */
> +	resctrl_kmode.ctrl = KMODE_ASSIGN;
> +	resctrl_kmode.mon = KMODE_ASSIGN;
> +}
> +
>  void resctrl_file_fflags_init(const char *config, unsigned long fflags)
>  {
>  	struct rftype *rft;
> @@ -4814,6 +4837,8 @@ int resctrl_init(void)
>  	if (ret)
>  		return ret;
>  
> +	resctrl_kmode_init();
> +
>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>  	if (ret) {
>  		resctrl_l3_mon_resource_exit();

Reinette