Message-ID: <f1721eba-2963-46c2-a68d-dd53f274df01@amd.com>
Date: Mon, 4 Aug 2025 16:20:43 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.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, akpm@...ux-foundation.org,
 paulmck@...nel.org, rostedt@...dmis.org, Neeraj.Upadhyay@....com,
 david@...hat.com, arnd@...db.de, fvdl@...gle.com, seanjc@...gle.com,
 thomas.lendacky@....com, pawan.kumar.gupta@...ux.intel.com,
 yosry.ahmed@...ux.dev, sohil.mehta@...el.com, xin@...or.com,
 kai.huang@...el.com, xiaoyao.li@...el.com, peterz@...radead.org,
 me@...aill.net, mario.limonciello@....com, xin3.li@...el.com,
 ebiggers@...gle.com, ak@...ux.intel.com, chang.seok.bae@...el.com,
 andrew.cooper3@...rix.com, perry.yuan@....com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 05/10] fs/resctrl: Update bit_usage to reflect io_alloc

Hi Reinette,

On 7/21/25 18:35, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/10/25 10:16 AM, Babu Moger wrote:
>> When the io_alloc feature is enabled, a portion of the cache can be
>> configured for shared use between hardware and software.
>>
>> Update the bit_usage representation to reflect the io_alloc configuration.
> 
> This patch is early in the series but also relies on capabilities added
> later in the series. This cryptic changelog leaves a lot for the user
> to decipher. For example:
> - How is the "io_alloc CLOSID" chosen? This is mentioned in cover letter but
>   not here. Doing so here may help explain the hardcoding of CDP_CODE done
>   in this patch (which seem unnecessary, more later, but a lot depends on
>   changes that follow this patch).
> - No mention that when io_alloc is enabled then an associated CLOSID will
>   be allocated. This is done later in series but assumed to be known here
>   where rdt_bit_usage_show() implicitly assumes that when io_alloc is enabled
>   then the "io_alloc CLOSID" is supported AND allocated (otherwise array overrun?)

Make sense. Moved this patch to the last.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v7: New patch split from earlier patch #5.
>>     Added resctrl_io_alloc_closid() to return max COSID.
>> ---
>>  Documentation/filesystems/resctrl.rst | 20 ++++++++++-----
>>  fs/resctrl/rdtgroup.c                 | 37 +++++++++++++++++++++++++--
>>  2 files changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index c7949dd44f2f..c3c412733632 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -89,12 +89,20 @@ related to allocation:
>>  		must be set when writing a mask.
>>  
>>  "shareable_bits":
>> -		Bitmask of shareable resource with other executing
>> -		entities (e.g. I/O). User can use this when
>> -		setting up exclusive cache partitions. Note that
>> -		some platforms support devices that have their
>> -		own settings for cache use which can over-ride
>> -		these bits.
>> +		Bitmask of shareable resource with other executing entities
>> +		(e.g. I/O). Applies to all instances of this resource. User
>> +		can use this when setting up exclusive cache partitions.
>> +		Note that some platforms support devices that have their
>> +		own settings for cache use which can over-ride these bits.
>> +
>> +		When "io_alloc" feature is enabled, a portion of the cache
>> +		can be configured for shared use between hardware and software.
> 
> To help distinguish how "io_alloc" is different from "shareable_bits" this can be:
> 		When "io_alloc" is enabled, a portion of each cache instance can
> 		be configured for shared use between hardware and software.

Sure.

> 
> Please merge these two paragraphs.

Sure.

> 
>> +		"bit_usage" should be used to see which portions of each cache
>> +		instance is configured for hardware use via "io_alloc" feature
>> +		because every cache instance can have its "io_alloc" bitmask
>> +		configured independently.
> 
> append " via "io_alloc_cbm"."?

sure.

> (but io_alloc_cbm does not exist at this point ... another motivation for this
> patch to move later)
> 

Sure.

>> +
>>  "bit_usage":
>>  		Annotated capacity bitmasks showing how all
>>  		instances of the resource are used. The legend is:
> 
> Please note that this "bit_usage" section contain several references to "shareable_bits"
> that should be updated to refer to "io_alloc_cbm" also.
> 
> With all these new terms introduced as common knowledge it is starting to look
> like this patch would be easier to consume later in the series.

Sure.

