Message-ID: <7c6a4f7e-e810-4d81-b01d-b0cbf644472f@amd.com>
Date: Fri, 19 Sep 2025 15:49:40 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....com>, corbet@....net, tony.luck@...el.com,
 Dave.Martin@....com, james.morse@....com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, kas@...nel.org,
 rick.p.edgecombe@...el.com, akpm@...ux-foundation.org, paulmck@...nel.org,
 pmladek@...e.com, pawan.kumar.gupta@...ux.intel.com, rostedt@...dmis.org,
 kees@...nel.org, arnd@...db.de, fvdl@...gle.com, seanjc@...gle.com,
 thomas.lendacky@....com, manali.shukla@....com, perry.yuan@....com,
 sohil.mehta@...el.com, xin@...or.com, peterz@...radead.org,
 mario.limonciello@....com, gautham.shenoy@....com, nikunj@....com,
 dapeng1.mi@...ux.intel.com, ak@...ux.intel.com, chang.seok.bae@...el.com,
 ebiggers@...gle.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-coco@...ts.linux.dev, kvm@...r.kernel.org
Subject: Re: [PATCH v9 09/10] fs/resctrl: Introduce interface to modify
 io_alloc Capacity Bit Masks

Hi Reinette,

On 9/18/2025 1:03 AM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/2/25 3:41 PM, Babu Moger wrote:
>> The io_alloc feature in resctrl enables system software to configure the
>> portion of the cache allocated for I/O traffic. When supported, the
>> io_alloc_cbm file in resctrl provides access to Capacity Bit Masks (CBMs)
>> reserved for I/O devices.
> 
> reserved -> allocated?

Sure.

> 
> The cache portions represented by CBMs are not reserved for I/O devices - these
> portions are available for sharing and can still be used by CPU cache allocation.
> 
>>
>> Enable users to modify io_alloc CBMs (Capacity Bit Masks) via the
> 
> Can drop "(Capacity Bit Masks)" since acronym was spelled out in first paragraph.

sure.

> 
>> io_alloc_cbm resctrl file when io_alloc is enabled.
>>
>> To ensure consistent cache allocation when CDP is enabled, the CBMs
> 
> This is not about "consistent cache allocation" but instead a consistent user
> interface. How about "To present consistent I/O allocation information to user
> space when CDP is enabled, the CBMs ..."
> 
>> written to either L3CODE or L3DATA are mirrored to the other, keeping both
>> resource types synchronized.
> 
> (needs imperative)

Here is the updated full changelog.

fs/resctrl: Introduce interface to modify io_alloc Capacity Bit Masks

The io_alloc feature in resctrl enables system software to configure the
portion of the cache allocated for I/O traffic. When supported, the
io_alloc_cbm file in resctrl provides access to Capacity Bit Masks 
(CBMs) allocated for I/O devices.

Enable users to modify io_alloc CBMs via io_alloc_cbm resctrl file when 
the feature is enabled.

Mirror the CBMs between CDP_CODE and CDP_DATA when CDP is enabled to 
present consistent I/O allocation information to user space and keep 
both resource types synchronized.

Signed-off-by: Babu Moger <babu.moger@....com>


> 
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
> 
> ...
> 
>> ---
>>   Documentation/filesystems/resctrl.rst | 11 ++++
>>   fs/resctrl/ctrlmondata.c              | 93 +++++++++++++++++++++++++++
>>   fs/resctrl/internal.h                 |  3 +
>>   fs/resctrl/rdtgroup.c                 |  3 +-
>>   4 files changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 15e3a4abf90e..7e3eda324de5 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -188,6 +188,17 @@ related to allocation:
>>   			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
>>   			0=ffff;1=ffff
>>   
>> +		CBMs can be configured by writing to the interface.
>> +
>> +		Example::
>> +
>> +			# echo 1=ff > /sys/fs/resctrl/info/L3/io_alloc_cbm
>> +			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
>> +			0=ffff;1=00ff
>> +			# echo 0=ff;1=f > /sys/fs/resctrl/info/L3/io_alloc_cbm
> 
> To accommodate how a shell may interpret above this should perhaps be (see schemata examples):

