Re: [PATCH v2 01/16] fs/resctrl: Add kernel mode (kmode) data structures and arch hook

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

Hi Babu,

On 3/12/26 1:36 PM, Babu Moger wrote:
> Add resctrl_kmode, resctrl_kmode_cfg, kernel mode bit defines, and
> resctrl_arch_get_kmode_cfg() for resctrl kernel mode (e.g. PLZA) support.

We should not have to start every series from scratch.
Documentation/process/maintainer-tip.rst. Always.

> ---
>  include/linux/resctrl.h       | 10 ++++++++++
>  include/linux/resctrl_types.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 006e57fd7ca5..2c36d1ac392f 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -699,6 +699,16 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable);
>   */
>  bool resctrl_arch_get_io_alloc_enabled(struct rdt_resource *r);
>  
> +/**
> + * resctrl_arch_get_kmode_cfg() - Get resctrl kernel mode configuration
> + * @kcfg:	Filled with current kernel mode config (kmode, kmode_cur, k_rdtgrp).
> + *
> + * Used by the arch (e.g. x86) to report which kernel mode is active and,
> + * when a global assign mode is in use, which rdtgroup is assigned to
> + * kernel work.
> + */
> +void resctrl_arch_get_kmode_cfg(struct resctrl_kmode_cfg *kcfg);

This interface does not look right. Would it not be resctrl fs that determines
which resource group is assigned? This cannot be set by arch. Why does arch decide
which mode is active? Is this not also resctrl fs? Should arch not just tell
resctrl fs what it supports?

> +
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index a5f56faa18d2..6b78b08eab29 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
> @@ -65,7 +65,37 @@ enum resctrl_event_id {
>  	QOS_NUM_EVENTS,
>  };
>  
> +/**
> + * struct resctrl_kmode - Resctrl kernel mode descriptor
> + * @name:	Human-readable name of the kernel mode.
> + * @val:	Bitmask value for the kernel mode (e.g. INHERIT_CTRL_AND_MON).
> + */
> +struct resctrl_kmode {
> +	char    name[32];
> +	u32     val;
> +};

There is no reason why this needs to be in a central header exposed to archs. Could
this not be a static within the only function that uses it? Something like
rdt_mode_str[]?

> +
> +/**
> + * struct resctrl_kmode_cfg - Resctrl kernel mode configuration
> + * @kmode:	Requested kernel mode.
> + * @kmode_cur:	Currently active kernel mode.
> + * @k_rdtgrp:	Resource control structure in use, or NULL otherwise.
> + */
> +struct resctrl_kmode_cfg {
> +	u32 kmode;
> +	u32 kmode_cur;
> +	struct rdtgroup *k_rdtgrp;
> +};
> +
>  #define QOS_NUM_L3_MBM_EVENTS	(QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
>  #define MBM_STATE_IDX(evt)	((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
>  
> +/* Resctrl kernel mode bits (e.g. for PLZA). */
> +#define INHERIT_CTRL_AND_MON		BIT(0)	/* Kernel uses same CLOSID/RMID as user. */
> +/* One CLOSID for all kernel work; RMID inherited from user. */
> +#define GLOBAL_ASSIGN_CTRL_INHERIT_MON	BIT(1)
> +/* One resource group (CLOSID+RMID) for all kernel work. */
> +#define GLOBAL_ASSIGN_CTRL_ASSIGN_MON	BIT(2)
> +#define RESCTRL_KERNEL_MODES_NUM	3

I think it will make the code much easier to understand if the different modes are described by an
enum. For example, 

	enum resctrl_kernel_modes {
		INHERIT_CTRL_AND_MON,
		GLOBAL_ASSIGN_CTRL_INHERIT_MON,
		GLOBAL_ASSIGN_CTRL_ASSIGN_MON,
		RESCTRL_KMODE_LAST = GLOBAL_ASSIGN_CTRL_ASSIGN_MON
	};
	#define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)

The supported kernel modes can still be managed as a bitmap with intuitive API using the
enum that will make the code easier to read. For example, __set_bit(INHERIT_CTRL_AND_MON, ...)
or BIT(INHERIT_CTRL_AND_MON). The naming is awkward at the moment though, we should improve here.
		
Reinette