> 
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 77d08229d855..a2eea85aecc8 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1030,6 +1030,20 @@ static int rdt_shareable_bits_show(struct kernfs_open_file *of,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * resctrl_io_alloc_closid() - io_alloc feature routes I/O traffic using
>> + * the highest available CLOSID. Retrieve the maximum CLOSID supported by the
>> + * resource. Note that if Code Data Prioritization (CDP) is enabled, the number
>> + * of available CLOSIDs is reduced by half.
>> + */
>> +static u32 resctrl_io_alloc_closid(struct rdt_resource *r)
> 
> Please move to ctrlmondata.c.

Yes.

> 
>> +{
>> +	if (resctrl_arch_get_cdp_enabled(r->rid))
>> +		return resctrl_arch_get_num_closid(r) / 2  - 1;
>> +	else
>> +		return resctrl_arch_get_num_closid(r) - 1;
>> +}
>> +
>>  /*
>>   * rdt_bit_usage_show - Display current usage of resources
>>   *
>> @@ -1063,15 +1077,17 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
>>  
>>  	cpus_read_lock();
>>  	mutex_lock(&rdtgroup_mutex);
>> -	hw_shareable = r->cache.shareable_bits;
>>  	list_for_each_entry(dom, &r->ctrl_domains, hdr.list) {
>>  		if (sep)
>>  			seq_putc(seq, ';');
>> +		hw_shareable = r->cache.shareable_bits;
>>  		sw_shareable = 0;
>>  		exclusive = 0;
>>  		seq_printf(seq, "%d=", dom->hdr.id);
>>  		for (i = 0; i < closids_supported(); i++) {
>> -			if (!closid_allocated(i))
>> +			if (!closid_allocated(i) ||
>> +			    (resctrl_arch_get_io_alloc_enabled(r) &&
>> +			     i == resctrl_io_alloc_closid(r)))
>>  				continue;
>>  			ctrl_val = resctrl_arch_get_config(r, dom, i,
>>  							   s->conf_type);
>> @@ -1099,6 +1115,23 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
>>  				break;
>>  			}
>>  		}
>> +
>> +		/*
>> +		 * When the "io_alloc" feature is enabled, a portion of the
>> +		 * cache is configured for shared use between hardware and software.
>> +		 */
>> +		if (resctrl_arch_get_io_alloc_enabled(r)) {
> 
> Here is undocumented implicit assumption that when io_alloc is enabled then
> the "io_alloc CLOSID" is allocated. This is also outside the closids_supported()
> loop which adds the other implicit assumption that if io_alloc is enabled then
> its CLOSID is supported by resctrl fs. None of these concepts have been introduced
> so far and is not mentioned in changelog.
> It is not obvious here that an io_alloc CLOSID must be supported (this is just
> something enforced by later patches) and also not obvious that an io_alloc CLOSID
> must be allocated from same "pool" as other CLOSIDs. Without a good changelog and
> context of later patches this is hard to understand.
> These are motivations for this patch to move later in the series and then the
> changelog can just refer to these assumptions as fact, making it all easier to follow.

Sure.

> 
>> +			if (resctrl_arch_get_cdp_enabled(r->rid))
>> +				ctrl_val = resctrl_arch_get_config(r, dom,
>> +								   resctrl_io_alloc_closid(r),
>> +								   CDP_CODE);
>> +			else
>> +				ctrl_val = resctrl_arch_get_config(r, dom,
>> +								   resctrl_io_alloc_closid(r),
>> +								   CDP_NONE);
> 
> This does not look necessary to me. Why not just:
> 		if (resctrl_arch_get_io_alloc_enabled(r)) {
> 			ctrl_val = resctrl_arch_get_config(r, dom,
> 							   resctrl_io_alloc_closid(r),
> 							   s->conf_type);
> 			hw_shareable |= ctrl_val;
> 		}
> 
> Since the later patches keep the CDP_CODE and CDP_DATA CBMs in sync it does not matter
> if the io_alloc CBM is obtained from CDP_CODE or CDP_DATA and just providing the
> schema's type to resctrl_arch_get_config() will have it retrieve the right CBM, no?

Yes. That is correct.

> 
> This may also be easier to understand/claim if this patch is later in the series.

Sure.

> 
> 
>> +			hw_shareable |= ctrl_val;
>> +		}
>> +
>>  		for (i = r->cache.cbm_len - 1; i >= 0; i--) {
>>  			pseudo_locked = dom->plr ? dom->plr->cbm : 0;
>>  			hwb = test_bit(i, &hw_shareable);
> 
> Reinette
> 

-- 
Thanks
Babu Moger

Powered by blists - more mailing lists