Re: [PATCH v8 23/25] x86/resctrl: Update assignments on event configuration changes

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

Hi Reinette,

On 10/15/2024 10:40 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/9/24 10:39 AM, 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>
>> ---
> ...
> 
>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 49 ++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index f890d294e002..cf2e0ad0e4f4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1669,6 +1669,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
>>   }
>>   
>>   struct mon_config_info {
>> +	struct rdt_resource *r;
>>   	struct rdt_mon_domain *d;
>>   	u32 evtid;
>>   	u32 mon_config;
>> @@ -1694,11 +1695,46 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>>   	return INVALID_CONFIG_VALUE;
>>   }
>>   
>> +static void mbm_cntr_event_update(int cntr_id, unsigned int index, u32 val)
>> +{
>> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
>> +	struct rdtgroup *prgrp, *crgrp;
>> +	int update = 0;
>> +
>> +	/* Check if the cntr_id is associated to the event type updated */
>> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>> +		if (prgrp->mon.cntr_id[index] == cntr_id) {
>> +			abmc_cfg.split.bw_src = prgrp->mon.rmid;
>> +			update = 1;
>> +			goto out_update;
>> +		}
>> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
>> +			if (crgrp->mon.cntr_id[index] == cntr_id) {
>> +				abmc_cfg.split.bw_src = crgrp->mon.rmid;
>> +				update = 1;
>> +				goto out_update;
>> +			}
>> +		}
> 
> This code looks like it is better suited for resctrl fs. Note that
> after the arch fs split struct rdtgroup is private to resctrl fs.

ok

> 
>> +	}
>> +
>> +out_update:
>> +	if (update) {
>> +		abmc_cfg.split.cfg_en = 1;
>> +		abmc_cfg.split.cntr_en = 1;
>> +		abmc_cfg.split.cntr_id = cntr_id;
>> +		abmc_cfg.split.bw_type = val;
>> +		wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
>> +	}
>> +}
>> +
>>   void resctrl_arch_mon_event_config_set(void *info)
>>   {
>>   	struct mon_config_info *mon_info = info;
>> +	struct rdt_mon_domain *d = mon_info->d;
>> +	struct rdt_resource *r = mon_info->r;
>>   	struct rdt_hw_mon_domain *hw_dom;
>>   	unsigned int index;
>> +	int cntr_id;
>>   
>>   	index = mon_event_config_index_get(mon_info->evtid);
>>   	if (index == INVALID_CONFIG_INDEX)
>> @@ -1718,6 +1754,18 @@ void resctrl_arch_mon_event_config_set(void *info)
>>   		hw_dom->mbm_local_cfg =  mon_info->mon_config;
>>   		break;
>>   	}
>> +
>> +	/*
>> +	 * Update the assignment if the domain has the cntr_id's assigned
>> +	 * to event type updated.
>> +	 */
>> +	if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> +		for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
>> +			if (test_bit(cntr_id, d->mbm_cntr_map))
>> +				mbm_cntr_event_update(cntr_id, index,
>> +						      mon_info->mon_config);
>> +		}
>> +	}
>>   }
>>   
>>   /**
>> @@ -1805,6 +1853,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
>>   	mon_info.d = d;
>>   	mon_info.evtid = evtid;
>>   	mon_info.mon_config = val;
>> +	mon_info.r = r;
>>   
>>   	/*
>>   	 * Update MSR_IA32_EVT_CFG_BASE MSR on one of the CPUs in the
> 
> If I understand correctly, mbm_config_write_domain() paints itself into a corner by
> calling arch code via IPI. As seen above it needs resctrl help to get all the information
> and doing so from the arch helper is not appropriate.
> 
> How about calling a resctrl fs helper via IPI instead? For example:
> 
> resctrl_mon_event_config_set() {
> 
> 	resctrl_arch_mon_event_config_set();
> 
> 	if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
> 		for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> 			if (test_bit(cntr_id, d->mbm_cntr_map)) {
> 				/* determine rmid */
> 				resctrl_arch_config_cntr()

The call resctrl_arch_config_cntr() requires both RMID and CLOSID. So, 
we will have to find the rdtgroup here (not just RMID, we need CLOSID also).

Yea. I think it can be done. Let me try.


> 			}
> 		}
> 	}
> }
> 
> 
> mbm_config_write_domain() {
> 
> 	...
> 	smp_call_function_any(&d->hdr.cpu_mask, resctrl_mon_event_config_set, ...)
> 	...
> 
> }
> 
> By removing reset of arch state from resctrl_arch_config_cntr() this works well with the
> resctrl_arch_reset_rmid_all() that is done from mbm_config_write_domain().
> Even though resctrl_arch_config_cntr() contains a smp_call_function_any() it should
> already be running on CPU in mask and thus should just run on local CPU.

Ok.
-- 
- Babu Moger