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

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

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.

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

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



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

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

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

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

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.


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

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

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

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

> + *
> + * Enumeration values are contiguous indices from 0 through
> + * @RESCTRL_KMODE_LAST inclusive. 

Above sentence is not necessary.

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

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


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

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

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

> + * @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?
Should this not be just one mode and can thus have type "enum resctrl_kernel_mode" to make
this obvious?

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

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

Reinette