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
