Re: [PATCH v6 04/18] fs/resctrl: Introduce kernel mode enum

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

Hi Babu,

On 10/1/26 8:10 AM, Babu Moger wrote:
> Kernel mode work can use different allocation and monitoring associations

Here I think "different allocation and monitoring associations" means
different CLOSID/RMID, while other places "different allocation and monitoring
associations" means different MSR_IA32_PQR_PLZA_ASSOC.closid_en and
MSR_IA32_PQR_PLZA_ASSOC.rmid_en? 

> than the originating user task. On x86, Privilege Level Zero Association
> (PLZA) enables the kernel to switch to a different CLOSID and/or RMID when
> entering kernel mode.
> 
> Architectures need a common way to name kernel modes before resctrl
> can report what is active or what the platform supports.
> 
> Introduce enum resctrl_kernel_mode:
>   - RESCTRL_INHERIT_USER: Kernel mode work inherits allocation and
>     monitoring from the user task.
>   - RESCTRL_GLOBAL_ENABLE_PER_CPU: Kernel mode work uses allocation and/or
>     monitoring associations that differ from the user task.

As written this is not accurate because there is nothing that prevents
the "kernel mode group" from being the same as a task's own group?

Could the description of RESCTRL_GLOBAL_ENABLE_PER_CPU be made more specific
to include that (a) it is a single (global) association that applies to *all*
user space tasks, and (b) it can be enabled/disabled on each CPU? With these
two additional details reader will more easily understand why the name of this
mode is "RESCTRL_GLOBAL_ENABLE_PER_CPU".

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

...

> ---
>  include/linux/resctrl.h | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 10dfdca7f4bf..f6adff7f4e1f 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -723,6 +723,35 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable);
>   */
>  bool resctrl_arch_get_io_alloc_enabled(struct rdt_resource *r);
>  
> +/**
> + * enum resctrl_kernel_mode - Kernel mode control and monitoring association.
> + *
> + * @RESCTRL_INHERIT_USER:
> + *     Kernel mode inherits both allocation and monitoring associations
> + *     from the user-space task. On x86, the kernel uses the same CLOSID and
> + *     RMID as the user-space task.
> + *
> + * @RESCTRL_GLOBAL_ENABLE_PER_CPU:
> + *     All kernel mode work uses the same allocation and/or monitoring
> + *     association via an assigned CTRL_MON or MON group. The mode can
> + *     be enabled/disabled per-CPU. When the mode is disabled on a CPU
> + *     then the kernel work on that CPU inherits the allocation and
> + *     monitoring association from the user-space task, effectively the
> + *     kernel mode becomes RESCTRL_INHERIT_USER on that CPU.
> + *
> + *     All online CPUs are included by default. A subset may be selected
> + *     through the resctrl group interface, and a CTRL_MON or MON group
> + *     may be assigned to this mode. On x86, CLOSID and RMID are configured

The sentence starting with "A subset ..." repeats what is described in the
first paragraph so it could just be dropped? This leaves the first sentence
of second paragraph out of place, how about merging it into first paragraph
like:

	The mode can be enabled/disabled per-CPU, all online CPUs are enabled
	by default.

> + *     independently; either or both may differ from the user-space task.
> + */
> +enum resctrl_kernel_mode {
> +	RESCTRL_INHERIT_USER,
> +	RESCTRL_GLOBAL_ENABLE_PER_CPU,
> +	RESCTRL_KMODE_LAST = RESCTRL_GLOBAL_ENABLE_PER_CPU
> +};
> +
> +#define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)
> +
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  

Reinette