Re: [RESEND PATCH v4 13/15] fs/resctrl: Add interface to modify kernel-mode via info/kernel_mode

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

Hi Babu,

On 7/7/26 2:50 PM, Babu Moger wrote:
> info/kernel_mode reports which kernel-mode policies the platform supports
> and which one is active, but it is read-only and does not show the rdtgroup
> bound to the active kernel-mode policy.
> 
> User space needs both pieces to manage kernel-mode assignment: it must be
> able to select a policy and bind the global-assign modes to a specific
> resctrl group, and it must be able to read back which group is currently
> bound.
> 
> Make info/kernel_mode writable and extend the read format to identify the

"Make info/kernel_mode writable and extend the read format" - what does this mean?

> bound group for active global-assign modes.  Inactive global-assign modes
> are reported as "group=uninitialized"; INHERIT has no group suffix because
> it does not bind an rdtgroup.  Square brackets mark the active mode on read
> only and must not be included when writing.
> 
> Document the interface in Documentation/filesystems/resctrl.rst.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  Documentation/filesystems/resctrl.rst |  80 +++++++++
>  fs/resctrl/rdtgroup.c                 | 238 +++++++++++++++++++++++++-
>  2 files changed, 315 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index f3e941404967..5a13814d1325 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -528,6 +528,86 @@ 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" controls how
> +	resource allocation and monitoring work in kernel mode. This is used on
> +	some platforms to assign dedicated allocation and/or monitoring to
> +	kernel-mode work.

"assign dedicated allocation and/or monitoring to kernel-mode work" - I do not understand
what this means.

> +
> +	Reading the file lists supported kernel modes, one per line.  The
> +	currently active mode is wrapped in square brackets for display only.
> +
> +	The modes are displayed in the following format:
> +	"<mode>:group=<ctrl>/<mon>/"

Please no! We know that resctrl needs to support more kernel modes and we know that their
configuration interfaces will be different from what is needed to support AMD. They will
introduce new modes to "kernel_mode" but those modes may bring new sytax to kernel_mode
associated with those modes and/or introduce new files associated with those modes (like kmode_cpus
for these modes). Writing something like above derails the plans to support those future kernel modes.

Instead, please write something like: "The display format depends on the kernel mode."
followed by each supported kernel mode (nothing that there may be more) and what *that*
kernel mode's display format looks like. Please do not enforce PLZA display format on
this and all future kernel modes.


> +
> +	The inherit_ctrl_and_mon mode is shown as "<mode>" with no ":group=" suffix.
> +	Inactive global-assign modes report "group=uninitialized".
> +
> +	The bound group path uses empty components when they do not apply:
> +
> +	"//":
> +		The default (root) control group.
> +	"<ctrl>//":
> +		The control group named <ctrl>.
> +	"/<mon>/":
> +		The monitor group named <mon> under the default control group.
> +	"<ctrl>/<mon>/":
> +		The monitor group named <mon> under the control group named <ctrl>.
> +
> +	Modes:
> +
> +	- "inherit_ctrl_and_mon": Kernel work inherits allocation and monitoring
> +	  from the current user-space task (default).
> +	- "global_assign_ctrl_inherit_mon_per_cpu": A single allocation is
> +	  assigned for all kernel work; monitoring is still inherited from user
> +	  space.  Requires a CTRL_MON group.
> +	- "global_assign_ctrl_assign_mon_per_cpu": A single resource group
> +	  supplies both allocation and monitoring for all kernel work.  May be
> +	  a CTRL_MON or MON group.
> +
> +	Only modes supported by the platform are listed on read.
> +	Example::
> +
> +	  # mount -t resctrl resctrl /sys/fs/resctrl
> +	  # cd /sys/fs/resctrl
> +	  # cat info/kernel_mode
> +	  [inherit_ctrl_and_mon]
> +	  global_assign_ctrl_inherit_mon_per_cpu:group=uninitialized
> +	  global_assign_ctrl_assign_mon_per_cpu:group=uninitialized
> +
> +	The modes and binding can be modified by writing to the interface. Writing
> +	one line (terminated by a newline) selects the active mode and binds it to
> +	a resctrl group.
> +
> +	Writes must follow the format:
> +	"<mode>:group=<ctrl>/<mon>/"

Please no. Here also, please replace with something like: "Write format depends
on the kernel mode." Followed by a list of supported kernel modes and description of
what format each mode uses.

