Re: [PATCH v6 16/18] fs/resctrl: Add interface to modify kernel mode via info/kernel_mode

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

Hi Reinette,

On 10/15/26 11:07, Reinette Chatre wrote:
> Hi Babu,
> 
> nit: subject states this adds new interfaces but that is not true?

fs/resctrl: Extend info/kernel_mode to modify kernel mode

> 
> On 10/1/26 8:10 AM, Babu Moger wrote:
>> ---
>>   Documentation/filesystems/resctrl.rst |  28 +++
>>   fs/resctrl/rdtgroup.c                 | 274 +++++++++++++++++++++++++-
>>   2 files changed, 301 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 73ccbb9594f5..bf9d34d6a335 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -575,6 +575,34 @@ conveyed in the error returns from file operations. E.g.
>>   	  [inherit_user]
>>   	  global_enable_per_cpu:ctrl=assign;mon=assign;group=//
>>   
>> +	Writes use the same line format as read, without square brackets and
>> +	with a trailing newline.
>> +
>> +	inherit_user is written without any parameters.
>> +
>> +	For global_enable_per_cpu, the parameters are optional. An omitted
>> +	ctrl=, mon=, or group= keeps the value currently shown. Parameters
> 
> "keeps the value currently shown" -> "keeps the default values"
> Text as suggested, or similar, will connect this text with the earlier
> "When the mode is not active, the parameters are the defaults ..."
> Although, please check earlier comment about the "parameters are the defaults"
> text.

Sure.

> 
>> +	may appear in any order. Empty values are rejected.
> 
> More on this later but I do not think it is possible for parameters to
> appear in any order ...
> 

Ack.

