Re: [PATCH v5 09/16] fs/resctrl: Add interface to display supported and active kernel modes

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

Hi Babu,

On 8/26/26 12:32 PM, Babu Moger wrote:
> ---
>  Documentation/filesystems/resctrl.rst |  29 +++++++
>  fs/resctrl/rdtgroup.c                 | 115 ++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index f3e941404967..c6e8cf828e18 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -528,6 +528,35 @@ conveyed in the error returns from file operations. E.g.
>  	# cat info/last_cmd_status
>  	mask f7 has non-consecutive 1-bits
>  
> +"kernel_mode":
> +	In the top level of the "info" directory, "kernel_mode" reports
> +	supported and active kernel modes available on the system.
> +
> +	Reading the file lists one mode per line. The active mode is wrapped in
> +	square brackets. inherit_user is shown without options.
> +	assign_global_enable_per_cpu is shown as::
> +
> +	  assign_global_enable_per_cpu:ctrl=<assign|inherit>;mon=<assign|inherit>;group=<ctrl>/<mon>/
> +
> +	- inherit_user: inherit allocation and monitoring from the user task.

To make this easier to read, instead of mixing the modes, could you please have text like:
	
	Possible kernel modes are:
	
	<description of inherit_user>

	<description of assign_global_enable_per_cpu>

In addition to being easier to read something like above will also be easier to expand.

> +	- assign_global_enable_per_cpu: kernel mode may use separate
> +	  allocation and/or monitoring associations. ctrl= and mon= show
> +	  whether each is assigned or inherited, and group= identifies the

Earlier is "ctrl=<assign|inherit>;mon=<assign|inherit>" and above is
"ctrl= and mon= show whether each is assigned or inherited". This is rewrites
the same thing in different ways without helping to explain what the parameters
and their values mean.


> +	  associated resource group using <CTRL_MON>/<MON>/ path syntax.

This text is difficult to parse. It switches between "assigned" and "associated"
*in the same sentence* when referring to the same thing. Please use consistent terms
and just be specific about what this actually by avoiding this vague language. 

> +
> +	Only supported modes are listed. On an inactive
> +	assign_global_enable_per_cpu line, ctrl= and mon= show platform
> +	capability defaults rather than the last active assignment, and

I find the text to be very vague. Above could be moved to section dedicated to
the assign_global_enable_per_cpu mode and be made specific. For example,

	When inactive, the parameter values displayed are the default values
	that will be used when no new value is provided during enabling/activation(*) 
	
	(*) pick a term and stick with it

> +	group=// is shown. When the mode is active, ctrl= and mon= report
> +	the current assignment state and group= identifies the associated
> +	resource group.

Last sentence just repeats the paragraph above, no?


