Date: Fri, 7 Jun 2024 09:49:04 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>
CC: 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>,
	<x86@...nel.org>, <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v19 06/20] x86/resctrl: Introduce snc_nodes_per_l3_cache

Hi Tony,

On 5/31/24 11:17 AM, Tony Luck wrote:
> On Thu, May 30, 2024 at 01:20:39PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 5/28/24 3:19 PM, Tony Luck wrote:
>>> Intel Sub-NUMA Cluster (SNC) is a feature that subdivides the CPU cores
>>> and memory controllers on a socket into two or more groups. These are
>>> presented to the operating system as NUMA nodes.
>>>
>>> This may enable some workloads to have slightly lower latency to memory
>>> as the memory controller(s) in an SNC node are electrically closer to the
>>> CPU cores on that SNC node. This cost may be offset by lower bandwidth
>>> since the memory accesses for each core can only be interleaved between
>>> the memory controllers on the same SNC node.
>>>
>>> Resctrl monitoring on an Intel system depends upon attaching RMIDs to tasks
>>> to track L3 cache occupancy and memory bandwidth. There is an MSR that
>>> controls how the RMIDs are shared between SNC nodes.
>>>
>>> The default mode divides them numerically. E.g. when there are two SNC
>>> nodes on a socket the lower number half of the RMIDs are given to the
>>> first node, the remainder to the second node. This would be difficult
>>> to use with the Linux resctrl interface as specific RMID values assigned
>>> to resctrl groups are not visible to users.
>>>
>>> The other mode divides the RMIDs and renumbers the ones on the second
>>> SNC node to start from zero.
>>>
>>> Even with this renumbering SNC mode requires several changes in resctrl
>>> behavior for correct operation.
>>>
>>> Add a static global to arch/x86/kernel/cpu/resctrl/monitor.c to indicate
>>> how many SNC domains share an L3 cache instance.  Initialize this to
>>> "1". Runtime detection of SNC mode will adjust this value.
>>>
>>> Update all places to take appropriate action when SNC mode is enabled:
>>> 1) The number of logical RMIDs per L3 cache available for use is the
>>>      number of physical RMIDs divided by the number of SNC nodes.
>>> 2) Likewise the "mon_scale" value must be divided by the number of SNC
>>>      nodes.
>>> 3) Add a function to convert from logical RMID values (assigned to
>>>      tasks and loaded into the IA32_PQR_ASSOC MSR on context switch)
>>>      to physical RMID values to load into IA32_QM_EVTSEL MSR when
>>>      reading counters on each SNC node.
>>>
>>> Signed-off-by: Tony Luck <tony.luck@...el.com>
>>> ---
>>>    arch/x86/kernel/cpu/resctrl/monitor.c | 37 ++++++++++++++++++++++++---
>>>    1 file changed, 33 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index 89d7e6fcbaa1..b9b4d2b5ca82 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -97,6 +97,8 @@ unsigned int resctrl_rmid_realloc_limit;
>>>    #define CF(cf)	((unsigned long)(1048576 * (cf) + 0.5))
>>> +static int snc_nodes_per_l3_cache = 1;
>>> +
>>>    /*
>>>     * The correction factor table is documented in Documentation/arch/x86/resctrl.rst.
>>>     * If rmid > rmid threshold, MBM total and local values should be multiplied
>>> @@ -185,10 +187,37 @@ static inline struct rmid_entry *__rmid_entry(u32 idx)
>>>    	return entry;
>>>    }
>>> -static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val)
>>> +/*
>>> + * When Sub-NUMA Cluster (SNC) mode is not enabled, the physical RMID
>>> + * is the same as the logical RMID.
>>> + *
>>> + * When SNC mode is enabled the physical RMIDs are distributed across
>>> + * the SNC nodes. E.g. with two SNC nodes per L3 cache and 200 physical
>>> + * RMIDs are divided with 0..99 on the first node and 100..199 on
>>> + * the second node. Compute the value of the physical RMID to pass to
>>> + * resctrl_arch_rmid_read().
>>
>> Please stop rushing version after version. I do not think you read the
>> above after you wrote it. The sentences run into each other.
> 
> Re-written. Would you like to try reviewing these patches one at a time
> as I fix them? That will:
> 
> a) Slow me down.
> b) Avoid me building subsequent patches on earlier mistakes.
> c) Give you bite-sized chunks to review in each sitting (I think
>     the overall direction of the series is well enough understood
>     at this point).

I've been thinking about this a lot ... but I am not able to understand
how you could draw your conclusions based on what I wrote.

I do not believe we need a new process. I find the basic patch
review process sufficient and would appreciate if that can be followed.
What I mean by this is: after a patch series is submitted it is reviewed,
any disagreement or clarification of review feedback is discussed during
review of that series, next series is submitted with all review feedback
addressed. Exceptions may be when there are such big changes that
a new version may be needed just to create a new baseline, but that does
not apply here.

> I've attached the updated version of patch 6 at the end of this e-mail.
> 
>> Could this be specific about what is meant by "physical" and "logical" RMID?
>> To me "physical RMID" implies the RMID used by hardware and "logical RMID"
>> is the RMID used by software ... but when it comes to SNC it is actually:
>> "physical RMID" - RMID used by MSR_IA32_QM_EVTSEL
>> "logical RMID" - RMID used by software and the MSR_IA32_PQR_ASSOC register
>>
>>> + *
>>> + * Caller is responsible to make sure execution running on a CPU in
>>
>> "is responsible" and "make sure" means the same, no?
>>
>> "make sure execution running"?
> 
> Also re-written.
> 
>> (Looking ahead in this series and coming back to this, this looks like
>> rushed work that you in turn expect folks spend quality time reviewing.)
>>
>>> + * the domain to be read.
>>> + */
>>> +static int logical_rmid_to_physical_rmid(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;
>>> +
>>> +	return lrmid + (cpu_to_node(cpu) % snc_nodes_per_l3_cache) * r->num_rmid;
>>> +}
>>> +
>>> +static int __rmid_read(u32 lrmid,
>>> +		       enum resctrl_event_id eventid, u64 *val)
>>
>> This line does not need to be split.
> 
> Joined now. I also pulled in your suggestion from a later patch to
> rename this __rmid_read_phys() and do the logical to physical RMID
> translation at the two callsites.
> 
>>>    {
>>>    	u64 msr_val;
>>> +	int prmid;
>>> +	prmid = logical_rmid_to_physical_rmid(lrmid);
>>>    	/*
>>>    	 * 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
>>> @@ -197,7 +226,7 @@ static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val)
>>>    	 * IA32_QM_CTR.Error (bit 63) and IA32_QM_CTR.Unavailable (bit 62)
>>>    	 * are error bits.
>>>    	 */
>>> -	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
>>> +	wrmsr(MSR_IA32_QM_EVTSEL, eventid, prmid);
>>>    	rdmsrl(MSR_IA32_QM_CTR, msr_val);
>>>    	if (msr_val & RMID_VAL_ERROR)
>>> @@ -1022,8 +1051,8 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>>    	int ret;
>>>    	resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
>>> -	hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale;
>>> -	r->num_rmid = boot_cpu_data.x86_cache_max_rmid + 1;
>>> +	hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;
>>> +	r->num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
>>>    	hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
>>>    	if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)
>>
>> Reinette
> 
> -Tony
> 
> Proposed v6 patch with fixes applied. I didn't include a URL
> to the RDT architecture spec I reference in the comment  for
> logical_rmid_to_physical_rmid() because Intel URLs are notoriously
> unstable. But I did check that a web search finds the document based on
> the title. With Google it was second hit for me. Bing lists it as first
> result.
> 
>  From ab33bacb9bf4dcf7b04310c1296b9dacddc4cd80 Mon Sep 17 00:00:00 2001
> From: Tony Luck <tony.luck@...el.com>
> Date: Thu, 30 May 2024 09:45:35 -0700
> Subject: [PATCH] x86/resctrl: Introduce snc_nodes_per_l3_cache
> 
> Intel Sub-NUMA Cluster (SNC) is a feature that subdivides the CPU cores
> and memory controllers on a socket into two or more groups. These are
> presented to the operating system as NUMA nodes.
> 
> This may enable some workloads to have slightly lower latency to memory
> as the memory controller(s) in an SNC node are electrically closer to the
> CPU cores on that SNC node. This cost may be offset by lower bandwidth
> since the memory accesses for each core can only be interleaved between
> the memory controllers on the same SNC node.
> 
> Resctrl monitoring on an Intel system depends upon attaching RMIDs to tasks
> to track L3 cache occupancy and memory bandwidth. There is an MSR that
> controls how the RMIDs are shared between SNC nodes.
> 
> The default mode divides them numerically. E.g. when there are two SNC
> nodes on a socket the lower number half of the RMIDs are given to the
> first node, the remainder to the second node. This would be difficult
> to use with the Linux resctrl interface as specific RMID values assigned
> to resctrl groups are not visible to users.
> 
> RMID sahring mode divides the RMIDs and renumbers the ones on the second

