Re: [PATCH v3 03/12] fs/resctrl: Add kernel mode (kmode) data structures and arch hook

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

Hi Reinette,

On 6/16/26 18:30, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/30/26 4:24 PM, Babu Moger wrote:
>> Privilege-Level Zero Association (PLZA) allows the user to specify a CLOSID
> 
> Subject prefix makes it clear this is a resctrl fs patch so please take care to
> not mix architecture specific terms with resctrl fs generalized support.
> 
> Something that may help here is to consider all resctrl fs changes to be
> relevant from MPAM perspective. Please do so with all resctrl fs changes in
> this series.

Ok. Agreed.

> 
>> and/or RMID associated with execution in Privilege-Level Zero. Introduce a
>> generic enumeration so that architecture and generic code can agree on the
>> available policies.
>>
>> Introduce enum resctrl_kernel_modes with the following values:
> 
> Please make the enum name singular, "resctrl_kernel_modes" -> "resctrl_kernel_mode"
> Doing so will make its use in code easier to parse.

Ok. Sure.

> 
>>
>>    - INHERIT_CTRL_AND_MON: kernel and user tasks share the same CLOSID and
>>      RMID.  This is the default and matches today's resctrl behaviour.
> 
> CLOSID and RMID are x86 terms where the meaning is not 1:1 with other architectures.
> Since this is a new resctrl fs interface it is expected to be usable by all
> architectures. Making this architecture specific is not appropriate.
> 
> These are the modes that are exposed to user space and user space has no insight
> into CLOSID and RMID (ignoring scenario of debugging). I see no reason for
> resctrl do dictate CLOSID/RMID assignment as part of these modes but instead
> what the modes mean should be explained. If it is helpful then any x86 specific
> details can be added by highlighting it is x86 specific. For example,
> 
>       "Kernel work inherits the allocation and monitoring from the user space task.
>        On x86 this means that kernel work shares the same CLOSID and RMID as
>        the user space task."

Sure.

> 
> 
> 
>>
>>    - GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU: a CLOSID is assigned for kernel
>>      work while the RMID used for monitoring is inherited from the running
>>      user task.  The default scope is all online CPUs and may be narrowed to
>>      a subset via the resctrl group interface.  A CTRL_MON group can be
>>      bound to this mode.
> 
> Is binding a CTRL_MON group optional? Consider, for example:
> 
> 	"A CTRL_MON group is bound to this mode."
> 

Ok. Sure

>>
>>    - GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU: both CLOSID and RMID are
>>      assigned to kernel work.  The default scope is all online CPUs and may
>>      be narrowed per CPU via the resctrl group interface.  A CTRL_MON group
>>      can be bound to this mode.
> 
> It should be possible to bind a MON group also, no?

Yes. We can bind either CTRL_MON or MON group to this mode.

Here is the discussion about it.
https://lore.kernel.org/lkml/1d7c79bf-1e40-4db7-8f66-45f234b6d87e@amd.com/

GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU can be either CTRL_MON or MON group.

GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU can only be CTRL_MON.


"A CTRL_MON or MON group is bound to this mode."


> 
>>
>>    - RESCTRL_KMODE_LAST: highest enumerator naming a policy mode.
>>
>>    - RESCTRL_NUM_KERNEL_MODES: number of policy modes; use this to size
>>      static tables indexed by mode.
> 
> The last two can be dropped, this is clear from the patch.

Ok.

> 
>>
>> Also add struct resctrl_kmode_cfg (the snapshot architecture code returns)
>> in include/linux/resctrl_types.h, and declare
>> resctrl_arch_get_kmode_support() in include/linux/resctrl.h so architecture
>> code can advertise the supported modes.
> 
> Above mostly just describes what is clear from the patch. Instead this can summarize
> what the addition does: "Provide callback with which architecture can set the
> kernel modes supported by it". (not exactly what this patch does though, but more below ...)
> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v3: Removed resctrl_kmode definition.
>>      Changed the kernel mode definitions to enum resctrl_kernel_modes.
>>      Used BIT() to set/test the features.
>>      Added details to changelog.
>>
>> v2: New patch to handle PLZA interfaces with /sys/fs/resctrl/info/ directory.
>>      https://lore.kernel.org/lkml/2ab556af-095b-422b-9396-f845c6fd0342@intel.com/
>> ---
>>   include/linux/resctrl.h       | 13 ++++++++++
>>   include/linux/resctrl_types.h | 46 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 59 insertions(+)
>>
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 006e57fd7ca5..ce28418df00f 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -699,6 +699,19 @@ 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_support() - Advertise kernel-mode capabilities
> 
> "Advertise" implies a "broadcast" while the function name is "get" that implies
> retrieval.
> 
> Why does resctrl query the support from the architecture? The typical resctrl initialization
> involves the architecture setting certain capabilities. This simplifies enabling since
> it does not require the addition of this feature to be accompanied with an implementation of
> this call by every architecture.
> 
> Instead, resctrl can just initialize the defaults and an architecture can
> make any adjustments using the optional callback. So, instead of
> resctrl_arch_get_kmode_support(), why not resctrl_set_kmode_support() that is
> implemented in resctrl fs and called by architecture?

Yes. We can do that. We can add resctrl_set_kmode_support() in FS and 
call from architecture.

> 
> When considering the x86 implementation of this it seems as though this implementation
> assumes that all architectures will support inherit_ctrl_and_mon but this is not
> enforced anywhere. Having any assumptions enforced/verified will help to make this
> more robust. The fs/arch separation depending on so many architectures
> "doing the right thing" seems risky.

ok.

