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

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

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.    

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

Reinette