>> +
>> +	Selecting a new mode, parameter, or group replaces the active
>> +	association. A new global_enable_per_cpu association sets that
>> +	group's kmode_cpus to every online CPU. Errors are reported in
>> +	"info/last_cmd_status".
>> +	Example::
>> +
>> +	  # mkdir ctrl1
>> +	  # echo "global_enable_per_cpu:ctrl=assign;mon=assign;group=ctrl1//" \
>> +	         > info/kernel_mode
>> +
>> +	  # cat info/kernel_mode
>> +	  inherit_user
>> +	  [global_enable_per_cpu:ctrl=assign;mon=assign;group=ctrl1//]
>> +
>> +	  # echo "inherit_user" > info/kernel_mode
>> +	  # 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 dcf9f26540d6..73749e5a4ad0 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1216,6 +1216,277 @@ static void rdtgroup_kmode_global_detach(struct rdtgroup *rdtgrp)
>>   	resctrl_kmode.mon = KMODE_ASSIGN;
>>   }
>>   
>> +/* Program @rdtgrp on every online CPU and show its kmode_cpus files. */
>> +static void rdtgroup_kmode_global_activate(struct rdtgroup *rdtgrp)
>> +{
>> +	cpumask_copy(&rdtgrp->kmode_cpu_mask, cpu_online_mask);
>> +	resctrl_configure_kmode_global(&rdtgrp->kmode_cpu_mask, rdtgrp, true);
>> +	resctrl_kmode_global_files_show(rdtgrp->kn, true);
>> +}
>> +
>> +/**
>> + * rdtgroup_by_kmode_path() - Resolve a "<ctrl>/<mon>/" path to an rdtgroup
> 
> It is a contradiction to have description read "Resolve a "<ctrl>/<mon>/" path"
> and then the control and monitor group names are provided as separate parameters
>

ack.

> The function name also includes "path" but there is nothing about a path in
> the implementation, the caller parses the path, no?

I can handle all this in resctrl_kmode_parse_group(). Dont need a 
separate rdtgroup_by_kmode_path().

> 
>> + * @ctrl_name:	CTRL_MON group name, or "" for the default control group.
>> + * @mon_name:	MON group name, or "" to select the control group itself.
>> + *
>> + * Matches the path syntax displayed by resctrl_kernel_mode_show():
>> + *   "//"            - the default CTRL_MON group
>> + *   "<ctrl>//"      - CTRL_MON group @ctrl_name
>> + *   "/<mon>/"       - MON group @mon_name under the default control group
>> + *   "<ctrl>/<mon>/" - MON group @mon_name under control group @ctrl_name
>> + *
>> + * 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 entries of rdt_all_groups are of type RDTCTRL_GROUP, no?

Yes. That is correct. Will remove this check.

> 
>> +			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;
>> +}
>> +
>> +static int resctrl_kmode_parse_state(char *val, enum kmode_state *state)
>> +{
>> +	if (!strcmp(val, "assign")) {
>> +		*state = KMODE_ASSIGN;
>> +		return 0;
>> +	}
>> +	if (!strcmp(val, "inherit")) {
>> +		*state = KMODE_INHERIT;
>> +		return 0;
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +static int resctrl_kmode_parse_group(char *group, struct rdtgroup **rdtgrp)
>> +{
>> +	struct rdtgroup *grp;
>> +	char *mon, *slash;
>> +
>> +	slash = strchr(group, '/');
>> +	if (!slash) {
>> +		rdt_last_cmd_puts("Group must be <CTRL_MON>/<MON>/\n");
>> +		return -EINVAL;
>> +	}
>> +	*slash = '\0';
>> +	mon = strim(slash + 1);
>> +
> 
> The reason why we picked the <CTRL_MON>/<MON>/ syntax is because "/" is the
> one character that is not allowed by kernfs to be in a directory name,
> additionally resctrl rejects \n in group names.
> 
> This parsing should thus take care with stripping spaces and just parsing
> in general to support all the different resource group names that users
> may be able to create (it may even contain ";"). Because of this it seems that
> it should be required for the resource group to be the last parameter provided
> by user space since it will be terminated by \n that cannot be in a resource
> group name?

I hope I got it.  We should generalize the parsing to here to handle 
special characters. Yes. It makes it easy as the group is the last 
parameter.


> 
>> +	slash = strchr(mon, '/');
>> +	if (!slash || slash[1] != '\0') {
>> +		rdt_last_cmd_puts("Group must be <CTRL_MON>/<MON>/\n");
>> +		return -EINVAL;
>> +	}
>> +	*slash = '\0';
>> +
> 
> The user should not be required to provide a monitor group when
> "mon=inherit" or if system does not support monitoring.

Yes. that is correct. This code need to change.

> 
>> +	grp = rdtgroup_by_kmode_path(group, mon);
>> +	if (!grp) {
>> +		rdt_last_cmd_puts("Group not found\n");
>> +		return -EINVAL;
>> +	}
>> +	*rdtgrp = grp;
>> +	return 0;
>> +}
>> +
>> +static int resctrl_kmode_parse_parameters(char *params, enum kmode_state *ctrl,
>> +					  enum kmode_state *mon,
>> +					  struct rdtgroup **rdtgrp)
>> +{
>> +	char *key, *tok;
>> +
>> +	while ((tok = strsep(&params, ";"))) {
>> +		tok = strim(tok);
>> +		if (!*tok)
> 
> Please use the same string parsing patterns as the rest of resctrl. Specifically,
> please use the familiar (to resctrl) comparison against '\0' as in "if (tok[0] == '\0')"
> instead of pointer dereference like above (and below).

Sure.


> 
>> +			continue;
>> +
>> +		key = strsep(&tok, "=");
>> +		if (!tok) {
>> +			rdt_last_cmd_puts("Empty kernel mode parameters\n");
> 
> Not sure how accurate messages should be but this is just duplicate error message
> of when there are no kernel mode parameters when the issue is actually that "="
> is missing.

It can be "Unsupported kernel mode parameters"

> 
>> +			return -EINVAL;
>> +		}
>> +		key = strim(key);
>> +		tok = strim(tok);
>> +		if (!*key || !*tok) {
>> +			rdt_last_cmd_puts("Empty kernel mode parameters\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (!strcmp(key, "ctrl")) {
> 
> Should this not fail on a system that is not capable of allocation?


Sure. Let me verify this.


> 
>> +			if (resctrl_kmode_parse_state(tok, ctrl)) {
>> +				rdt_last_cmd_puts("Invalid ctrl= parameter\n");
>> +				return -EINVAL;
>> +			}
>> +		} else if (!strcmp(key, "mon")) {
> 
> Similarly this should fail on system that does not support monitoring?

Yes. Will check this.

> 
>> +			if (resctrl_kmode_parse_state(tok, mon)) {
>> +				rdt_last_cmd_puts("Invalid mon= parameter\n");
>> +				return -EINVAL;
>> +			}
>> +		} else if (!strcmp(key, "group")) {
>> +			if (resctrl_kmode_parse_group(tok, rdtgrp))
>> +				return -EINVAL;
>> +		} else {
>> +			rdt_last_cmd_puts("Unknown kernel mode parameter\n");
> 
> nit: "Unknown" -> "Unsupported"

Sure.

> 
>> +			return -EINVAL;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int resctrl_kmode_global_validate(struct rdtgroup *rdtgrp,
>> +					 enum kmode_state ctrl,
>> +					 enum kmode_state mon)
>> +{
>> +	if (ctrl == KMODE_ASSIGN && !resctrl_arch_alloc_capable()) {
>> +		rdt_last_cmd_puts("ctrl=assign is not supported\n");
>> +		return -EINVAL;
>> +	}
>> +	if (mon == KMODE_ASSIGN && !resctrl_arch_mon_capable()) {
>> +		rdt_last_cmd_puts("mon=assign is not supported\n");
>> +		return -EINVAL;
>> +	}
> 
> Does this mean that user can provide "ctrl=inherit" on system that does
> not support allocation and "mon=inherit" on system that does not support
> monitoring? That does not look right.

Good point. Will fix that.

> 
> 
>> +	if (mon == KMODE_INHERIT && rdtgrp->type == RDTMON_GROUP) {
>> +		rdt_last_cmd_puts("A monitor group requires mon=assign to use its RMID\n");
>> +		return -EINVAL;
>> +	}
>> +	if (rdtgrp->mode != RDT_MODE_SHAREABLE) {
>> +		rdt_last_cmd_puts("The group is not in shareable mode\n");
>> +		return -EINVAL;
>> +	}
>> +	if (rdtgrp->type == RDTMON_GROUP &&
>> +	    rdtgrp->mon.parent->mode != RDT_MODE_SHAREABLE) {
>> +		rdt_last_cmd_puts("The parent group is not in shareable mode\n");
>> +		return -EINVAL;
>> +	}
> 
> I think it will be useful to organize the "kernel_mode" resctrl.rst portion
> for writing to "kernel_mode" similar to the portion for reading to clearly associate
> the above rules with the "global_enable_per_cpu" mode.

Ok. Sure. Will add few lines about this in resctrl.rst.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static void resctrl_kmode_switch(enum resctrl_kernel_mode mode,
>> +				 struct rdtgroup *rdtgrp,
>> +				 enum kmode_state ctrl, enum kmode_state mon)
>> +{
>> +	if (resctrl_kmode.rdtgrp)
> 
> Please do not use resctrl_kmode.rdtgrp as synonym for and interchangeably with
> resctrl_kmode.kmode_cur == RESCTRL_GLOBAL_ENABLE_PER_CPU. Please check
> other places too, like in resctrl_kernel_mode_write()

Sure. Will do.

> 
>> +		rdtgroup_kmode_global_deactivate(resctrl_kmode.rdtgrp);
>> +
>> +	resctrl_kmode.kmode_cur = mode;
> 
> Now looking at how things come together, what do you think of renaming
> "kmode_cur" to "active"? "kmode" in "kmode_cur" is redundant and "active"
> looks to be a good match with existing documentation ("... with the
> active mode enclosed in square brackets") and code like
> rdtgroup_kmode_global_activate() and "active" used as local variable in
> resctrl_kernel_mode_show().

Sounds good.

> 
> Similar to this, could resctrl_kmode.kmode_sup be renamed to resctrl_kmode.supported?
>

Yes. Will change it.


> 
>> +	if (mode != RESCTRL_GLOBAL_ENABLE_PER_CPU) {
> 
> If this is needed please make the test explicit to the "inherit" mode.
> 
>> +		resctrl_kmode.rdtgrp = NULL;
>> +		resctrl_kmode.ctrl = KMODE_ASSIGN;
>> +		resctrl_kmode.mon = KMODE_ASSIGN;
> 
> Something does not look right here ... function parameters provide values that are not used.
> If any reset is needed, should it not be done as part of rdtgroup_kmode_global_deactivate()
> especially since it appears that the contract with architecture is for disable of
> global kernel mode to mean that inherit mode has to be enabled?

Yes. Will remove resctrl_kmode_switch(). Will handle this in 
rdtgroup_kmode_global_deactivate().
> 
>> +		return;
>> +	}
>> +
>> +	resctrl_kmode.ctrl = ctrl;
>> +	resctrl_kmode.mon = mon;
>> +	resctrl_kmode.rdtgrp = rdtgrp;
>> +	rdtgroup_kmode_global_activate(rdtgrp);
>> +}
>> +
>> +/**
>> + * resctrl_kernel_mode_write() - Set the active kernel mode
>> + * @of:		kernfs open file
>> + * @buf:	write buffer
> 
> Could this be more specific about what the buffer represents? For example,
> 	Kernel mode line written by user space

Sure.

> 
>> + * @nbytes:	length of @buf
>> + * @off:	unused
>> + *
>> + * Return: @nbytes on success, negative errno on error.
>> + */
>> +static ssize_t resctrl_kernel_mode_write(struct kernfs_open_file *of,
>> +					 char *buf, size_t nbytes, loff_t off)
>> +{
>> +	struct rdtgroup *rdtgrp = &rdtgroup_default;
>> +	enum resctrl_kernel_mode mode;
>> +	enum kmode_state ctrl, mon;
>> +	char *params;
>> +	int ret = 0;
>> +
>> +	if (!info_kn_lock(of->kn))
>> +		return -ENOENT;
>> +
>> +	rdt_last_cmd_clear();
>> +
>> +	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
>> +		rdt_last_cmd_puts("kernel_mode: Invalid input\n");
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +	buf[nbytes - 1] = '\0';
>> +	buf = strim(buf);
>> +
>> +	params = strchr(buf, ':');
>> +	if (params)
>> +		*params++ = '\0';
> 
> Why not use strsep() to be consistent with other parsing in resctrl?
> 

Sure. Will change it.

>> +
>> +	for (mode = 0; mode < RESCTRL_NUM_KERNEL_MODES; mode++) {
>> +		if (!strcmp(buf, resctrl_kmode_str[mode]))
> 
> The whitespace trimming is inconsistent. Whitespace is trimmed from buffer,
> but before looking for the ":". Looks like something like
> "global_enable_per_cpu :ctrl=assign" fails with "Unknown kernel mode",
> while spaces around ; and = are accepted.

Sure. Fix it.

> 
>> +			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_kmode.kmode_sup)) {
>> +		rdt_last_cmd_puts("Kernel mode not available\n");
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
>> +	if (mode == RESCTRL_INHERIT_USER) {
>> +		if (params) {
>> +			rdt_last_cmd_puts("inherit_user has no parameters\n");
> 
> nit: Just in case resctrl needs to add parameters later, could this be
> "Unsupported parameter" instead?

Sure.

> 
>> +			ret = -EINVAL;
>> +			goto out_unlock;
>> +		}
>> +		if (resctrl_kmode.kmode_cur != mode)
>> +			resctrl_kmode_switch(mode, NULL, KMODE_ASSIGN, KMODE_ASSIGN);
>> +		goto out_unlock;
>> +	}
>> +
>> +	/* Omitted parameters keep the values shown by reading the file. */
> 
> This comment does not seem to match the show() that always prints resctrl_kmode.ctrl.
> Should rdtgroup_kmode_global_deactivate() reset relevant resctrl_kmode fields?

Yes. rdtgroup_kmode_global_deactivate() will reset the fields now.

The resctrl_kmode_switch() and resctrl_kmode_detach() will be removed.

Thanks
Babu