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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <03b278b5-6c15-4d09-9ab7-3317e84a409e@intel.com>
Date: Tue, 15 Oct 2024 20:40:29 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <fenghua.yu@...el.com>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
	<rdunlap@...radead.org>, <tj@...nel.org>, <peterz@...radead.org>,
	<yanjiewtw@...il.com>, <kim.phillips@....com>, <lukas.bulwahn@...il.com>,
	<seanjc@...gle.com>, <jmattson@...gle.com>, <leitao@...ian.org>,
	<jpoimboe@...nel.org>, <rick.p.edgecombe@...el.com>,
	<kirill.shutemov@...ux.intel.com>, <jithu.joseph@...el.com>,
	<kai.huang@...el.com>, <kan.liang@...ux.intel.com>,
	<daniel.sneddon@...ux.intel.com>, <pbonzini@...hat.com>,
	<sandipan.das@....com>, <ilpo.jarvinen@...ux.intel.com>,
	<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<eranian@...gle.com>, <james.morse@....com>
Subject: Re: [PATCH v8 23/25] x86/resctrl: Update assignments on event
 configuration changes

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@....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.

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


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.

Reinette