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

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

Hi Babu,

nit: subject states this adds new interfaces but that is not true?

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.

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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

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


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

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

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

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

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

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

> +	ctrl = resctrl_arch_alloc_capable() ? resctrl_kmode.ctrl : KMODE_INHERIT;
> +	mon = resctrl_arch_mon_capable() ? resctrl_kmode.mon : KMODE_INHERIT;
> +	if (resctrl_kmode.rdtgrp)
> +		rdtgrp = resctrl_kmode.rdtgrp;
> +
> +	if (params) {
> +		ret = resctrl_kmode_parse_parameters(params, &ctrl, &mon, &rdtgrp);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +
> +	ret = resctrl_kmode_global_validate(rdtgrp, ctrl, mon);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (resctrl_kmode.kmode_cur != mode || resctrl_kmode.rdtgrp != rdtgrp ||
> +	    resctrl_kmode.ctrl != ctrl || resctrl_kmode.mon != mon)
> +		resctrl_kmode_switch(mode, rdtgrp, ctrl, mon);
> +
> +out_unlock:
> +	info_kn_unlock(of->kn);
> +	return ret ?: nbytes;
> +}
> +
>  void *rdt_kn_parent_priv(struct kernfs_node *kn)
>  {
>  	/*
> @@ -2230,9 +2501,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