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

[Aaron Tomlin]

On Wed, Feb 18, 2026 at 10:30:57AM -0800, Reinette Chatre wrote:
> Hi Aaron,

Hi Reinette,

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

Acknowledged. I have dropped "Currently," from the opening sentence of the
commit message.

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

Acknowledged. I have updated the phrasing to "set the same Capacity Bitmask
(CBM)" in the commit message.

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

Acknowledged. I completely dropped the final paragraph containing the
redundant example, the typo, and the validation disclaimer from the commit
message.

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

Yes. I removed "Set each CBM to a specified value" from resctrl.rst.

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

I updated the caveat preceding the example block in resctrl.rst exactly as
suggested.

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

I applied your suggested change; at declaration to eliminate the
uninitialised variable state.

> >  			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;
> >  }
> 

I have applied it exactly as suggested.


Kind regards,
-- 
Aaron Tomlin

Attachment: signature.asc
Description: PGP signature