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

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

Hi Reinitte,

On 9/19/2024 12:45 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/4/24 3:21 PM, Babu Moger wrote:
>> Users can modify the configuration of assignable events. Whenever the
>> event configuration is updated, MBM assignments must be revised across
>> all monitor groups within the impacted domains.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v7: New patch to update the assignments. Missed it earlier.
>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 53 ++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 1054583bef9d..0b1490d71e77 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -871,6 +871,15 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
>>    */
>>   #define MBM_EVENT_ARRAY_INDEX(_event) ((_event) - 2)
>>   
>> +static bool resctrl_mbm_event_assigned(struct rdtgroup *rdtg,
>> +				       struct rdt_mon_domain *d, u32 evtid)
>> +{
>> +	int index = MBM_EVENT_ARRAY_INDEX(evtid);
>> +	int cntr_id = rdtg->mon.cntr_id[index];
>> +
>> +	return  (cntr_id != MON_CNTR_UNSET && test_bit(cntr_id, d->mbm_cntr_map));
> 
> (Please check spaces and paren use.)

Sure.

> 
>> +}
>> +
>>   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().

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);

         /* 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);
         }
}


-- 
- Babu Moger