> +
> +	The ":group=<spec>" suffix is optional; when omitted the default (root)
> +	control group is used for global-assign modes. Selecting a new mode or group
> +	tears down any active global-assign binding before programming the new one,
> +	including when switching between global-assign modes on the same group.
> +	The inherit_ctrl_and_mon mode ignores any supplied group and clears the
> +	active kernel-mode binding.  The mode must match one of the supported names
> +	exactly, and modes not advertised by the platform cannot be set. The
> +	display-only "group=uninitialized" form is rejected. Errors are reported in
> +	"info/last_cmd_status".
> +	Example::
> +
> +	  # mkdir ctrl1
> +	  # echo "global_assign_ctrl_assign_mon_per_cpu:group=ctrl1//" \
> +	         > info/kernel_mode
> +
> +	  # cat info/kernel_mode
> +	  inherit_ctrl_and_mon
> +	  global_assign_ctrl_inherit_mon_per_cpu:group=uninitialized
> +	  [global_assign_ctrl_assign_mon_per_cpu:group=ctrl1//]
> +
> +	  # echo "inherit_ctrl_and_mon" > info/kernel_mode
> +	  # cat info/kernel_mode
> +	  [inherit_ctrl_and_mon]
> +	  global_assign_ctrl_inherit_mon_per_cpu:group=uninitialized
> +	  global_assign_ctrl_assign_mon_per_cpu:group=uninitialized
> +
>  Resource alloc and monitor groups
>  =================================
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index c537846d9264..7b06c3b3f00e 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1059,7 +1059,8 @@ static_assert(ARRAY_SIZE(resctrl_mode_str) == RESCTRL_NUM_KERNEL_MODES);
>   * @v: unused
>   *
>   * Displays one line per mode set in resctrl_kcfg.kmode. Bracket the active
> - * policy (resctrl_kcfg.kmode_cur).
> + * policy (resctrl_kcfg.kmode_cur). Square brackets are display-only; writes
> + * to info/kernel_mode must not include them.
>   *
>   * INHERIT_CTRL_AND_MON is displayed as "[inherit_ctrl_and_mon]" when active
>   * or "inherit_ctrl_and_mon" when supported but inactive, with no :group=
> @@ -1068,7 +1069,7 @@ static_assert(ARRAY_SIZE(resctrl_mode_str) == RESCTRL_NUM_KERNEL_MODES);
>   * Global-assign modes append :group=. An inactive mode is emitted as
>   * "<mode>:group=uninitialized". An active mode with a bound group is emitted
>   * as "[<mode>:group=<ctrl>/<mon>/]", where <ctrl>/<mon>/ is derived from
> - * resctrl_kcfg.k_rdtgrp.
> + * resctrl_kcfg.k_rdtgrp. The bracketed form is not accepted on write.
>   *
>   * Return: 0 on success, or -ENOENT on error.
>   */
> @@ -1164,6 +1165,8 @@ static void resctrl_kmode_files_set_visible(struct rdtgroup *rdtgrp, bool visibl
>   * @kmode:	Kernel-mode policy currently active on @rdtgrp.
>   *
>   * Reset the kernel-mode binding on the CPUs in @rdtgrp's @kmode_cpu_mask.
> + * Called from resctrl_kernel_mode_write() whenever an active global-assign
> + * policy is replaced, including a mode change on the same group.
>   */
>  static void rdtgroup_config_kmode_reset(struct rdtgroup *rdtgrp,
>  					enum resctrl_kernel_mode kmode)
> @@ -1212,6 +1215,234 @@ static void rdtgroup_kmode_detach(struct rdtgroup *rdtgrp)
>  	resctrl_kcfg.kmode_cur = INHERIT_CTRL_AND_MON;
>  }
>  
> +/**
> + * rdtgroup_config_kmode() - Push @rdtgrp's kernel CLOSID/RMID to hardware
> + * @rdtgrp:	Resctrl group whose CLOSID/RMID should be programmed.
> + * @kmode:	Kernel-mode policy to program for @rdtgrp.
> + *
> + * @rdtgrp carries the CLOSID/RMID to program. For monitor groups, the CLOSID

I just cannot get used to this writing. It is not coherent. It reads like text
from several people were just concatenated into this series.

> + * matches the parent control group while the RMID belongs to the monitor group.
> + *
> + * The caller (resctrl_kernel_mode_write()) is responsible for validating that
> + * the (kmode, group type) pair is permitted before invoking this helper.
> + * This helper records the current online CPUs in @rdtgrp->kmode_cpu_mask and
> + * programs those CPUs with @rdtgrp's CLOSID/RMID.

Please do not describe the code in such detail - this detail can be seen from the
code self. Please make the description high level. Please check whole series.

> + */
> +static void rdtgroup_config_kmode(struct rdtgroup *rdtgrp, enum resctrl_kernel_mode kmode)
> +{
> +	bool assign_mon = (kmode == GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU);
> +
> +	/* A new binding starts with all currently online CPUs in scope. */
> +	cpumask_copy(&rdtgrp->kmode_cpu_mask, cpu_online_mask);
> +
> +	resctrl_arch_configure_kmode(&rdtgrp->kmode_cpu_mask, rdtgrp->closid,
> +				     rdtgrp->mon.rmid, assign_mon, true);
> +
> +	rdtgrp->kmode = true;
> +	resctrl_kmode_files_set_visible(rdtgrp, true);
> +}
> +
> +/**
> + * rdtgroup_by_kmode_path() - Resolve a "<ctrl>/<mon>/" path to an rdtgroup
> + * @ctrl_name:	Control-group name, or "" for the default control group.
> + * @mon_name:	Monitor-group name, or "" to select the control group itself.
> + *
> + * Matches the path syntax emitted by resctrl_kernel_mode_show():
> + *   "//"            - the default control group
> + *   "<ctrl>//"      - control group @ctrl_name
> + *   "/<mon>/"       - monitor group @mon_name under the default control group
> + *   "<ctrl>/<mon>/" - monitor group @mon_name under control group @ctrl_name
> + *
> + * An empty @ctrl_name selects &rdtgroup_default. Otherwise @ctrl_name must
> + * match an existing control group. If @mon_name is empty, the selected control
> + * group is returned; otherwise @mon_name is looked up in the selected control
> + * group's monitor children.
> + *
> + * Return: Pointer to the matching rdtgroup, or NULL if no such group exists.
> + */
> +static struct rdtgroup *rdtgroup_by_kmode_path(const char *ctrl_name,
> +					       const char *mon_name)
> +{
> +	struct rdtgroup *rdtg, *parent = &rdtgroup_default;
> +
> +	if (*ctrl_name) {
> +		parent = NULL;
> +		list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
> +			if (rdtg->type != RDTCTRL_GROUP)
> +				continue;

All groups in rdt_all_groups are control groups, no?

> +			if (!strcmp(rdt_kn_name(rdtg->kn), ctrl_name)) {
> +				parent = rdtg;
> +				break;
> +			}
> +		}
> +	}
> +	if (!parent)
> +		return NULL;
> +
> +	if (!*mon_name)
> +		return parent;
> +
> +	list_for_each_entry(rdtg, &parent->mon.crdtgrp_list, mon.crdtgrp_list)
> +		if (!strcmp(rdt_kn_name(rdtg->kn), mon_name))
> +			return rdtg;
> +	return NULL;
> +}
> +
> +/**
> + * resctrl_kernel_mode_write() - Select kernel mode and bind group via info/kernel_mode
> + * @of:		kernfs file handle.
> + * @buf:	One line of the form "<mode>[:group=<ctrl>/<mon>/]"; must end
> + *		with a newline. Do not include the square brackets used to mark
> + *		the active mode in resctrl_kernel_mode_show(). Leading and trailing
> + *		whitespace is ignored, as is whitespace between the mode name and
> + *		an optional ":group=" suffix. The ":group=<spec>" suffix is
> + *		optional; when omitted the default control group
> + *		(&rdtgroup_default) is used.
> + * @nbytes:	Length of @buf.
> + * @off:	File offset (unused).
> + *
> + * Parses @buf, validates that <mode> is listed in resctrl_mode_str[] and is
> + * supported by the platform (resctrl_kcfg.kmode), resolves <ctrl>/<mon>/ to
> + * an existing rdtgroup (or picks &rdtgroup_default if no group was specified),
> + * treats INHERIT as an unbound mode, tears down any active global-assign
> + * binding via rdtgroup_config_kmode_reset(), programs hardware via
> + * rdtgroup_config_kmode() when the new mode is not INHERIT_CTRL_AND_MON, and
> + * on success updates resctrl_kcfg.k_rdtgrp and resctrl_kcfg.kmode_cur. The
> + * display-only "group=uninitialized" form is rejected.  Errors are reported
> + * in last_cmd_status.

This is just a summary of the code? 

> + *
> + * Return: @nbytes on success, negative errno with last_cmd_status set on error.
> + */
> +static ssize_t resctrl_kernel_mode_write(struct kernfs_open_file *of,
> +					 char *buf, size_t nbytes, loff_t off)
> +{
> +	enum resctrl_kernel_mode mode;
> +	char *mode_str, *group_str, *slash;
> +	const char *ctrl_name, *mon_name;

(needs reverse fir)

> +	struct rdtgroup *rdtgrp;
> +	int ret = 0;
> +
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> +		return -EINVAL;

function comment states: "negative errno with last_cmd_status set on error" yet here it
returns negative errno without any update to last_cmd_status.
Please see commit 242c0ab4d51d ("fs/resctrl: Change last_cmd_status custom during input parsing")

> +	buf[nbytes - 1] = '\0';
> +
> +	/* Tolerate surrounding whitespace before mode parsing. */
> +	buf = strim(buf);
> +
> +	/*
> +	 * Split "<mode>:group=<spec>"; the ":group=<spec>" suffix is optional
> +	 * and when omitted the default control group (&rdtgroup_default) is used.
> +	 * Trim mode_str after the split so whitespace before ":group=" is ignored.
> +	 * Square brackets from resctrl_kernel_mode_show() are not accepted.
> +	 */
> +	group_str = strstr(buf, ":group=");
> +	if (group_str) {
> +		*group_str = '\0';
> +		group_str += strlen(":group=");
> +	}
> +	mode_str = strim(buf);
> +
> +	mutex_lock(&rdtgroup_mutex);
> +	rdt_last_cmd_clear();

Needs info_kn_lock()/info_kn_unlock()

> +
> +	for (mode = 0; mode < RESCTRL_NUM_KERNEL_MODES; mode++)
> +		if (!strcmp(mode_str, resctrl_mode_str[mode]))
> +			break;
> +
> +	if (mode == RESCTRL_NUM_KERNEL_MODES) {
> +		rdt_last_cmd_puts("Unknown kernel mode\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	if (!(test_bit(mode, &resctrl_kcfg.kmode))) {
> +		rdt_last_cmd_puts("Kernel mode not available\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * INHERIT mode binds no rdtgroup. Ignore any user-supplied :group=
> +	 * suffix and let show print the mode without a group.

copy&paste?

> +	 */
> +	if (mode == INHERIT_CTRL_AND_MON) {
> +		rdtgrp = NULL;
> +		goto update_mode;
> +	}
> +
> +	if (!group_str) {
> +		/* No ":group=" suffix: fall back to the default control group. */
> +		rdtgrp = &rdtgroup_default;
> +	} else if (!strcmp(group_str, "uninitialized")) {

Why is this needed? Dropping this would just result in one of the group error messages
that should be sufficient?

> +		/* Display-only placeholder emitted by show; not selectable. */
> +		rdt_last_cmd_puts("Cannot bind to 'uninitialized' group\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	} else {
> +		/* Require exactly "<ctrl>/<mon>/" format */
> +		slash = strchr(group_str, '/');
> +		if (!slash) {
> +			rdt_last_cmd_puts("Group must be <ctrl>/<mon>/\n");
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +		*slash = '\0';
> +		ctrl_name = group_str;
> +		mon_name = slash + 1;
> +		slash = strchr(mon_name, '/');
> +		if (!slash || slash[1] != '\0') {
> +			rdt_last_cmd_puts("Group must be <ctrl>/<mon>/\n");
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +		*slash = '\0';
> +
> +		rdtgrp = rdtgroup_by_kmode_path(ctrl_name, mon_name);
> +		if (!rdtgrp) {
> +			rdt_last_cmd_puts("Group not found\n");
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +	}
> +
> +	if (mode == GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU &&
> +	    rdtgrp->type != RDTCTRL_GROUP) {
> +		rdt_last_cmd_puts("global_assign_ctrl_inherit_mon_per_cpu requires a control group\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> +	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {

Could it be more specific to just require here that the resource group is
required to be shareable?

> +		rdt_last_cmd_puts("Pseudo-locking in progress\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +update_mode:
> +	/* No-op if the same mode is already active on the same group. */
> +	if (resctrl_kcfg.kmode_cur == mode && resctrl_kcfg.k_rdtgrp == rdtgrp)
> +		goto out_unlock;
> +
> +	/*
> +	 * Tear down any active global-assign binding before applying the new
> +	 * policy, including when only the mode changes on the same group.
> +	 */
> +	if (resctrl_kcfg.kmode_cur != INHERIT_CTRL_AND_MON)
> +		rdtgroup_config_kmode_reset(resctrl_kcfg.k_rdtgrp,
> +					    resctrl_kcfg.kmode_cur);
> +
> +	if (mode != INHERIT_CTRL_AND_MON)
> +		rdtgroup_config_kmode(rdtgrp, mode);
> +
> +	resctrl_kcfg.k_rdtgrp = rdtgrp;
> +	resctrl_kcfg.kmode_cur = mode;
> +
> +out_unlock:
> +	mutex_unlock(&rdtgroup_mutex);
> +	return ret ?: nbytes;
> +}
> +
>  void *rdt_kn_parent_priv(struct kernfs_node *kn)
>  {
>  	/*
> @@ -2117,9 +2348,10 @@ static struct rftype res_common_files[] = {
>  	},
>  	{
>  		.name		= "kernel_mode",
> -		.mode		= 0444,
> +		.mode		= 0644,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
>  		.seq_show	= resctrl_kernel_mode_show,
> +		.write		= resctrl_kernel_mode_write,
>  		.fflags		= RFTYPE_TOP_INFO,
>  	},
>  	{

Reinette