linux-kernel - Re: [PATCH v7 22/24] 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: <384c2074-0076-4686-bebd-ba3ac3c05188@intel.com>
Date: Thu, 19 Sep 2024 10:45:17 -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 v7 22/24] x86/resctrl: Update assignments on event
 configuration changes

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

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

Reinette