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

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 12/23/24 10:20, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/21/24 6:59 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 12/19/2024 9:12 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 12/12/24 12:15 PM, Babu Moger wrote:
>>>> Resctrl provides option to configure events by writing to the interfaces
>>>> /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config or
>>>> /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config when BMEC (Bandwidth
>>>> Monitoring Event Configuration) is supported.
>>>>
>>>> Whenever the event configuration is updated, MBM assignments must be
>>>> revised across all monitor groups within the impacted domains.
>>>
>>> This needs imperative tone description of what this patch does.
>>
>> Sure.
>>
>>>
>>>   ...
>>>
>>>> @@ -1825,6 +1825,54 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>>>       return 0;
>>>>   }
>>>>   +/*
>>>> + * Review the cntr_cfg domain configuration. If a matching assignment is found,
>>>> + * update the counter assignment accordingly. This is within the IPI Context,
>>>
>>> This "Review the cntr_cfg domain configuration. If a matching assignment is found,"
>>> is too vague for me to make sense of what it is trying to do. Can this be made more specific?
>>
>> Does this look ok?
>>
>> Check the counter configuration in the domain. If the specific event is configured, then update the assignment with the new event configuration value. This is within the IPI Context,  so call resctrl_abmc_config_one_amd directly"
> 
> I think it will be easier to understand what this function does if the
> comment is made more specific. For example:
> 	Update hardware counter configuration after event configuration change.         
>                                                                                 
> 	Walk the hardware counters of domain @d to reconfigure all assigned
> 	counters that are monitoring @evtid with the event's new configuration
> 	@mon_config (or @config_val).                                     
>                                                                                 
> 	This is run on a CPU belonging to domain @d so call                             
> 	resctrl_abmc_config_one_amd() directly.    

Looks good.  Thanks

> 
> Looking closer at architecture specific resctrl_arch_update_cntr() the
> reset of non-arch state (get_mbm_state()->memset()) seems out of place.
> There is a resctrl_arch_reset_rmid_all() within mbm_config_write_domain() that
> resets all architectural state after the event configuration is changed,
> should the non-architectural state not also be reset at that time? It looks

Moved the reset of non-arch state inside mbm_config_write_domain(). It
seems to work fine. Also I can simplify the IPI code further.


diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5f5cf9b3a053..ce08fb718e2e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2076,9 +2076,6 @@ static void resctrl_abmc_config_one_amd(void *info)
        abmc_cfg.split.bw_type = config->val;

        wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
-
-       resctrl_arch_reset_rmid(config->r, config->d, config->closid,
-                               config->rmid, config->evtid);
 }

 static int mbm_config_show(struct seq_file *s, struct rdt_resource *r,
u32 evtid)
@@ -2153,10 +2150,6 @@ static void resctrl_arch_update_cntr(struct
rdt_resource *r, struct rdt_mon_doma
                        config.assign = 1;

                        resctrl_abmc_config_one_amd(&config);
-
-                       m = get_mbm_state(d, rdtgrp->closid,
rdtgrp->mon.rmid, evtid);
-                       if (m)
-                               memset(m, 0, sizeof(struct mbm_state));
                }
        }
 }
@@ -2178,6 +2171,7 @@ static void resctrl_mon_event_config_set(void *info)
 static void mbm_config_write_domain(struct rdt_resource *r,
                                    struct rdt_mon_domain *d, u32 evtid,
u32 val)
 {
+       u32 idx_limit = resctrl_arch_system_num_rmid_idx();
        struct mon_config_info mon_info = {0};
        u32 config_val;

@@ -2214,6 +2208,12 @@ static void mbm_config_write_domain(struct
rdt_resource *r,
         * mbm_local and mbm_total counts for all the RMIDs.
         */
        resctrl_arch_reset_rmid_all(r, d);
+
+       if (is_mbm_total_enabled())
+               memset(d->mbm_total, 0, sizeof(struct mbm_state) * idx_limit);
+
+       if (is_mbm_local_enabled())
+               memset(d->mbm_local, 0, sizeof(struct mbm_state) * idx_limit);
 }

 static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)



> to me like it is something that may be needed for existing event
> configuration (but not an issue until Peter's new feature lands) and when done,
> the reset done within resctrl_arch_update_cntr() will no longer be necessary.
> 
> Something else to consider is the resctrl_arch_reset_rmid() within
> resctrl_abmc_config_one_amd() seems redundant on this call path since
> it is followed by resctrl_arch_reset_rmid_all(). resctrl_arch_reset_rmid() 
> does one MSR write and one MSR read for every counter that needs to be
> reconfigured and if that is unnecessary it may be worthwhile to optimize
> out?

Yes. Removed the resctrl_arch_reset_rmid() within
resctrl_abmc_config_one_amd().

Tested the code and seems to  work fine.

-- 
Thanks
Babu Moger