Re: [PATCH v5 05/16] x86,fs/resctrl: Introduce architecture hooks to program kernel mode

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

Hi Babu,

On 8/26/26 12:32 PM, Babu Moger wrote:
> Kernel modes defined by enum resctrl_kernel_mode must be applied on
> the CPUs when user space activates, deactivates, or updates a
> configuration.

Not necessarily. This is just what PLZA/RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU
requires, no? The "applied on the CPUs" seems specific to the
RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU mode - hence the name includes
"enable per CPU". Above text implies that all possible kernel modes
require this, we know that upcoming ones don't so this could just be
specific to the only kernel mode that needs it?

> 
> Generic resctrl has no architecture hook to apply these modes across
> a CPU mask when the active mode changes.

I cannot believe this. v3 of this series wrote the changelogs of this
new feature enabling as bugfixes. I asked you several times in v3 to not do this:

https://lore.kernel.org/lkml/2429a51a-92ad-4810-bee9-44bd6fba3443@intel.com/    
https://lore.kernel.org/lkml/57f6324b-6340-4633-b3a0-b40683a5ec12@intel.com/    
https://lore.kernel.org/lkml/10c18df6-d990-4050-bd79-1ca914eee673@intel.com/ 

v4 did not follow that style ... but now this style of presenting enabling
code as bugfix is back in v5! In v2 I already expressed frustration that every
new series seemingly starts from scratch 

https://lore.kernel.org/lkml/57c72d52-e62a-44f6-a08a-891a354058e5@intel.com/

