Message-ID: <b3c1e304-85e8-421f-9a31-bcf1443c2545@intel.com>
Date: Mon, 2 Feb 2026 20:26:27 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: Aaron Tomlin <atomlin@...mlin.com>, <tony.luck@...el.com>,
	<Dave.Martin@....com>, <james.morse@....com>, <babu.moger@....com>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>
CC: <sean@...e.io>, <neelx@...e.com>, <mproche@...il.com>,
	<chjohnst@...il.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [v4 PATCH 1/1] x86/resctrl: Add "*" shorthand to set io_alloc CBM
 for all domains

Hi Aaron,

No need to add a cover letter when there is just one patch. From what
I can tell the cover letter contains duplicate text so can just be
dropped in the next version. Although, there may possibly be two patches
in next version (more later).

For the subject, please follow the custom to have the "PATCH" text
be the prefix. Specifically "[PATCH vX]". For reference, see
"Subject Line" in Documentation/process/submitting-patches.rst.

On 1/25/26 9:17 AM, Aaron Tomlin wrote:
> Introduce a wildcard domain ID selector "*" for the io_alloc_cbm
> interface. This allows a user to update the Capacity Bitmask (CBM)
> across all cache domains in a single operation.

The changelog jumps in with what the patch does before providing any
context (which can be found in paragraph below). Please follow the
"context, problem, solution" structure as detailed in section
"Changelog" found in Documentation/process/maintainer-tip.rst

> 
> Currently, configuring io_alloc_cbm requires an explicit ID for each
> domain, which is cumbersome on systems with high core counts and
> numerous cache clusters. Supporting a wildcard selector simplifies
> automation and management tasks.
> 
> For example, a user can now write "*=0" to the io_alloc_cbm file to
> program every domain to the hardware-defined minimum CBM. Note that the

(nit: not all hardware has 0 as minimum CBM).

> value provided must still adhere to the constraints defined in the
> resource's min_cbm_bits.
> 
> Signed-off-by: Aaron Tomlin <atomlin@...mlin.com>
> ---
>  Documentation/filesystems/resctrl.rst |  8 ++++++++
>  fs/resctrl/ctrlmondata.c              | 20 +++++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 8c8ce678148a..734aba0d19fd 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -215,6 +215,14 @@ related to allocation:
>  			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
>  			0=00ff;1=000f
>  
> +		Set each CBM to a specified value.
> +
> +		An ID of "*" configures all domains with the provided CBM.
> +
> +		Example::
> +
> +			# echo "*=0" > /sys/fs/resctrl/info/L3/io_alloc_cbm
> +
>  		When CDP is enabled "io_alloc_cbm" associated with the CDP_DATA and CDP_CODE
>  		resources may reflect the same values. For example, values read from and
>  		written to /sys/fs/resctrl/info/L3DATA/io_alloc_cbm may be reflected by
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index b2d178d3556e..f5fb74a7668a 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -873,21 +873,31 @@ static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
>  	struct rdt_ctrl_domain *d;
>  	char *dom = NULL, *id;
>  	unsigned long dom_id;
> +	bool update_all;
>  
>  next:
>  	if (!line || line[0] == '\0')
>  		return 0;
>  
> +	update_all = false;
>  	dom = strsep(&line, ";");
>  	id = strsep(&dom, "=");
> -	if (!dom || kstrtoul(id, 10, &dom_id)) {
> +
> +	if (id && !strcmp(id, "*")) {
> +		update_all = true;

Since this cannot be reached if line is NULL, can id ever be NULL here? 

> +	} else if (!dom || kstrtoul(id, 10, &dom_id)) {
>  		rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
>  		return -EINVAL;
>  	}
>  
>  	dom = strim(dom);
> +	if (update_all && !dom) {

Have you tried just writing '*' to this file as suggested in v3? Please do
include doing so in your tests.

The NULL check should be _before_ any access to dom. I think this can be done
earlier though. How about something like below?

	if (dom && !strcmp(id, "*")) {
		...
	} else if (!dom || kstrtoul(id, 10, &dom_id)) {
		...
	}
	

> +		rdt_last_cmd_puts("Missing '=' after '*'\n");
> +		return -EINVAL;
> +	}
> +
>  	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> -		if (d->hdr.id == dom_id) {
> +		if (update_all || d->hdr.id == dom_id) {
>  			data.buf = dom;
>  			data.mode = RDT_MODE_SHAREABLE;
>  			data.closid = closid;
> @@ -903,10 +913,14 @@ static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
>  				       &d->staged_config[s->conf_type],
>  				       sizeof(d->staged_config[0]));
>  			}
> -			goto next;
> +			if (!update_all)
> +				goto next;
>  		}
>  	}
>  
> +	if (update_all)
> +		goto next;

I see that this aims to support input like "*=f;*=0" but I do not see how something like
this can ever succeed since parse_cbm() stages the config and should fail if any domain
already has a config. Should this perhaps just return success here? This could be
made more robust by only returning success if there is no more text to parse, thus failing
on input like "*=f;1=f".

> +
>  	return -EINVAL;

I just noticed that this is one spot where the user interface may be confusing. The intent
here is to return an error if the user provided an invalid domain ID. While this interface
returns an error the last_cmd_status file is not updated. What a user would see is thus that
writing to io_alloc_cmd fails but last_cmd_status returns "ok". This is a problem with
existing implementation.
Would you like to include a fix for this as part of this work? I think just something like
	rdt_last_cmd_printf("Invalid domain %d\n", dom_id);
before the "return -EINVAL" should be sufficient. This needs to be a separate patch though with
a:
Fixes: 28fa2cce7a83 ("fs/resctrl: Introduce interface to modify io_alloc capacity bitmasks")

Reinette

Powered by blists - more mailing lists