linux-kernel - Re: [v4 PATCH 1/1] x86/resctrl: Add "*" shorthand to set io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <57hmamtu4ql4pxmqfbw5nsytibg6k7ybnhal5tsvi6nyd36v2t@kucbm5thf634>
Date: Sat, 7 Feb 2026 18:28:07 -0500
From: Aaron Tomlin <atomlin@...mlin.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: 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, 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

On Mon, Feb 02, 2026 at 08:26:27PM -0800, Reinette Chatre wrote:
> 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.

Hi Reinette,

Thank you for the detailed review.

I will incorporate all the stylistic feedback (Subject prefix, Changelog
structure, and dropping the cover letter) in v5.

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

I understand. I shall update the text to ensure the distinction regarding
the hardware-defined minimum is precise.

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

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

Agreed. I will refactor the parsing logic to prioritise the check for dom
before performing the string comparison, as you suggested. This yields a
significantly cleaner flow and mitigates potential dereference issues.

I will ensure that edge cases, including the standalone * (which should
correctly trigger the "Missing '='" error), are explicitly covered in my
testing before submission.

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

This is a valid concern. To address this properly, I intend to split the
next submission into a two-patch series:

    1. A fix for the existing last_cmd_status behaviour (reporting "ok"
       despite failure) when an invalid domain is provided.

    2. The introduction of the wildcard "*" support.

This approach ensures the fix is cleanly separated from the new feature
logic.


Kind regards,
-- 
Aaron Tomlin

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)