Re: [PATCH v7 22/24] x86/resctrl: Update assignments on event configuration changes

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

Hi Babu,

On 9/27/24 9:22 AM, Moger, Babu wrote:
> Hi Reinitte,
> 
> On 9/19/2024 12:45 PM, Reinette Chatre wrote:
>> On 9/4/24 3:21 PM, Babu Moger wrote:

...

>>> +}
>>> +
>>>   static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
>>>                        struct seq_file *s, void *v)
>>>   {
>>> @@ -1793,12 +1802,48 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>>       return 0;
>>>   }
>>>   +static int resctrl_mbm_event_update_assign(struct rdt_resource *r,
>>> +                       struct rdt_mon_domain *d, u32 evtid)
>>> +{
>>> +    struct rdt_mon_domain *dom;
>>> +    struct rdtgroup *rdtg;
>>> +    int ret = 0;
>>> +
>>> +    if (!resctrl_arch_mbm_cntr_assign_enabled(r))
>>> +        return ret;
>>> +
>>> +    list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
>>> +        struct rdtgroup *crg;
>>> +
>>> +        list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>>> +            if (d == dom && resctrl_mbm_event_assigned(rdtg, dom, evtid)) {
>>> +                ret = rdtgroup_assign_cntr(r, rdtg, dom, evtid);
>>> +                if (ret)
>>> +                    goto out_done;
>>> +            }
>>> +        }
>>> +
>>> +        list_for_each_entry(crg, &rdtg->mon.crdtgrp_list, mon.crdtgrp_list) {
>>> +            list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>>> +                if (d == dom && resctrl_mbm_event_assigned(crg, dom, evtid)) {
>>> +                    ret = rdtgroup_assign_cntr(r, crg, dom, evtid);
>>> +                    if (ret)
>>> +                        goto out_done;
>>> +                }
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +out_done:
>>> +    return ret;
>>> +}
>>>     static void mbm_config_write_domain(struct rdt_resource *r,
>>>                       struct rdt_mon_domain *d, u32 evtid, u32 val)
>>>   {
>>>       struct mon_config_info mon_info = {0};
>>>       u32 config_val;
>>> +    int ret;
>>>         /*
>>>        * Check the current config value first. If both are the same then
>>> @@ -1822,6 +1867,14 @@ static void mbm_config_write_domain(struct rdt_resource *r,
>>>                     resctrl_arch_event_config_set,
>>>                     &mon_info, 1);
>>>   +    /*
>>> +     * Counter assignments needs to be updated to match the event
>>> +     * configuration.
>>> +     */
>>> +    ret = resctrl_mbm_event_update_assign(r, d, evtid);
>>> +    if (ret)
>>> +        rdt_last_cmd_puts("Assign failed, event will be Unavailable\n");
>>> +
>>
>> This does not look right. This function _just_ returned from an IPI on appropriate CPU and then
>> starts flow to do _another_ IPI to run code that could have just been run during previous IPI.
>> The whole flow to call rdgroup_assign_cntr() also seems like an obfuscated way to call resctrl_arch_assign_cntr()
>> to just reconfigure the counter (not actually assign it).
>> Could it perhaps call some resctrl fs code via single IPI that in turn calls the appropriate arch code to set the new
>> mon event config and re-configures the counter?
>>
> 
> I think we can simplify this. We dont have to go thru all the rdtgroups to figure out if the counter is assigned or not.
> 
> I can move the code inside mon_config_write() after the call mbm_config_write_domain().

mbm_config_write_domain() already does an IPI so if I understand correctly this will still
result in a second IPI that seems unnecessary to me. Why can the reconfigure not be done
with a single IPI?

> 
> Using the domain bitmap we can figure out which of the counters are assigned in the domain. I can use the hardware help to update the assignment for each counter.  This has to be done via IPI.
> Something like this.
> 
> static void rdtgroup_abmc_dom_cfg(void *info)
> {
>     union l3_qos_abmc_cfg *abmc_cfg = info;
>         u32 val = abmc_cfg->bw_type;
> 
>         /* Get the counter configuration */
>     wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *abmc_cfg);
>     rdmsrl(MGR_IA32_L3_QOS_ABMC_DSC, *abmc_cfg);
> 

This is not clear to me. I expected MSR_IA32_L3_QOS_ABMC_DSC
to return the bw_type that was just written to 
MSR_IA32_L3_QOS_ABMC_CFG. 

It is also not clear to me how these registers can be
used without a valid counter ID. I seem to miss
the context of this call.

>         /* update the counter configuration */
>         if (abmc_cfg->bw_type != val) {
>             abmc_cfg->bw_type = val;
>             abmc_cfg.split.cfg_en = 1;
>             wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *abmc_cfg);
>         }
> }
> 
> 

Reinette