Re: [PATCH v6 09/18] fs/resctrl: Add interface to display kernel mode status

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

Hi Babu,

On 10/1/26 8:10 AM, Babu Moger wrote:
> Resctrl kernel modes control how allocation and monitoring associations

"allocation and monitoring associations" - unclear what this refers to at this point.

> are applied in kernel mode relative to user mode.
> 
> Add info/kernel_mode to expose the supported and active kernel modes via
> the resctrl filesystem. Supported modes are listed one per line, with the

(needs imperative)

> active mode enclosed in square brackets.
> 
> Document the interface in Documentation/filesystems/resctrl.rst.

Above sentence is not necessary, it is obvious from the patch.

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  Documentation/filesystems/resctrl.rst |  48 +++++++++++++
>  fs/resctrl/rdtgroup.c                 | 100 ++++++++++++++++++++++++++
>  2 files changed, 148 insertions(+)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 73bf89d0ac83..73ccbb9594f5 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -527,6 +527,54 @@ 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" lists the

Note that just above this addition the documentation reads: "Finally, in the
top level of the "info" directory there is a file named "last_cmd_status"."
This addition thus creates a contradiction by adding another file after the
"Finally". Could you please rework that portion of the documentation to
have an introduction for the files in the top level info directory and make
it clear how there are two files in this directory, similar to how the other
directories are documented?

> +	kernel modes supported by the system, one per line, with the active
> +	mode enclosed in square brackets. A kernel mode determines the
> +	allocation and monitoring associations when a task switches from

Would user space know what "allocation and monitoring associations" means?

> +	user mode to kernel mode.
> +
> +	Possible kernel modes are:
> +
> +	"inherit_user":
> +		Kernel mode work uses the allocation and monitoring associations
> +		of the user-space task on whose behalf it runs.
> +
> +	"global_enable_per_cpu":
> +		Kernel mode work uses the allocation and/or monitoring
> +		association of a backing CTRL_MON or MON group. The mode can
> +		be activated or deactivated per CPU. The line contains group=

Having a global "activate"/"deactivate" as well as a per CPU "activate"/"deactivate"
is confusing. The series switches between language here but I find other places
that refer to the global "activate"/"deactivate" and then per CPU "enable"/"disable"
to be easier to follow. Doing so will match the text of the "kmode_cpus" file that
reads "The association is enabled on CPUs added to the mask and disabled on CPUs
removed".

> +		and whichever of ctrl= and mon= the platform supports::
> +
> +		  global_enable_per_cpu:ctrl=<assign|inherit>;mon=<assign|inherit>;group=<CTRL_MON>/<MON>/
> +
> +		"ctrl":
> +			"assign" if kernel mode work uses the allocation
> +			association of the resource group named by "group",
> +			"inherit" if it uses the allocation association of
> +			the user-space task. Present only on systems that
> +			support allocation.

ok - this is already late in the series but this seems to make it clear that
"allocation association" means the portions of resources allocated to this group.
Thus the earlier " enum kmode_state - Control or monitoring association" is wrong?

> +		"mon":
> +			"assign" if kernel mode work uses the monitoring
> +			association of the resource group named by "group",
> +			"inherit" if it uses the monitoring association of
> +			the user-space task. Present only on systems that
> +			support monitoring.
> +		"group":
> +			Path of the resource group, relative to the resctrl
> +			mount point. A CTRL_MON group is shown as <CTRL_MON>//,

Shouldn't it just be <CTRL_MON>/ ?

> +			a MON group as <CTRL_MON>/<MON>/.
> +
> +		When the mode is not active, the parameters are the defaults
> +		used if it is activated without new values.

"parameters are the defaults" seems to conflate two things: (a) the
supported parameters, and (2) the default values of those supported
parameters.

> +
> +	Example of a system that supports allocation and monitoring, with
> +	default mode inherit_user::
> +
> +	  # cat info/kernel_mode
> +	  [inherit_user]
> +	  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 75b81d07a247..e82709d64a00 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -993,6 +993,99 @@ static int rdt_last_cmd_status_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +/* Names used in info/kernel_mode; indexed by enum resctrl_kernel_mode */
> +static const char * const resctrl_kmode_str[] = {
> +	[RESCTRL_INHERIT_USER]		= "inherit_user",
> +	[RESCTRL_GLOBAL_ENABLE_PER_CPU]	= "global_enable_per_cpu"
> +};
> +
> +static_assert(ARRAY_SIZE(resctrl_kmode_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->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 and brackets the active mode. inherit_user
> + * has no parameters. global_enable_per_cpu prints ctrl= only when
> + * allocation is supported and mon= only when monitoring is supported,
> + * then group=.
> + *
> + * 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_grp, *mon_grp;
> +	enum resctrl_kernel_mode mode;
> +	bool active;
> +	char sep;
> +
> +	if (!info_kn_lock(of->kn))
> +		return -ENOENT;
> +
> +	for (mode = 0; mode < RESCTRL_NUM_KERNEL_MODES; mode++) {
> +		if (!test_bit(mode, resctrl_kmode.kmode_sup))
> +			continue;
> +
> +		active = resctrl_kmode.kmode_cur == mode;
> +
> +		seq_printf(seq, active ? "[%s" : "%s", resctrl_kmode_str[mode]);
> +
> +		if (mode == RESCTRL_GLOBAL_ENABLE_PER_CPU) {
> +			ctrl_grp = "";
> +			mon_grp = "";
> +			if (active) {
> +				if (resctrl_kmode.rdtgrp)
> +					resctrl_kmode_group_path(resctrl_kmode.rdtgrp,
> +								 &ctrl_grp, &mon_grp);
> +				else
> +					pr_warn("%s active without a resource group\n",
> +						resctrl_kmode_str[mode]);
> +			}
> +
> +			sep = ':';
> +			if (resctrl_arch_alloc_capable()) {
> +				seq_printf(seq, "%cctrl=%s", sep,
> +					   resctrl_kmode_state_str(resctrl_kmode.ctrl));
> +				sep = ';';
> +			}
> +			if (resctrl_arch_mon_capable()) {
> +				seq_printf(seq, "%cmon=%s", sep,
> +					   resctrl_kmode_state_str(resctrl_kmode.mon));
> +				sep = ';';
> +			}
> +			seq_printf(seq, "%cgroup=%s/%s/", sep, ctrl_grp, mon_grp);

Looks like this will print a monitoring group on a system that does not support
monitoring? I understand that the monitoring group will be the empty string in this
scenario but in resctrl that has meaning of "default monitoring group" so printing
this when monitoring is not supported is unexpected.

I also do not expect the monitoring group to be displayed if "mon=inherit" since
that presents a contradiction with stating that "monitoring group is inherited"
while printing a monitoring group.

> +		}
> +
> +		seq_puts(seq, active ? "]\n" : "\n");
> +	}
> +
> +	info_kn_unlock(of->kn);
> +
> +	return 0;
> +}
> +
>  void *rdt_kn_parent_priv(struct kernfs_node *kn)
>  {
>  	/*
> @@ -1978,6 +2071,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