> 
> 
>> + * @kcfg:	Architecture ORs BIT() flags into @kcfg->kmode for each supported
>> + *		&enum resctrl_kernel_modes value (see &struct resctrl_kmode_cfg).
>> + *
>> + * Used for optional features (for example PLZA on x86) that can assign CLOSID
>> + * and/or RMID to kernel work separately from user tasks.  Generic code compares
>> + * @kcfg->kmode with the effective @kcfg->kmode_cur; when a global-assign mode is
>> + * active, @kcfg->k_rdtgrp identifies the active &struct rdtgroup. The default mode
> 
> Does the architecture need to know these implementation details?

Not required.

> 
>> + * is INHERIT_CTRL_AND_MON and group is default group.
>> + */
>> +void resctrl_arch_get_kmode_support(struct resctrl_kmode_cfg *kcfg);
> 
> Why does architecture need to know the layout of struct resctrl_kmode_cfg? It only needs

Arch does not need to know the resctrl_kmode_cfg.

> to share the modes it supports and need not be concerned with any of the internals - from
> what I can tell the hook to program the kernel mode does not use this structure either and
> this is the only "outside of resctrl fs" usage and it does not seem necessary.

We can just pass the modes to resctrl_set_kmode_support() from arch to 
set it.

> 
>> +
>>   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..3aba07764b99 100644
>> --- a/include/linux/resctrl_types.h
>> +++ b/include/linux/resctrl_types.h
> 
> Please keep in mind that resctrl_types.h is reserved for those types that an architecture
> needs to use in its asm/resctrl.h ... it does not look like any of the types added here qualify.

I will move it to include/linux/resctrl.h ad both ARCH and FS need to 
know about these modes.

> 
>> @@ -68,4 +68,50 @@ enum resctrl_event_id {
>>   #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)
>>   
>> +/**
>> + * enum resctrl_kernel_modes - Kernel versus user CLOSID/RMID policy
> 
> What does "versus user" mean? Can this be dropped?

ok.

> 
>> + *
>> + * Enumeration values are contiguous indices from 0 through
>> + * @RESCTRL_KMODE_LAST inclusive.
> 
> Above sentence is not necessary.

ok.

> 
>> Global-assign modes treat all online CPUs as
>> + * in scope by default; a subset of CPUs may be selected by using resctrl
>> + * group's interface.
>> + *
>> + * @INHERIT_CTRL_AND_MON:
>> + *	User and kernel tasks use the same CLOSID and RMID.
> 
> Similar comment as earlier. Since this is generic resctrl fs interface it needs to
> be applicable to all architectures. For example (same suggestion as earlier),
>    "Kernel work inherits the allocation and monitoring of the user space task.

ok.

> 
>> + * @GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU:
>> + *	A CLOSID may be assigned for kernel work while RMID selection for
> 
> "may be assigned" - this is not optional, right? How about "A control group is assigned ..."

ok.

> 
> 
>> + *	monitoring follows the same inheritance rules as for user contexts.
>> + *	Default scope is all online CPUs: subset of CPUs may be selected by
>> + *	using resctrl group's interface.
>> + * @GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU:
>> + *	A single resource group (CLOSID and RMID together) may be assigned to
> 
> "may be" -> "is" ?

ok.


>> + *	kernel work. Default scope is all online CPUs: subset of CPUs may be
>> + *	selected by using resctrl group's interface.
>> + * @RESCTRL_KMODE_LAST:
> 
> Documenting @RESCTRL_KMODE_LAST is not necessary.
> 
>> + *	Highest enumerator that names a policy mode. Use RESCTRL_NUM_KERNEL_MODES
>> + *	to size static tables indexed by mode.
> 
> No need to document this.

ok.

> 
>> + */
>> +enum resctrl_kernel_modes {
>> +	INHERIT_CTRL_AND_MON,
>> +	GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU,
>> +	GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU,
>> +	RESCTRL_KMODE_LAST = GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU,
>> +};
>> +
>> +#define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)
>> +
>> +/**
>> + * struct resctrl_kmode_cfg - Kernel-mode policy snapshot from architecture
> 
> Only @kmode is initialized from the architecture. The rest is managed by resctrl fs.
> I do not see why architecture needs to know the structure details.

Correct. Will move this to FS.

> 
>> + * @kmode:	Hardware- or policy-supported modes: each enumerator from
>> + *		&enum resctrl_kernel_modes is represented by BIT(mode index).
>> + * @kmode_cur:	Effective mode(s) in the same BIT(index) form as @kmode.
> 
> "mode(s)" ... this is plural implying more than one mode can be active at a time?

no.

> Should this not be just one mode and can thus have type "enum resctrl_kernel_mode" to make
> this obvious?

Yes.

> 
>> + * @k_rdtgrp:	Resource group backing global-assign modes when applicable;
>> + *		initialized to the default group at boot.
> 
> Why is this initialized to default group at boot? I believe inherit_ctrl_and_mon is
> the default mode and it does not have a group so should this not be NULL by default?

Yes. It should be NULL at boot. Will change it.

> 
>> + */
>> +struct resctrl_kmode_cfg {
>> +	u32 kmode;
>> +	u32 kmode_cur;
>> +	struct rdtgroup *k_rdtgrp;
> 
> Please align struct members in tabular fashion.
> 
> Not specific to this patch: After so many contributions to resctrl I am very surprised how
> this series does not respect Documentation/process/maintainer-tip.rst in many ways. For example,
> later patches at some point just stops writing changelogs in imperative tone and just
> documents what the code does, patches document locking requirements instead of using code
> like lockdep_assert_held(), variables are not declared in reverse fir, changelogs refer to
> other patches in series. Following Documentation/process/maintainer-tip.rst should be
> very familiar by now.

My bad. Yes. Will focus on process in next revision.

Thanks
Babu