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

[Moger, Babu]

On 8/22/25 17:53, Moger, Babu wrote:
> Hi Reinette,
> 
> On 8/7/2025 8:49 PM, Reinette Chatre wrote:
>> 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.
> 
> 
> Sure.
> 
>>
>>> 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)?
> 
> May not be required.  Comments 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.
> 
> 
> Sure.
> 
>>
>>> +    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@xxxxxxxxx/
> 
> Let met try to understand.
> 
> So, rdtgroup_init_cat() sets up the staged _config for the specific CDP
> type for all the domains.
> 
> We need to apply those staged_configs to its peer type on all the domains.
> 
> Something like this?
> 
> /* Initialize staged_config of the peer type when CDP is enabled */
>         if (resctrl_arch_get_cdp_enabled(r->rid)) {
>                 list_for_each_entry(d, &s->res->ctrl_domains, hdr.list) {
>                         cfg = &d->staged_config[s->conf_type];
>                         cfg_peer = &d->staged_config[peer_type];
>                         cfg_peer->new_ctrl = cfg->new_ctrl;
>                         cfg_peer->have_new_ctrl = cfg->have_new_ctrl;
>                 }
>         }
> 

Replaced with following snippet.

/* 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);
+		list_for_each_entry(d, &s->res->ctrl_domains, hdr.list)
+			memcpy(&d->staged_config[peer_type],
+			       &d->staged_config[s->conf_type],
+			       sizeof(*d->staged_config));
+	}



> 
>>
>> 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 :(
> 
> 
> My bad.
> 
> I will address that.
> 
> Thanks
> 
> Babu
> 
> 

-- 
Thanks
Babu Moger