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

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,

On 9/16/26 00:26, Reinette Chatre wrote:
> 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?

That is correct. Will rephrase 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 :(

My apologies. Thank you for the feedback. I'll address these issues and 
aim for a better version in the next revision.

> 
>>
>> 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.

Again, my apologies. It wasn't intentional. I try to address your 
feedback in every revision, but clearly I'm still missing the mark here.

I'll keep working on it and do my best to improve with each version. :(

Thanks for your patience and continued feedback. Please do not hesitate 
to call it.

>   
>> 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.

Sure.

> 
>>   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.

Sure.

> 
>> + * 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?

Yes. Some of it. Let me shorten it little bit for the context here.

> 
>> + *
>> + * @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?

Yes. Will change it.

> 
>> + * @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.
> 

Sure.

>> + * @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()" ?

Yes, Sure. Will change it to "resctrl_arch_configure_kmode_global()".

> 
>> + * @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"?

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

sure.

> 
>> + *   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.

Yes.

"RESCTRL_GLOBAL_ENABLE_PER_CPU kernel mode."

> 
>> + *   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.

I can add a line about the difference.

> 
>> + * - 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.

Will mention the kernel mode name here.

Thanks
Babu