Sure.

> 
> 			# echo "0=ff;1=f" > /sys/fs/resctrl/info/L3/io_alloc_cbm
> 
>> +			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
>> +			0=00ff;1=000f
>> +
>>   		When CDP is enabled "io_alloc_cbm" associated with the DATA and 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 a4e861733a95..791ecb559b50 100644
>> --- a/fs/resctrl/ctrlmondata.c
>> +++ b/fs/resctrl/ctrlmondata.c
>> @@ -848,3 +848,96 @@ int resctrl_io_alloc_cbm_show(struct kernfs_open_file *of, struct seq_file *seq,
>>   	cpus_read_unlock();
>>   	return ret;
>>   }
>> +
>> +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;
>> +	struct rdt_parse_data data;
>> +	struct rdt_ctrl_domain *d;
>> +	char *dom = NULL, *id;
>> +	unsigned long dom_id;
>> +
>> +next:
>> +	if (!line || line[0] == '\0')
>> +		return 0;
>> +
>> +	dom = strsep(&line, ";");
>> +	id = strsep(&dom, "=");
>> +	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) {
>> +			data.buf = dom;
>> +			data.mode = RDT_MODE_SHAREABLE;
>> +			data.closid = closid;
>> +			if (parse_cbm(&data, s, d))
>> +				return -EINVAL;
>> +			/*
>> +			 * When CDP is enabled, update the schema for both CDP_DATA
>> +			 * and CDP_CODE.
> 
> The comment just describes what can be seen from the code. How about something like
> "Keep io_alloc CLOSID's CBM of CDP_CODE and CDP_DATA in sync."?

Sure.

> 
> Of note is that these comments are generic while earlier comments related to CDP are L3
> specific ("L3CODE" and "L3DATA"). Having resource specific names in generic code is not ideal,
> even if first implementation is only for L3. I think this was done in many places though,
> even in a couple of the changelogs I created and I now realize the impact after seeing
> this comment. Could you please take a look to make the name generic when it is used in
> generic changelog and comments?

Sure.

> 
>> +			 */
>> +			if (resctrl_arch_get_cdp_enabled(r->rid)) {
>> +				peer_type = resctrl_peer_type(s->conf_type);
>> +				memcpy(&d->staged_config[peer_type],
>> +				       &d->staged_config[s->conf_type],
>> +				       sizeof(d->staged_config[0]));
>> +			}
>> +			goto next;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +ssize_t resctrl_io_alloc_cbm_write(struct kernfs_open_file *of, char *buf,
>> +				   size_t nbytes, loff_t off)
>> +{
>> +	struct resctrl_schema *s = rdt_kn_parent_priv(of->kn);
>> +	struct rdt_resource *r = s->res;
>> +	u32 io_alloc_closid;
>> +	int ret = 0;
>> +
>> +	/* Valid input requires a trailing newline */
>> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
>> +		return -EINVAL;
>> +
>> +	buf[nbytes - 1] = '\0';
>> +
>> +	cpus_read_lock();
>> +	mutex_lock(&rdtgroup_mutex);
>> +	rdt_last_cmd_clear();
>> +
>> +	if (!r->cache.io_alloc_capable) {
>> +		rdt_last_cmd_printf("io_alloc is not supported on %s\n", s->name);
>> +		ret = -ENODEV;
>> +		goto out_unlock;
>> +	}
>> +
>> +	if (!resctrl_arch_get_io_alloc_enabled(r)) {
>> +		rdt_last_cmd_printf("io_alloc is not enabled on %s\n", s->name);
>> +		ret = -ENODEV;
> 
> Compare to comment in patch #7 where the same error of io_alloc not being enabled results
> in different error code (EINVAL). Please keep these consistent.

Yes. Changed it to -EINVAL.

thanks
Babu


> 
> 

Powered by blists - more mailing lists