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

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

Hi Babu,

On 8/26/26 12:32 PM, Babu Moger wrote:
> ---
>  Documentation/filesystems/resctrl.rst |  33 +++
>  fs/resctrl/pseudo_lock.c              |   5 +
>  fs/resctrl/rdtgroup.c                 | 319 +++++++++++++++++++++++++-
>  3 files changed, 356 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index c6e8cf828e18..490e8f534d37 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -557,6 +557,39 @@ conveyed in the error returns from file operations. E.g.
>  	  [inherit_user]
>  	  assign_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. To select inherit_user, write that mode name
> +	alone.
> +
> +	For assign_global_enable_per_cpu:
> +
> +	- Writing the mode name alone selects ctrl=assign, mon=assign, and the
> +	  default CTRL_MON group.

Please do not commit resctrl to use specific defaults. The documentation can just mention
that read of the file provides the defaults.

> +	- ctrl=, mon=, and group= are optional, use the same syntax as on read,
> +	  and may appear in any order. ctrl= and mon= default to assign;
> +	  group= defaults to the default CTRL_MON group.

This just repeats previous point?

> +	- Empty ctrl=, mon=, or group= values are rejected.
> +	- A write with both ctrl=inherit and mon=inherit is a no-op.

... because this assumes there are only two kernel modes that can ever be supported
and the other one must be "inherit_user" and thus providing "inherit" for
these two parameters imply that "inherit_user" kernel mode? Could you *please*
consider other architectures?

> diff --git a/fs/resctrl/pseudo_lock.c b/fs/resctrl/pseudo_lock.c
> index dea2b4bf966f..a0b22b95fc08 100644
> --- a/fs/resctrl/pseudo_lock.c
> +++ b/fs/resctrl/pseudo_lock.c
> @@ -536,6 +536,11 @@ int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp)
>  		return -EINVAL;
>  	}
>  
> +	if (rdtgrp->kmode) {
> +		rdt_last_cmd_puts("Group has an active kernel-mode association\n");
> +		return -EINVAL;
> +	}
> +

These snippets (also the changes to rdtgroup_mode_write() and rdtgroup_rename()) noting
when it is and is not ok to make other resctrl changes is not related to support for
modifying the kernel mode and should be in a separate patch.

...

> +
> +static int resctrl_kmode_parse_ctrl_mon(char *options, const char *field,
> +				    enum kmode_state *state)

run checkpatch.pl --strict as part of your patch prep.

> +{
> +	char *opt, *val, *end;
> +	int ret = 0;
> +
> +	opt = strstr(options, field);
> +	if (!opt)
> +		return 0;
> +
> +	val = opt + strlen(field);
> +	end = strchr(val, ';');
> +	if (end)
> +		*end = '\0';
> +	if (resctrl_kmode_parse_option(val, state))
> +		ret = -EINVAL;
> +
> +	if (end)
> +		*end = ';';
> +	return ret;
> +}

This is an unexpected and new pattern to use a temporary NUL in a buffer
then restore the original character. resctrl has a couple of instances where
options separated with ";" needs to be parsed - many of them written by you!
ctrlmondata.c:parse_line(), ctrlmondata.c:resctrl_io_alloc_parse_line(),
monitor.c:resctrl_parse_mbm_assignment() - why invent a new pattern?

sashiko seems to believe there is a issue here. Some of the other sashiko issues
look real to me. Please consider the sashiko feedback:

https://sashiko.dev/#/patchset/cover.1787772750.git.babu.moger%40amd.com


