Re: [PATCH v8 07/10] fs/resctrl: Introduce interface to display io_alloc CBMs

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 8/7/25 20:51, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/5/25 4:30 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.
>>
>> Add "io_alloc_cbm" resctrl file to display CBMs (Capacity Bit Mask) of
>> io_alloc feature.
> 
> This is a bit vague. How about:
> 	Add "io_alloc_cbm" resctrl file to display the Capacity Bit Masks
> 	(CBMs) that represent the portion of each cache instance allocated
> 	for I/O traffic.

Sure.

>>
>> The CBM interface file io_alloc_cbm resides in the info directory
>> (e.g., /sys/fs/resctrl/info/L3/). Displaying the resource name is not
>> necessary. Pass the resource name to show_doms() and print it only if
> 
> "Displaying the resource name is not necessary." -> "Since the
> resource name is part of the path it is not necessary to display the
> resource name as done in the schemata file."?

Sure.

> 
> 
>> the name is valid. For io_alloc, pass NULL to suppress printing the
>> resource name.
>>
>> When CDP is enabled, io_alloc routes traffic using the highest CLOSID
>> associated with the L3CODE resource. To ensure consistent cache allocation
>> behavior, the L3CODE and L3DATA resources must remain synchronized.
> 
> "must remain synchronized" -> "are kept in sync"

Sure.

> 
>> rdtgroup_init_cat() function takes both L3CODE and L3DATA into account when
> 
> I do not understand this part. rdtgroup_init_cat() is part of current implementation
> and it takes L3CODE and L3DATE of _other_ CLOSID into account when
> determining what CBM to initialize new CLOSID with. How is that relevant
> here? I wonder if you are not perhaps trying to say:
> "resctrl_io_alloc_init_cbm() initializes L3CODE and L3DATA of highest CLOSID
>  with the same CBM." 
> I do not think this is necessary to include here though since this is what the
> previous patch does and just saying that L3CODE and L3DATA are kept in sync is
> sufficient here.

Ok. Sounds good.
> 
>> initializing CBMs for new groups.  The io_alloc feature adheres to this
>> same principle, meaning that the Cache Bit Masks (CBMs) accessed through
>> either L3CODE or L3DATA will reflect identical values.
> 
> I do not understand what you are trying to say here. What do you mean with
> "same principle"? The fact that L3CODE and L3DATA are kept in sync is
> part of io_alloc only, no?

Yes. That is correct. I will remove that text.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> ---
> 
> ...
> 
>> +int resctrl_io_alloc_cbm_show(struct kernfs_open_file *of, struct seq_file *seq, void *v)
>> +{
>> +	struct resctrl_schema *s = rdt_kn_parent_priv(of->kn);
>> +	struct rdt_resource *r = s->res;
>> +	int ret = 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 = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
> 
> Could you please add a comment here that explains to the reader that CBMs of
> L3CODE and L3DATA are kept in sync elsewhere and the io_alloc CBMs displayed from
> either CDP resource are thus identical and accurately reflect the CBMs used
> for I/O.

Sure.

> 
>> +	show_doms(seq, s, NULL, resctrl_io_alloc_closid(r));
>> +
>> +out_unlock:
>> +	mutex_unlock(&rdtgroup_mutex);
>> +	cpus_read_unlock();
>> +	return ret;
>> +}
> 
> Reinette
> 

-- 
Thanks
Babu Moger