sahring -> sharing

> SNC node to start from zero.
> 
> Even with this renumbering SNC mode requires several changes in resctrl
> behavior for correct operation.
> 
> Add a static global to arch/x86/kernel/cpu/resctrl/monitor.c to indicate
> how many SNC domains share an L3 cache instance.  Initialize this to
> "1". Runtime detection of SNC mode will adjust this value.
> 
> Update all places to take appropriate action when SNC mode is enabled:
> 1) The number of logical RMIDs per L3 cache available for use is the
>     number of physical RMIDs divided by the number of SNC nodes.
> 2) Likewise the "mon_scale" value must be divided by the number of SNC
>     nodes.
> 3) Add a function to convert from logical RMID values (assigned to
>     tasks and loaded into the IA32_PQR_ASSOC MSR on context switch)
>     to physical RMID values to load into IA32_QM_EVTSEL MSR when
>     reading counters on each SNC node.
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 52 +++++++++++++++++++++++----
>   1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 89d7e6fcbaa1..0b05dfb5ab67 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -97,6 +97,8 @@ unsigned int resctrl_rmid_realloc_limit;
>   
>   #define CF(cf)	((unsigned long)(1048576 * (cf) + 0.5))
>   
> +static int snc_nodes_per_l3_cache = 1;
> +
>   /*
>    * The correction factor table is documented in Documentation/arch/x86/resctrl.rst.
>    * If rmid > rmid threshold, MBM total and local values should be multiplied
> @@ -185,7 +187,39 @@ static inline struct rmid_entry *__rmid_entry(u32 idx)
>   	return entry;
>   }
>   
> -static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val)
> +/*
> + * When Sub-NUMA Cluster (SNC) mode is not enabled the RMID value
> + * loaded into IA32_PQR_ASSOC for the CPU to accumulate data is
> + * the same as the RMID value loaded into IA32_QM_EVTSEL to
> + * retrieve the current value of counters from IA32_QM_CTR.

The function is logical_rmid_to_physical_rmid() and it is called
whether SNC is enabled or not. Above provides summary of expectations
when SNC is not enabled but there is no mention of "logical" or
"physical" RMID to understand what this means for non SNC.

> + *
> + * When SNC mode is enabled in RMID sharing mode there are fewer

What is difference between "SNC mode" and "RMID sharing mode"?
Please provide explanations about what terms mean and then be
consistent in their use.
This new version seems to be the first time "RMID sharing mode"
is used but there is no explanation about what that means.
Only previous mention was in subject of patch #18 but the comment
of  arch_mon_domain_online() has no mention of the term
"RMID sharing mode" in the place where that mode is actually
enabled.

> + * RMID values available to accumulate data (RMIDs are divided
> + * evenly between SNC nodes that share an L3 cache). Here we refer
> + * to the value loaded into IA32_PQR_ASSOC as the "logical RMID".

Please do not use "we".

> + *
> + * Data is collected independently on each SNC node and can be retrieved
> + * using the "physical RMID" value computed by this function. The
> + * cpu argument can be any CPU in the SNC domain for the node.

"The cpu argument" -> "@cpu"

"SNC domain for the node" - what does "the node" refer to in this context?
Later it becomes "SNC node domain"? So far this comment has "SNC node",
"SNC domain for the node", "SNC node domain" that all seem to refer to
the same thing. Inconsistent terminology adds unnecessary complication.

> + *
> + * Note that the scope of the IA32_QM_EVTSEL and IA32_QM_CTR MSRs is
> + * still at the L3 cache scope. So a physical RMID may be read from any

"The scope of the IA32_QM_EVTSEL and IA32_QM_CTR MSRs is at the L3 cache."

Pick if you want "physical RMID" in quotes or not and then stick to it.

> + * CPU that shares the L3 cache with the desired SNC node domain.
> + *
> + * For more details and examples see the "RMID Sharing Mode" section
> + * in the "Intel Resource Director Technology Architecture Specification".
> + */

Reinette

Powered by blists - more mailing lists