> +
> +static int resctrl_kmode_parse_group(char *options, struct rdtgroup **rdtgrp)
> +{
> +	const char *ctrl_name, *mon_name;
> +	char *group_str, *end, *slash;
> +	struct rdtgroup *grp;
> +	int ret = 0;
> +
> +	/* Skip parsing when group= is not present. */
> +	group_str = strstr(options, "group=");
> +	if (!group_str)
> +		return 0;
> +
> +	/* Isolate the group= value from any following options. */
> +	group_str += strlen("group=");
> +	end = strchr(group_str, ';');
> +	if (end)
> +		*end = '\0';
> +	group_str = strim(group_str);
> +	if (!*group_str) {
> +		rdt_last_cmd_puts("group= requires <CTRL_MON>/<MON>/\n");
> +		ret = -EINVAL;
> +		goto out_parse;
> +	}
> +	/* Split <CTRL_MON>/<MON>/ at the first slash. */
> +	slash = strchr(group_str, '/');
> +	if (!slash) {
> +		rdt_last_cmd_puts("Group must be <CTRL_MON>/<MON>/\n");
> +		ret = -EINVAL;
> +		goto out_parse;
> +	}
> +	*slash = '\0';
> +	ctrl_name = group_str;
> +	mon_name = slash + 1;
> +	/* Require a trailing slash after the monitor group name. */
> +	slash = strchr(mon_name, '/');
> +	if (!slash || slash[1] != '\0') {
> +		rdt_last_cmd_puts("Group must be <CTRL_MON>/<MON>/\n");
> +		ret = -EINVAL;
> +		goto out_parse;
> +	}
> +	*slash = '\0';
> +	/* Resolve the path to an existing rdtgroup. */
> +	grp = rdtgroup_by_kmode_path(ctrl_name, mon_name);
> +	if (!grp) {
> +		rdt_last_cmd_puts("Group not found\n");
> +		ret = -EINVAL;
> +		goto out_parse;
> +	}
> +	*rdtgrp = grp;
> +
> +out_parse:
> +	/* Restore the option string after temporary null termination. */
> +	if (end)
> +		*end = ';';

same here ... removing characters from buffer and then restoring them is
unexpected.

> +	return ret;
> +}
> +
> +/**
> + * resctrl_kernel_mode_write() - Set the active kernel mode policy
> + * @of: kernfs open file
> + * @buf: Write buffer; use the resctrl_kernel_mode_show() line format without
> + *	 brackets and with a trailing newline
> + * @nbytes: length of @buf
> + * @off: unused
> + *
> + * Parse and validate the request, then update the active kernel mode
> + * association.
> + *
> + * 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)
> +{
> +	enum kmode_state ctrl_mode = KMODE_ASSIGN, mon_mode = KMODE_ASSIGN;
> +	char *mode_str, *options;
> +	enum resctrl_kernel_mode mode;

needs reverse fir

> +	struct rdtgroup *rdtgrp;
> +	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_write: Invalid input\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	buf[nbytes - 1] = '\0';
> +
> +	buf = strim(buf);
> +	options = strchr(buf, ':');
> +	if (options) {
> +		*options = '\0';
> +		options++;
> +	}
> +	mode_str = strim(buf);
> +
> +	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.caps.kmode_sup)) {
> +		rdt_last_cmd_puts("Kernel mode not available\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	if (mode == RESCTRL_INHERIT_USER) {
> +		rdtgrp = NULL;
> +		goto update_mode;
> +	}
> +
> +	rdtgrp = &rdtgroup_default;
> +
> +	if (!options)
> +		goto validate_kmode;
> +
> +	ret = resctrl_kmode_parse_ctrl_mon(options, "ctrl=", &ctrl_mode);
> +	if (ret) {
> +		rdt_last_cmd_puts("Invalid ctrl= option\n");
> +		goto out_unlock;
> +	}
> +	ret = resctrl_kmode_parse_ctrl_mon(options, "mon=", &mon_mode);
> +	if (ret) {
> +		rdt_last_cmd_puts("Invalid mon= option\n");
> +		goto out_unlock;
> +	}
> +
> +	ret = resctrl_kmode_parse_group(options, &rdtgrp);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (ctrl_mode == KMODE_INHERIT && mon_mode == KMODE_INHERIT)
> +		goto out_unlock;
> +
> +validate_kmode:

This long function is difficult to follow and this usage of goto is a big 
part of making it difficult to understand since it just jumps to the middle
of the function instead of a cleanup label as is custom in the kernel. 

Please refactor.

Stopping here. Something is off with this series. It seems to be created
without the learnings and patterns accumulated from your previous resctrl
contributions while also ignoring x86 (and even kernel) customs.

Reinette