Re: [PATCH v5 06/16] fs/resctrl: Introduce kernel mode states for resctrl

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

Hi Babu,

On 8/26/26 12:32 PM, Babu Moger wrote:

> ---
>  fs/resctrl/internal.h | 51 +++++++++++++++++++++++++++++++++++++++++++
>  fs/resctrl/rdtgroup.c | 33 ++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index e62a277dee85..4087bf44a06d 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -314,6 +314,57 @@ struct mbm_state {
>  	u32	prev_bw;
>  };
>  
> +/**
> + * enum kmode_state - Control or monitoring state for a kernel mode
> + * @KMODE_INHERIT: Inherit from the user space task.
> + * @KMODE_ASSIGN: Use a global assignment for kernel mode.
> + */
> +enum kmode_state {
> +	KMODE_INHERIT,
> +	KMODE_ASSIGN
> +};
> +
> +/**
> + * struct resctrl_kmode_caps - Static kernel mode capabilities
> + * @kmode_sup:	Bitmap of supported kernel modes. Empty when neither
> + *		@ctrl_en nor @mon_en is set and kernel mode policy is
> + *		unavailable on this system.
> + * @ctrl_en:	Whether kernel mode may use global assignment for control.
> + * @mon_en:	Whether kernel mode may use global assignment for monitoring.

Why is ctrl_en and mon_en needed? It seems to just store the output of
whether system supports allocation and monitoring. I only see these used
when user interacts reads or writes the kernel mode so not a "hot path" that
needs to be optimized. Can these just be dropped and just use resctrl_arch_alloc_capable()
and resctrl_arch_mon_capable() directly? Please note they are in process of
being changed/renamed:

https://lore.kernel.org/lkml/20260831174421.13921-7-tony.luck@intel.com/

> + */
> +struct resctrl_kmode_caps {
> +	DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES);
> +	bool				ctrl_en;
> +	bool				mon_en;
> +};
> +
> +/**
> + * struct resctrl_kmode_active - Runtime kernel mode state
> + * @kmode_cur:	Currently selected kernel mode.
> + * @ctrl_mode:	Control assignment state when kernel mode is active.
> + * @mon_mode:	Monitoring assignment state when kernel mode is active.
> + * @k_rdtgrp:	Resource group backing global assignment mode.
> + *
> + * When @kmode_cur is %RESCTRL_INHERIT_USER, assignment state is ignored and
> + * @k_rdtgrp is %NULL.

This implies that this is only the state for RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU.
If it is made specifically so there is no need to pretend it is generic and
add all these caveats.

> + */
> +struct resctrl_kmode_active {
> +	enum resctrl_kernel_mode	kmode_cur;
> +	enum kmode_state		ctrl_mode;
> +	enum kmode_state		mon_mode;

Is "mode" accurate? This is all about kernel "mode" and now control and
monitoring have other modes?

> +	struct rdtgroup			*k_rdtgrp;

Could naming be consistent? Consider, for example, kmode_rdtgrp? Although
if this struct can be specific to the global per-CPU kernel mode then it can
just be "rdtgrp".

> +};
> +
> +/**
> + * struct resctrl_kmode_cfg - Global kernel mode state
> + * @caps:	Supported modes and assignment capabilities.
> + * @active:	Active mode, assignment state, and assigned group.

Please do not list the struct members as part of its description elsewhere since
that will be difficult to keep accurate. Just describe what the struct represents.

> + */
> +struct resctrl_kmode_cfg {
> +	struct resctrl_kmode_caps	caps;
> +	struct resctrl_kmode_active	active;
> +};
> +
>  extern struct mutex rdtgroup_mutex;
>  
>  static inline const char *rdt_kn_name(const struct kernfs_node *kn)
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 5dcbb0a964e8..3c53f3f74e5a 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -76,6 +76,13 @@ static void rdtgroup_destroy_root(void);
>  
>  struct dentry *debugfs_resctrl;
>  
> +/*
> + * Global kernel mode policy state: supported modes, active mode, assignment
> + * capabilities, assignment state, and the resource group selected for a global
> + * assignment.

Same here - please do not just provide a list of the struct's members. A high level
description instead.

This code is really strange. This whole series is difficult to read. I have not seen
these styles used before and surprised that it comes from you.

> + */
> +static struct resctrl_kmode_cfg resctrl_kcfg;

I think the code will be easier to read if "resctrl_kcfg" -> "resctrl_kmode".

> +
>  /*
>   * Memory bandwidth monitoring event to use for the default CTRL_MON group
>   * and each new CTRL_MON group created by the user.  Only relevant when
> @@ -2297,6 +2304,30 @@ static void io_alloc_init(void)
>  	}
>  }
>  
> +/*
> + * Initialize kernel mode policy defaults from architecture capabilities.
> + *
> + * When ctrl_en or mon_en is set, RESCTRL_INHERIT_USER is supported and
> + * selected as the initial active mode. When neither is set, kmode_sup is
> + * left empty, kernel mode policy is unavailable, and kmode_cur remains at
> + * its zero-initialized default (RESCTRL_INHERIT_USER) but is unused.

Above just verbatim describes the code. Please provide higher level why the
code does what it does.

> + */
> +static void resctrl_kmode_init(void)
> +{
> +	resctrl_kcfg.caps.ctrl_en = resctrl_arch_alloc_capable();
> +	resctrl_kcfg.active.ctrl_mode = KMODE_INHERIT;
> +	resctrl_kcfg.caps.mon_en = resctrl_arch_mon_capable();
> +	resctrl_kcfg.active.mon_mode = KMODE_INHERIT;
> +	resctrl_kcfg.active.k_rdtgrp = NULL;
> +
> +	if (resctrl_kcfg.caps.ctrl_en || resctrl_kcfg.caps.mon_en) {
> +		resctrl_kcfg.active.kmode_cur = RESCTRL_INHERIT_USER;
> +		__set_bit(RESCTRL_INHERIT_USER, resctrl_kcfg.caps.kmode_sup);
> +	} else {
> +		bitmap_zero(resctrl_kcfg.caps.kmode_sup, RESCTRL_NUM_KERNEL_MODES);
> +	}
> +}
> +
>  void resctrl_file_fflags_init(const char *config, unsigned long fflags)
>  {
>  	struct rftype *rft;
> @@ -4816,6 +4847,8 @@ int resctrl_init(void)
>  	if (ret)
>  		return ret;
>  
> +	resctrl_kmode_init();
> +
>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>  	if (ret) {
>  		resctrl_l3_mon_resource_exit();

Reinette