Message-ID: <9818c304-9056-4d79-acbe-2b35cb847ecd@intel.com>
Date: Thu, 30 May 2024 13:24:57 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghua.yu@...el.com>, Maciej
 Wieczor-Retman <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v19 16/20] x86/resctrl: Make resctrl_arch_rmid_read()
 handle sum over domains

Hi Tony,

On 5/28/24 3:20 PM, Tony Luck wrote:
> Legacy resctrl monitor files must provide the sum of event values across
> all Sub-NUMA Cluster (SNC) domains that share an L3 cache instance.
> 
> Rename the existing resctrl_arch_rmid_read() function as
> resctrl_arch_rmid_read_one() (with some small refactoring to drop
> arguments that are not needed.

Missing closing ")".

> 
> Create a new resctrl_arch_rmid_read() that iterates across
> domains when necessary. Pass a CPU number from the right domain to
> resctrl_arch_rmid_read_one().

"when necessary"? Can you elaborate?
"Pass a CPU ..." that just describes code that can be learned from patch.
Please describe the changes not the code.

> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 41 ++++++++++++++++++++-------
>   1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 5f89ed4823ee..c9dd6ec68fcd 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -200,10 +200,9 @@ static inline struct rmid_entry *__rmid_entry(u32 idx)
>    * Caller is responsible to make sure execution running on a CPU in
>    * the domain to be read.
>    */
> -static int logical_rmid_to_physical_rmid(int lrmid)
> +static int logical_rmid_to_physical_rmid(int cpu, int lrmid)
>   {
>   	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> -	int cpu = smp_processor_id();
>   
>   	if (snc_nodes_per_l3_cache  == 1)
>   		return lrmid;
> @@ -211,13 +210,13 @@ static int logical_rmid_to_physical_rmid(int lrmid)
>   	return lrmid + (cpu_to_node(cpu) % snc_nodes_per_l3_cache) * r->num_rmid;
>   }
>   
> -static int __rmid_read(u32 lrmid,
> +static int __rmid_read(int cpu, u32 lrmid,
>   		       enum resctrl_event_id eventid, u64 *val)
>   {
>   	u64 msr_val;
>   	int prmid;
>   
> -	prmid = logical_rmid_to_physical_rmid(lrmid);
> +	prmid = logical_rmid_to_physical_rmid(cpu, lrmid);
>   	/*

Passing CPU as parameter to __rmid_read(), which is run via IPI, really
obfuscates the code. How about renaming it to "__rmid_read_phys()"
and provide it the "physical RMID" as parameter to make it clear what it is
doing?
That would mean an extra call to determine "physical RMID" before calling
__rmid_read_phys() but making it clear that it needs a physical RMID should
help to explain what is going on.


>   	 * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured
>   	 * with a valid event code for supported resource type and the bits
> @@ -269,7 +268,7 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
>   		memset(am, 0, sizeof(*am));
>   
>   		/* Record any initial, non-zero count value. */
> -		__rmid_read(rmid, eventid, &am->prev_msr);
> +		__rmid_read(smp_processor_id(), rmid, eventid, &am->prev_msr);
>   	}
>   }
>   
> @@ -298,9 +297,8 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
>   	return chunks >> shift;
>   }
>   
> -int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> -			   u32 unused, u32 rmid, enum resctrl_event_id eventid,
> -			   u64 *val, bool sum, struct cacheinfo *ci, void *ignored)
> +static int resctrl_arch_rmid_read_one(struct rdt_resource *r, struct rdt_mon_domain *d,
> +				      int cpu, u32 rmid, enum resctrl_event_id eventid, u64 *val)
>   {
>   	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> @@ -313,7 +311,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
>   	if (!cpumask_test_cpu(smp_processor_id(), &d->hdr.cpu_mask))
>   		return -EINVAL;
>   
> -	ret = __rmid_read(rmid, eventid, &msr_val);
> +	ret = __rmid_read(cpu, rmid, eventid, &msr_val);
>   	if (ret)
>   		return ret;
>   
> @@ -327,7 +325,30 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
>   		chunks = msr_val;
>   	}
>   
> -	*val = chunks * hw_res->mon_scale;
> +	*val += chunks * hw_res->mon_scale;

The various new layers of indirection with SNC logic scattered between them
makes this change hard to understand.

> +
> +	return 0;
> +}
> +
> +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			   u32 unused, u32 rmid, enum resctrl_event_id eventid,
> +			   u64 *val, bool sum, struct cacheinfo *ci, void *ignored)

This is not architecture specific code.

> +{
> +	int cpu = smp_processor_id();
> +	int ret;
> +
> +	*val = 0;
> +	if (!sum)
> +		return resctrl_arch_rmid_read_one(r, d, cpu, rmid, eventid, val);
> +

	/* SNC quirk that needs to be documented */

> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		if (d->ci->id != ci->id)
> +			continue;
> +		cpu = cpumask_any(&d->hdr.cpu_mask);
> +		ret = resctrl_arch_rmid_read_one(r, d, cpu, rmid, eventid, val);

The cpu parameter can be dropped, no? With the domain provided to resctrl_arch_rmid_read_one()
it is not necessary to again split the SNC logic (in this case the "reading from any CPU
in the cache domain is ok but still need accurate arch state") across multiple layers,
just contain this in (documented portion of) resctrl_arch_rmid_read_one().

> +		if (ret)
> +			return ret;
> +	}
>   
>   	return 0;
>   }

Reinette

Powered by blists - more mailing lists