Now a new version seems to forget feedback from just two versions ago :(

> 
> Add resctrl_arch_configure_kmode() to program kernel mode allocation and
> monitoring associations on @cpu_mask. Accept separate assign_ctrl and
> assign_mon parameters so CLOSID and RMID can be assigned independently.

Below is just a sampling from the last two versions of me asking you to not
just verbatim describe the code:

https://lore.kernel.org/lkml/db9c0b3e-184c-4100-b59a-91f6e818fd31@intel.com/ V3 
https://lore.kernel.org/lkml/6273f424-9701-4731-9568-10b3eef8b5fd@intel.com/ V3 
https://lore.kernel.org/lkml/57f6324b-6340-4633-b3a0-b40683a5ec12@intel.com/ V3 
https://lore.kernel.org/lkml/0764a430-f64a-4655-a44f-5c2ff15f2ed7@intel.com/ V4 
https://lore.kernel.org/lkml/0764a430-f64a-4655-a44f-5c2ff15f2ed7@intel.com/ V4 
https://lore.kernel.org/lkml/681e0257-80e0-44c3-b826-20e314a3eb0d@intel.com/ V4 

Again, please do not just verbatim describe what clearly can be seen from the patch.
Use the changelog to describe why the code behaves a certain way. 

Maybe you need this request to come from Boris instead before you start following
the guidance? Here are some examples:

https://lore.kernel.org/all/20240702124524.GEZoP2ZKcTcKl1ca1R@fat_crate.local/  
https://lore.kernel.org/lkml/20250911165433.GBaML-yTUZHkywuJIe@fat_crate.local/ 

From here on the changelogs all seem to have this strange pattern of:
"Architecture needs X"

"Architecture is missing X"

"Verbatim description of X implementation"

Apart from the issues mentioned above this interchangeable repetition turns the
changelogs into a blur. The x86 format for changelogs is described in
Documentation/process/maintainer-tip.rst. Just follow that. This should not be
new to you.

Do not expect further comments on any of the changelogs that follow. I consider
them all unusable.

I clearly demonstrate above that you ignore my feedback. There really seems no
reason for me to provide any. I'll make a final attempt to provide feedback
to *just* the patches (as much as I can without being able to use the changelogs) to
try and help this work make progress.
 
> Implement the x86 hook to program per-CPU PLZA settings. On x86, PLZA
> programs these associations per CPU, with CLOSID and RMID configured
> independently.
> 
> Provide an MPAM stub so the filesystem layer can call the hook on systems
> without PLZA.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 38 +++++++++++++++++++++++
>  drivers/resctrl/mpam_resctrl.c            |  6 ++++

Needs "arm" in subject prefix. 

>  include/linux/resctrl.h                   | 33 ++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index e74f1ed54b86..40fd5e31c94e 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -131,3 +131,41 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable)
>  
>  	return 0;
>  }
> +
> +static void resctrl_kmode_set_one_amd(void *arg)
> +{
> +	union msr_pqr_plza_assoc *plza = arg;
> +
> +	wrmsrq(MSR_IA32_PQR_PLZA_ASSOC, plza->full);
> +}
> +
> +/*
> + * Program Privilege Level Zero Association (PLZA) on @cpu_mask.
> + *

Please follow the custom with function parameters described first, followed by
description.

> + * When @enable is true, kernel mode allocation on @cpu_mask uses @closid from
> + * MSR_IA32_PQR_PLZA_ASSOC if @assign_ctrl is true, otherwise the CLOSID from
> + * MSR_IA32_PQR_ASSOC. Kernel mode monitoring uses @rmid from
> + * MSR_IA32_PQR_PLZA_ASSOC if @assign_mon is true, otherwise the RMID of the
> + * current task.

This just seems to duplicate the description of union msr_pqr_plza_assoc?

> + *
> + * @cpu_mask:	CPUs whose PLZA MSR should be updated.
> + * @closid:	CLOSID to use for kernel mode allocation when @assign_ctrl is true.

Contrary to what the comment states the closid parameter is always programmed, whether
assign_ctrl is true or false. A valid closid is thus expected to always be provided?

> + * @assign_ctrl: Whether PLZA should provide the kernel mode CLOSID.
> + * @rmid:	RMID to use for kernel mode monitoring when @assign_mon is true.

Same comment.

> + * @assign_mon: Whether PLZA should provide the kernel mode RMID.
> + * @enable:	Whether PLZA should provide the kernel mode association.
> + */
> +void resctrl_arch_configure_kmode(const struct cpumask *cpu_mask, u32 closid,
> +				  bool assign_ctrl, u32 rmid,
> +				  bool assign_mon, bool enable)
> +{
> +	union msr_pqr_plza_assoc plza = { 0 };
> +
> +	plza.split.rmid = rmid;
> +	plza.split.rmid_en = assign_mon;
> +	plza.split.closid = closid;
> +	plza.split.closid_en = assign_ctrl;
> +	plza.split.plza_en = enable;
> +
> +	on_each_cpu_mask(cpu_mask, resctrl_kmode_set_one_amd, &plza, 1);
> +}
> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index 9d223057953a..286284ac8423 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -139,6 +139,12 @@ bool resctrl_arch_get_io_alloc_enabled(struct rdt_resource *r)
>  	return false;
>  }
>  
> +void resctrl_arch_configure_kmode(const struct cpumask *cpu_mask, u32 closid,
> +				  bool assign_ctrl, u32 rmid, bool assign_mon,
> +				  bool enable)
> +{
> +}
> +
>  void resctrl_arch_pre_mount(void)
>  {
>  }
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 4245d1e65ccc..8b30eef835aa 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -729,6 +729,39 @@ enum resctrl_kernel_mode {
>  
>  #define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)
>  
> +/**
> + * resctrl_arch_configure_kmode() - Program kernel mode association

resctrl_arch_configure_kmode() implies a generic kernel mode callback but the
parameters are specific to the global, per-CPU mode. I expect that either the
kernel mode self be a parameter or the callback be unique to the kernel mode.
To simplify the parameter management this could be the latter and renamed
to something like "resctrl_arch_configure_kmode_global()"/"resctrl_arch_configure_global_kmode()" ?

> + * @cpu_mask:	CPUs to assign the kernel mode on.
> + * @closid:	CLOSID that matches the RMID to program kernel mode. Depending
> + *		on the architecture, the counter may match traffic of both
> + *		@closid and @rmid, or @rmid only.
> + * @assign_ctrl: true to assign @closid for kernel mode; false to inherit
> + *		association from the user-space task.
> + * @rmid:	RMID to program the kernel mode. Some architectures may use
> + *		CLOSID/RMID separately, others will consider them together.
> + * @assign_mon:	true to assign @rmid for kernel mode; false to inherit
> + *		monitoring association from the user-space task.
> + * @enable:	true to enable kernel mode association on CPUs in @cpu_mask;
> + *		false to disable kernel mode.
> + *
> + * The function can be called in the following scenarios:

"can be" -> "is"?

> + * - If a per-cpu kernel mode is active when user space switches to a new

per-cpu -> per-CPU

> + *   per-cpu kernel mode then resctrl_arch_configure_kmode() will first be

"a new per-cpu kernel mode" - what does this refer to? There is only one
per-CPU kernel mode, no? It may help to refer to the kernel modes explicitly by
their enum value to be clear which modes this callback applies to.

> + *   called to de-activate the active kernel mode on all CPUs that the
> + *   kernel mode is active on.
> + * - When user space switches to a new per-cpu kernel mode then
> + *   resctrl_arch_configure_kmode() is called with cpu_online_mask.
> + * - When user space adds a CPU to an active per-cpu kernel mode.
> + * - When user space removes a CPU from an active per-cpu kernel mode.

Above scenarios all have the "per-cpu kernel mode" in description that
confirms that this callback is dedicated to this single kernel mode and
not actually a generic "enable kernel mode" callback.

Below does not seem to fall under "scenario" like the above but actually
represents a contract between fs and arch that can be separated and
highlighted.

> + * - resctrl fs will always provide the same closid, assign_ctrl, rmid,
> + *   and assign_mon parameters when activating a kernel mode, all

"a kernel mode" -> this callback is not generic so it should be specific to
which modes it applies to.

> + *   interactions (adding/removing CPU) while the kernel mode is active,
> + *   as well as when de-activating the kernel mode.
> + */
> +void resctrl_arch_configure_kmode(const struct cpumask *cpu_mask, u32 closid,
> +				  bool assign_ctrl, u32 rmid, bool assign_mon,
> +				  bool enable);
> +
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  

Reinette