Re: [RESEND PATCH v4 06/15] fs/resctrl: Introduce resctrl_set_kmode_support() to initialize supported modes

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

Hi Babu,

On 7/7/26 2:50 PM, Babu Moger wrote:
> Architectures need a way to advertise which kernel-mode resctrl policies

"kernel-mode resctrl policies" -> "kernel modes"?

> they support before generic resctrl exposes those policies through the

"policies" -> "modes" 
(this is related to the earlier "use consistent terminology comment" I
will stop highlighting instances)

> filesystem.
> 
> Add the `struct resctrl_kmode_cfg` to define the default mode statically,
> and introduce `resctrl_set_kmode_support()` so architecture code can
> register supported kernel-mode policies during resctrl initialization. The
> `INHERIT_CTRL_AND_MON` policy is supported by default, so architectures
> only need to register any additional modes.

Please keep changelog high level and not describe the code that can be seen from patch.
When doing so, please write in imperative tone.

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v4: New patch to initialize supported kernel-mode's.
>     https://lore.kernel.org/lkml/737a4228-52fb-4583-ac64-8efe79c107e6@intel.com/
>     Changed the kmode_cur to enum resctrl_kernel_mode in resctrl_kmode_cfg.
>     Moved the resctrl_kmode_cfg to filesystem code.
> ---
>  fs/resctrl/internal.h   | 12 ++++++++++++
>  fs/resctrl/rdtgroup.c   | 23 +++++++++++++++++++++++
>  include/linux/resctrl.h | 10 ++++++++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 48af75b9dc85..178126bb2da5 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -314,6 +314,18 @@ struct mbm_state {
>  	u32	prev_bw;
>  };
>  
> +/**
> + * struct resctrl_kmode_cfg - Kernel-mode policy snapshot from architecture
"policy snapshot"? what does this mean? This is not just from architecture either.

> + * @kmode:	Bitmap of supported &enum resctrl_kernel_mode values. Each
> + *		supported mode is represented by BIT(mode).
> + * @kmode_cur:	Currently selected kernel-mode policy.

How about "Active kernel mode"?

> + * @k_rdtgrp:	Resource group backing global-assign modes when applicable;

(stray semi-colon)

> + */
> +struct resctrl_kmode_cfg {
> +	unsigned long			kmode;

How about "kmode_sup" to complement the "kmode_cur"?

To make it obvious this is a bitmap you can use something like:
	DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES)


> +	enum resctrl_kernel_mode	kmode_cur;
> +	struct rdtgroup			*k_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 cc9966ff6cdf..0ee0dfaf9065 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -78,6 +78,16 @@ static void mon_put_kn_priv(void);
>  
>  struct dentry *debugfs_resctrl;
>  
> +/*
> + * Global kernel-mode resctrl policy. Architectures add supported modes before
> + * resctrl is mounted; @kmode_cur tracks the active policy after user selection.

ah - above it is referred to as "active". I find "active" to be more descriptive than
"selected" as highlighted above. The "after user selection" seems distracting.
"@kmode_cur is the active kernel mode" is all that is necessary, no?

> + */
> +static struct resctrl_kmode_cfg resctrl_kcfg = {
> +	.kmode = BIT(INHERIT_CTRL_AND_MON),
> +	.kmode_cur = INHERIT_CTRL_AND_MON,
> +	.k_rdtgrp = NULL
> +};

This initialization needs to be reconsidered to take into account that system may not
support allocation or monitoring. 

Reinette