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

[Peter Newman]

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.


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


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


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

Thanks!
-Peter

[1] https://lore.kernel.org/lkml/CALPaoCj_yb_muT78jFQ5gL0wkifohSAVwxMDTm2FX_2YVpANdw@xxxxxxxxxxxxxx/