> +
> +	Example::
> +
> +	  # cat info/kernel_mode
> +	  [inherit_user]
> +	  assign_global_enable_per_cpu:ctrl=assign;mon=assign;group=//
> +
>  Resource alloc and monitor groups
>  =================================
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 3bb0e3a203ac..992af586a194 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1010,6 +1010,114 @@ static int rdt_last_cmd_status_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +/* Sysfs lines for info/kernel_mode; indexed by enum resctrl_kernel_mode */
> +static const char * const resctrl_mode_str[] = {

This is incredibly close to resctrl's existing rdtgroup_mode_str while being
very high level. Could it be renamed to be more specific to this feature?
For example, resctrl_kmode_str[]?

> +	[RESCTRL_INHERIT_USER]	=			"inherit_user",
> +	[RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU] =	"assign_global_enable_per_cpu"

As mentioned earlier I am starting to think that the "assign" in the name
is not necessary and creates confusion with the "assign" values of its
parameters.

> +};
> +
> +static_assert(ARRAY_SIZE(resctrl_mode_str) == RESCTRL_NUM_KERNEL_MODES);
> +
> +static const char *resctrl_kmode_state_str(enum kmode_state state)
> +{
> +	return state == KMODE_ASSIGN ? "assign" : "inherit";
> +}
> +
> +static void resctrl_kmode_group_path(struct rdtgroup *rdtgrp,
> +				     const char **ctrl, const char **mon)
> +{
> +	*ctrl = "";
> +	*mon = "";
> +
> +	if (!rdtgrp)
> +		return;
> +
> +	if (rdtgrp->type == RDTMON_GROUP) {
> +		*ctrl = rdt_kn_name(rdtgrp->mon.parent->kn);
> +		*mon = rdt_kn_name(rdtgrp->kn);
> +	} else {
> +		*ctrl = rdt_kn_name(rdtgrp->kn);
> +	}
> +}
> +
> +/**
> + * resctrl_kernel_mode_show() - Display supported and active kernel modes
> + * @of: kernfs open file
> + * @seq: output seq_file
> + * @v: unused
> + *
> + * Lists one line per mode set in resctrl_kcfg.caps.kmode_sup. Brackets the
> + * active mode. inherit_user is shown without any options.
> + * assign_global_enable_per_cpu is shown as:
> + *
> + *   assign_global_enable_per_cpu:ctrl=<assign|inherit>;mon=<assign|inherit>;\
> + *					group=<ctrl>/<mon>/
> + *
> + * When assign_global_enable_per_cpu is inactive, ctrl=assign and mon=assign
> + * reflect resctrl_kcfg.caps.ctrl_en and resctrl_kcfg.caps.mon_en, and
> + * group=//. When active, assign state comes from resctrl_kcfg.active.ctrl_mode
> + * and resctrl_kcfg.active.mon_mode.

The code is much easier to read than above summary of it.

> + *
> + * Return: 0 on success, or -ENOENT on error.
> + */
> +static int resctrl_kernel_mode_show(struct kernfs_open_file *of,
> +				    struct seq_file *seq, void *v)
> +{
> +	const char *ctrl_state, *mon_state;
> +	enum resctrl_kernel_mode mode;
> +	struct rdtgroup *rdtgrp;
> +	const char *ctrl, *mon;
> +	bool active;
> +	int ret = 0;
> +
> +	if (!info_kn_lock(of->kn))
> +		return -ENOENT;
> +
> +	for (mode = 0; mode < RESCTRL_NUM_KERNEL_MODES; mode++) {
> +		if (!test_bit(mode, resctrl_kcfg.caps.kmode_sup))
> +			continue;
> +
> +		active = (resctrl_kcfg.active.kmode_cur == mode);
> +
> +		if (mode == RESCTRL_INHERIT_USER) {
> +			seq_printf(seq, active ? "[%s]\n" : "%s\n",
> +				   resctrl_mode_str[mode]);
> +			continue;
> +		}
> +
> +		if (active) {
> +			ctrl_state = resctrl_kmode_state_str(resctrl_kcfg.active.ctrl_mode);
> +			mon_state = resctrl_kmode_state_str(resctrl_kcfg.active.mon_mode);
> +			rdtgrp = resctrl_kcfg.active.k_rdtgrp;
> +			if (WARN_ON(!rdtgrp)) {

Just pr_warn() is sufficient and avoids the discussion about panic_on_warn kernels.

> +				rdt_last_cmd_puts("Invalid kernel mode group\n");
> +				ret = -ENOENT;
> +				goto out_unlock;
> +			}
> +			resctrl_kmode_group_path(rdtgrp, &ctrl, &mon);
> +		} else {
> +			ctrl_state = resctrl_kcfg.caps.ctrl_en ? "assign" : "inherit";

This does not look right. As I understand ctrl_en represents whether the system supports
allocation or not. So above means that if system supports allocation then the allocation
state is "assign" while a system that does *not* support allocation inherits *allocation*
association from user space?

> +			mon_state = resctrl_kcfg.caps.mon_en ? "assign" : "inherit";
> +			ctrl = "";
> +			mon = "";
> +		}
> +
> +		if (active) {

Why are there two separate "if (active)" blocks? 

> +			seq_printf(seq, "[%s:ctrl=%s;mon=%s;group=%s/%s/]\n",

This does not look right. The reason why this version moved to a single global mode
with parameters is to be able to support systems that do not support allocation or
monitoring. Having these separate parameters thus enables resctrl to only show the
"ctrl=" parameter on a system that only supports allocation and only show the
"mon=" parameter on a system that only supports monitoring. Above just keeps showing
both whether system supports allocation/monitoring or not.

> +				   resctrl_mode_str[mode], ctrl_state, mon_state,
> +				   ctrl, mon);
> +		} else {
> +			seq_printf(seq, "%s:ctrl=%s;mon=%s;group=%s/%s/\n",
> +				   resctrl_mode_str[mode], ctrl_state, mon_state,
> +				   ctrl, mon);
> +		}
> +	}
> +
> +out_unlock:
> +	info_kn_unlock(of->kn);
> +	return ret;
> +}
> +
>  void *rdt_kn_parent_priv(struct kernfs_node *kn)
>  {
>  	/*
> @@ -1995,6 +2103,13 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= rdt_last_cmd_status_show,
>  		.fflags		= RFTYPE_TOP_INFO,
>  	},
> +	{
> +		.name		= "kernel_mode",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= resctrl_kernel_mode_show,
> +		.fflags		= RFTYPE_TOP_INFO,
> +	},
>  	{
>  		.name		= "mbm_assign_on_mkdir",
>  		.mode		= 0644,

Reinette