Re: [PATCH v8 06/10] fs/resctrl: Add user interface to enable/disable io_alloc feature

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

Hi Babu,

On 8/5/25 4:30 PM, Babu Moger wrote:
> "io_alloc" feature in resctrl enables direct insertion of data from I/O
> devices into the cache.
> 
> On AMD systems, when io_alloc is enabled, the highest CLOSID is reserved
> exclusively for I/O allocation traffic and is no longer available for
> general CPU cache allocation. Users are encouraged to enable it only when
> running workloads that can benefit from this functionality.
> 
> Since CLOSIDs are managed by resctrl fs, it is least invasive to make the
> "io_alloc is supported by maximum supported CLOSID" part of the initial
> resctrl fs support for io_alloc. Take care not to expose this use of CLOSID
> for io_alloc to user space so that this is not required from other
> architectures that may support io_alloc differently in the future.
> 
> Introduce user interface to enable/disable io_alloc feature.

Please include high level overview of what this patch does to enable
and disable io_alloc. Doing so will help connect why the changelog contains
information about CLOSID management.

> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index d495a5d5c9d5..bf982eab7b18 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -685,3 +685,140 @@ int resctrl_io_alloc_show(struct kernfs_open_file *of, struct seq_file *seq, voi
>  
>  	return 0;
>  }
> +
> +/*
> + * resctrl_io_alloc_closid_supported() - io_alloc feature utilizes the
> + * highest CLOSID value to direct I/O traffic. Ensure that io_alloc_closid
> + * is in the supported range.
> + */
> +static bool resctrl_io_alloc_closid_supported(u32 io_alloc_closid)
> +{
> +	return io_alloc_closid < closids_supported();
> +}
> +
> +static struct resctrl_schema *resctrl_get_schema(enum resctrl_conf_type type)
> +{
> +	struct resctrl_schema *schema;
> +
> +	list_for_each_entry(schema, &resctrl_schema_all, list) {
> +		if (schema->conf_type == type)
> +			return schema;

This does not look right. More than one resource can have the same configuration type, no?
Think about L2 and L3 having CDP enabled ...
Looks like this is missing a resource type as parameter and a check for the resource ...
but is this function even necessary (more below)?

> +	}
> +
> +	return NULL;
> +}
> +
> +/*
> + * Initialize io_alloc CLOSID cache resource CBM with all usable (shared
> + * and unused) cache portions.
> + */
> +static int resctrl_io_alloc_init_cbm(struct resctrl_schema *s, u32 closid)
> +{
> +	struct rdt_resource *r = s->res;

Needs reverse fir.

> +	enum resctrl_conf_type peer_type;
> +	struct resctrl_schema *peer_s;
> +	int ret;
> +
> +	rdt_staged_configs_clear();
> +
> +	ret = rdtgroup_init_cat(s, closid);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Initialize schema for both CDP_DATA and CDP_CODE when CDP is enabled */
> +	if (resctrl_arch_get_cdp_enabled(r->rid)) {
> +		peer_type = resctrl_peer_type(s->conf_type);
> +		peer_s = resctrl_get_schema(peer_type);
> +		if (peer_s) {
> +			ret = rdtgroup_init_cat(peer_s, closid);

This is unexpected. In v7 I suggested that when parsing the CBM of one of the CDP
resources it is not necessary to do so again for the peer. The CBM can be
parsed *once* and the configuration just copied over. See:
https://lore.kernel.org/lkml/82045638-2b26-4682-9374-1c3e400a580a@intel.com/

Generally when feedback is provided it is good to check all places in series where
it is relevant. oh ... but looking ahead you ignored the feedback in the patch
it was given also :(

Reinette