Re: [PATCH v5 2/2] fs/resctrl: Add "*" shorthand to set io_alloc CBM for all domains

[Reinette Chatre]

Hi Aaron,

On 2/10/26 1:07 PM, Aaron Tomlin wrote:
> Currently, configuring the io_alloc_cbm interface requires an explicit

(nit: "Currently" can be dropped. Expectation is that the patch starts
with context that is by default the current behavior.)

> domain ID for each cache domain. On systems with high core counts and
> numerous cache clusters, this requirement becomes cumbersome for
> automation and management tasks that aim to apply a uniform policy.
> 
> Introduce a wildcard domain ID selector "*" for the io_alloc_cbm
> interface. This enables users to update the Capacity Bitmask (CBM)

"update the" -> "set the same"?

> across all cache domains in a single operation.
> 
> For example, a user can write "*=0" to the io_alloc_cbm file to

This example seems redundant. The description is clear and the documentation
addition found in the patch describes this scenario anyway.

> programme every domain with the same mask. The value supplied must,

"programme" -> "program"

I suggest to drop the last sentence "The value supplied ..." since it
implies that the addition cannot do its own error checking. 

> however, remain within the valid range defined by the resource
> (e.g., min_cbm_bits).
> 
> Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxxx>
> ---
>  Documentation/filesystems/resctrl.rst | 10 ++++++++++
>  fs/resctrl/ctrlmondata.c              | 15 ++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 8c8ce678148a..948219e58882 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -215,6 +215,16 @@ related to allocation:
>  			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
>  			0=00ff;1=000f
>  
> +		Set each CBM to a specified value.

Above sentence seems redundant when compared to below sentence that is
more specific. Can it just be dropped?

> +
> +		An ID of "*" configures all domains with the provided CBM.
> +
> +		Example::

Just to make this clear I suggest to add caveat:
	"Example on a system that does not require a minimum number of consecutive bits in the mask::"

> +
> +			# echo "*=0" > /sys/fs/resctrl/info/L3/io_alloc_cbm
> +			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
> +			0=0;1=0
> +
>  		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 b96b661626c2..f47331a97337 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -873,21 +873,26 @@ 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 (dom && !strcmp(id, "*")) {
> +		update_all = true;
> +	} else if (!dom || kstrtoul(id, 10, &dom_id)) {
>  		rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
>  		return -EINVAL;
>  	}
>  
>  	dom = strim(dom);
>  	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> -		if (d->hdr.id == dom_id) {
> +		if (update_all || d->hdr.id == dom_id) {

While this code is correct in that dom_id will not be accessed when
update_all is true it makes the code more difficult to understand
when an uninitialized variable depends on certain flows. 

>  			data.buf = dom;
>  			data.mode = RDT_MODE_SHAREABLE;
>  			data.closid = closid;
> @@ -903,10 +908,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;

One comment from v4 appears to be unaddressed. Copied here for convenience:
	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".

Below is a sample fixup that addresses the comments. What do you think?

diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index 869e37f7d22a..9a7dfc48cb2e 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -954,17 +954,21 @@ static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
 				       struct resctrl_schema *s, u32 closid)
 {
 	enum resctrl_conf_type peer_type;
+	unsigned long dom_id = ULONG_MAX;
 	struct rdt_parse_data data;
 	struct rdt_ctrl_domain *d;
+	bool update_all = false;
 	char *dom = NULL, *id;
-	unsigned long dom_id;
-	bool update_all;
 
 next:
 	if (!line || line[0] == '\0')
 		return 0;
 
-	update_all = false;
+	if (update_all) {
+		rdt_last_cmd_puts("Configurations after global '*'\n");
+		return -EINVAL;
+	}
+
 	dom = strsep(&line, ";");
 	id = strsep(&dom, "=");
 

> +
>  	rdt_last_cmd_printf("Invalid domain %lu\n", dom_id);
>  	return -EINVAL;
>  }

Reinette