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

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,

On 8/10/26 22:40, Reinette Chatre wrote:
> 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?

Will rewrite the changelog. The text "extend the read format to 
identify" is not nessasary.


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

I need to write in terms of CTLR_MON and MON group. Let me update that.

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

Ok.

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

ok.

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

Will rewrite it.

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

ok.

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

Yes. I think so. Let me check that.

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

Let me update this. Let me convert it  to high level summary,

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

Yes.

> 
>> +	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")

Sure.

> 
>> +	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()

Sure.

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

Hmm.. Text seems correct here to me. Did I miss something?

All these code will change with new interface.

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

Yes. That is correct.

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

Yes. Correct.


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