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

[Reinette Chatre <reinette.chatre@intel.com>]

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


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

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

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

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

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

Reinette