Re: [RFC PATCH v3 17/17] x86/resctrl: Introduce interface to modify assignment states of the groups

[Moger, Babu]

Hi Peter,

On 4/17/24 12:45, Peter Newman wrote:
> Hi Babu,
> 
> On Thu, Mar 28, 2024 at 6:10 PM Babu Moger <babu.moger@xxxxxxx> wrote:
>>
>> Introduce rdtgroup_mbm_assign_control_write to assign mbm events.
>> Assignment state can be updated by writing to this interface.
>> Assignment states are applied on all the domains. Assignment on one
>> domain applied on all the domains. User can pass one valid domain and
>> assignment will be updated on all the available domains.
> 
> It sounds like you said the same thing 3 times in a row.

Sure. Will change it. With the introduction of domain specific assignment,
I can change it to something like this below.
------------------
"Introduce rdtgroup_mbm_assign_control_write to assign mbm events.

By default, the assignment is applied on all the domains when a new group
is created if the hardware counter is available at the time. This
interface provides the option to modify the assignment specific to each
domain."
------------------

> 
> 
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 2d96565501ab..64ec70637c66 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -328,6 +328,77 @@ with the following files:
>>          None of events are assigned on this mon group. This is a child
>>          monitor group of the non default control mon group.
>>
>> +       Assignment state can be updated by writing to this interface.
>> +
>> +       NOTE: Assignment on one domain applied on all the domains. User can
>> +       pass one valid domain and assignment will be updated on all the
>> +       available domains.
> 
> How would different assignments to different domains work? If the
> allocations are global, then the allocated monitor ID is available to
> all domains whether they use it or not.

That is correct.
[A] Hardware counters(max 2 per group) are allocated at the group level.
So, those counters are available to all the domains on that group. I will
maintain a bitmap at the domain level. The bitmap will be set on the
domains where assignment is applied and IPIs are sent. IPIs will not be
sent to other domains.

> 
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 9fd37b6c3b24..7f8b1386287a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1011,6 +1035,215 @@ static int rdtgroup_mbm_assign_control_show(struct kernfs_open_file *of,
>>         return 0;
>>  }
>>
>> +static struct rdtgroup *resctrl_get_rdtgroup(enum rdt_group_type rtype, char *p_grp, char *c_grp)
>> +{
>> +       struct rdtgroup *rdtg, *crg;
>> +
>> +       if (rtype == RDTCTRL_GROUP && *p_grp == '\0') {
>> +               return &rdtgroup_default;
>> +       } else if (rtype == RDTCTRL_GROUP) {
>> +               list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list)
>> +                       if (!strcmp(p_grp, rdtg->kn->name))
>> +                               return rdtg;
>> +       } else if (rtype == RDTMON_GROUP) {
>> +               list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
>> +                       if (!strcmp(p_grp, rdtg->kn->name)) {
>> +                               list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
>> +                                                   mon.crdtgrp_list) {
>> +                                       if (!strcmp(c_grp, crg->kn->name))
>> +                                               return crg;
>> +                               }
>> +                       }
>> +               }
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static int resctrl_process_flags(enum rdt_group_type rtype, char *p_grp, char *c_grp, char *tok)
>> +{
>> +       struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +       int op, mon_state, assign_state, unassign_state;
>> +       char *dom_str, *id_str, *op_str;
>> +       struct rdtgroup *rdt_grp;
>> +       struct rdt_domain *d;
>> +       unsigned long dom_id;
>> +       int ret, found = 0;
>> +
>> +       rdt_grp = resctrl_get_rdtgroup(rtype, p_grp, c_grp);
>> +
>> +       if (!rdt_grp) {
>> +               rdt_last_cmd_puts("Not a valid resctrl group\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +next:
>> +       if (!tok || tok[0] == '\0')
>> +               return 0;
>> +
>> +       /* Start processing the strings for each domain */
>> +       dom_str = strim(strsep(&tok, ";"));
>> +
>> +       op_str = strpbrk(dom_str, "=+-_");
>> +
>> +       if (op_str) {
>> +               op = *op_str;
>> +       } else {
>> +               rdt_last_cmd_puts("Missing operation =, +, -, _ character\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       id_str = strsep(&dom_str, "=+-_");
>> +
>> +       if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
>> +               rdt_last_cmd_puts("Missing domain id\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Verify if the dom_id is valid */
>> +       list_for_each_entry(d, &r->domains, list) {
>> +               if (d->id == dom_id) {
>> +                       found = 1;
>> +                       break;
>> +               }
>> +       }
>> +       if (!found) {
>> +               rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (op != '_')
>> +               mon_state = str_to_mon_state(dom_str);
>> +
>> +       assign_state = 0;
>> +       unassign_state = 0;
>> +
>> +       switch (op) {
>> +       case '+':
>> +               assign_state = mon_state;
>> +               break;
>> +       case '-':
>> +               unassign_state = mon_state;
>> +               break;
>> +       case '=':
>> +               assign_state = mon_state;
>> +               unassign_state = (ASSIGN_TOTAL | ASSIGN_LOCAL) & ~assign_state;
>> +               break;
>> +       case '_':
>> +               unassign_state = ASSIGN_TOTAL | ASSIGN_LOCAL;
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +
>> +       if (assign_state & ASSIGN_TOTAL)
>> +               ret = rdtgroup_assign_abmc(rdt_grp, QOS_L3_MBM_TOTAL_EVENT_ID,
>> +                                          ASSIGN_TOTAL);
> 
> Related to my comments yesterday[1], it seems redundant for an
> interface to need two names for the same event.

Yea. I will remove one of this parameter.

> 
> 
>> +       if (ret)
>> +               goto out_fail;
>> +
>> +       if (assign_state & ASSIGN_LOCAL)
>> +               ret = rdtgroup_assign_abmc(rdt_grp, QOS_L3_MBM_LOCAL_EVENT_ID,
>> +                                          ASSIGN_LOCAL);
>> +
>> +       if (ret)
>> +               goto out_fail;
>> +
>> +       if (unassign_state & ASSIGN_TOTAL)
>> +               ret = rdtgroup_unassign_abmc(rdt_grp, QOS_L3_MBM_TOTAL_EVENT_ID,
>> +                                            ASSIGN_TOTAL);
>> +       if (ret)
>> +               goto out_fail;
>> +
>> +       if (unassign_state & ASSIGN_LOCAL)
>> +               ret = rdtgroup_unassign_abmc(rdt_grp, QOS_L3_MBM_LOCAL_EVENT_ID,
>> +                                            ASSIGN_LOCAL);
>> +       if (ret)
>> +               goto out_fail;
>> +
>> +       goto next;
> 
> I saw that each call to rdtgroup_assign_abmc() allocates a counter.
> Does that mean assigning to multiple domains (in the same or multiple
> commands) allocates a new counter (or pair of counters) in every
> domain?

No. Counters allocation is at group level which is global. Will maintain a
bitmap at the domain to determine if the counter is assigned or unassigned
at the specific domain. Please see the comment above [A].

> 
> Thanks!
> -Peter
> 
> [1] https://lore.kernel.org/lkml/CALPaoCj_yb_muT78jFQ5gL0wkifohSAVwxMDTm2FX_2YVpANdw@xxxxxxxxxxxxxx/

-- 
Thanks
Babu Moger