There isn't a simple h/w bit that indicates whether a CPU is
running in Sub NUMA Cluster mode. Infer the state by comparing
the ratio of NUMA nodes to L3 cache instances.

When SNC mode is detected, reconfigure the RMID counters by updating
the MSR_RMID_SNC_CONFIG MSR on each socket as CPUs are seen.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/msr-index.h   |  1 +
 arch/x86/kernel/cpu/resctrl/core.c | 68 ++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d111350197f..393d1b047617 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1100,6 +1100,7 @@
 #define MSR_IA32_QM_CTR			0xc8e
 #define MSR_IA32_PQR_ASSOC		0xc8f
 #define MSR_IA32_L3_CBM_BASE		0xc90
+#define MSR_RMID_SNC_CONFIG		0xca0
 #define MSR_IA32_L2_CBM_BASE		0xd10
 #define MSR_IA32_MBA_THRTL_BASE		0xd50
 
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index ed4f55b3e5e4..9f0ac9721fab 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -16,11 +16,14 @@
 
 #define pr_fmt(fmt)	"resctrl: " fmt
 
+#include <linux/cpu.h>
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/cacheinfo.h>
 #include <linux/cpuhotplug.h>
+#include <linux/mod_devicetable.h>
 
+#include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/resctrl.h>
 #include "internal.h"
@@ -724,11 +727,34 @@ static void clear_closid_rmid(int cpu)
 	wrmsr(MSR_IA32_PQR_ASSOC, 0, 0);
 }
 
+/*
+ * The power-on reset value of MSR_RMID_SNC_CONFIG is 0x1
+ * which indicates that RMIDs are configured in legacy mode.
+ * Clearing bit 0 reconfigures the RMID counters for use
+ * in Sub NUMA Cluster mode.
+ */
+static void snc_remap_rmids(int cpu)
+{
+	u64 val;
+
+	/* Only need to enable once per package */
+	if (cpumask_first(topology_core_cpumask(cpu)) != cpu)
+		return;
+
+	rdmsrl(MSR_RMID_SNC_CONFIG, val);
+	val &= ~BIT_ULL(0);
+	wrmsrl(MSR_RMID_SNC_CONFIG, val);
+}
+
 static int resctrl_online_cpu(unsigned int cpu)
 {
 	struct rdt_resource *r;
 
 	mutex_lock(&rdtgroup_mutex);
+
+	if (snc_nodes_per_l3_cache > 1)
+		snc_remap_rmids(cpu);
+
 	for_each_capable_rdt_resource(r)
 		domain_add_cpu(cpu, r);
 	/* The cpu is set in default rdtgroup after online. */
@@ -983,11 +1009,53 @@ static __init bool get_rdt_resources(void)
 	return (rdt_mon_capable || rdt_alloc_capable);
 }
 
+/* CPU models that support MSR_RMID_SNC_CONFIG */
+static const struct x86_cpu_id snc_cpu_ids[] __initconst = {
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, 0),
+	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, 0),
+	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, 0),
+	{}
+};
+
+static __init int get_snc_config(void)
+{
+	unsigned long *node_caches;
+	int mem_only_nodes = 0;
+	int cpu, node, ret;
+
+	if (!x86_match_cpu(snc_cpu_ids))
+		return 1;
+
+	node_caches = kcalloc(BITS_TO_LONGS(nr_node_ids), sizeof(*node_caches), GFP_KERNEL);
+	if (!node_caches)
+		return 1;
+
+	cpus_read_lock();
+	for_each_node(node) {
+		cpu = cpumask_first(cpumask_of_node(node));
+		if (cpu < nr_cpu_ids)
+			set_bit(get_cpu_cacheinfo_id(cpu, 3), node_caches);
+		else
+			mem_only_nodes++;
+	}
+	cpus_read_unlock();
+
+	ret = (nr_node_ids - mem_only_nodes) / bitmap_weight(node_caches, nr_node_ids);
+	kfree(node_caches);
+
+	if (ret > 1)
+		rdt_resources_all[RDT_RESOURCE_L3].r_resctrl.mon_scope = RESCTRL_NODE;
+
+	return ret;
+}
+
 static __init void rdt_init_res_defs_intel(void)
 {
 	struct rdt_hw_resource *hw_res;
 	struct rdt_resource *r;
 
+	snc_nodes_per_l3_cache = get_snc_config();
+
 	for_each_rdt_resource(r) {
 		hw_res = resctrl_to_arch_res(r);
 
-- 
2.41.0

Hi Tony,

On 8/29/23 18:44, Tony Luck wrote:
> There isn't a simple h/w bit that indicates whether a CPU is
> running in Sub NUMA Cluster mode. Infer the state by comparing
> the ratio of NUMA nodes to L3 cache instances.
> 
> When SNC mode is detected, reconfigure the RMID counters by updating
> the MSR_RMID_SNC_CONFIG MSR on each socket as CPUs are seen.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/msr-index.h   |  1 +
>  arch/x86/kernel/cpu/resctrl/core.c | 68 ++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 1d111350197f..393d1b047617 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1100,6 +1100,7 @@
>  #define MSR_IA32_QM_CTR			0xc8e
>  #define MSR_IA32_PQR_ASSOC		0xc8f
>  #define MSR_IA32_L3_CBM_BASE		0xc90
> +#define MSR_RMID_SNC_CONFIG		0xca0
>  #define MSR_IA32_L2_CBM_BASE		0xd10
>  #define MSR_IA32_MBA_THRTL_BASE		0xd50
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index ed4f55b3e5e4..9f0ac9721fab 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -16,11 +16,14 @@
>  
>  #define pr_fmt(fmt)	"resctrl: " fmt
>  
> +#include <linux/cpu.h>
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/cacheinfo.h>
>  #include <linux/cpuhotplug.h>
> +#include <linux/mod_devicetable.h>

I didnt see the need for this include.
>  
> +#include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
>  #include <asm/resctrl.h>
>  #include "internal.h"
> @@ -724,11 +727,34 @@ static void clear_closid_rmid(int cpu)
>  	wrmsr(MSR_IA32_PQR_ASSOC, 0, 0);
>  }
>  
> +/*
> + * The power-on reset value of MSR_RMID_SNC_CONFIG is 0x1
> + * which indicates that RMIDs are configured in legacy mode.
> + * Clearing bit 0 reconfigures the RMID counters for use
> + * in Sub NUMA Cluster mode.
> + */
> +static void snc_remap_rmids(int cpu)

While adding the new functions, i see that new function names start with
resctrl_ prefix.  However, we are all not very consistent. Can ypu rename
this function to resctrl_snc_remap_rmids?


> +{
> +	u64 val;
> +
> +	/* Only need to enable once per package */
> +	if (cpumask_first(topology_core_cpumask(cpu)) != cpu)
> +		return;
> +
> +	rdmsrl(MSR_RMID_SNC_CONFIG, val);
> +	val &= ~BIT_ULL(0);
> +	wrmsrl(MSR_RMID_SNC_CONFIG, val);
> +}
> +
>  static int resctrl_online_cpu(unsigned int cpu)
>  {
>  	struct rdt_resource *r;
>  
>  	mutex_lock(&rdtgroup_mutex);
> +
> +	if (snc_nodes_per_l3_cache > 1)
> +		snc_remap_rmids(cpu);
> +
>  	for_each_capable_rdt_resource(r)
>  		domain_add_cpu(cpu, r);
>  	/* The cpu is set in default rdtgroup after online. */
> @@ -983,11 +1009,53 @@ static __init bool get_rdt_resources(void)
>  	return (rdt_mon_capable || rdt_alloc_capable);
>  }
>  
> +/* CPU models that support MSR_RMID_SNC_CONFIG */
> +static const struct x86_cpu_id snc_cpu_ids[] __initconst = {
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, 0),
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, 0),
> +	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, 0),
> +	{}
> +};
> +
> +static __init int get_snc_config(void)

Same comment as above.

> +{
> +	unsigned long *node_caches;
> +	int mem_only_nodes = 0;
> +	int cpu, node, ret;
> +
> +	if (!x86_match_cpu(snc_cpu_ids))
> +		return 1;
> +
> +	node_caches = kcalloc(BITS_TO_LONGS(nr_node_ids), sizeof(*node_caches), GFP_KERNEL);
> +	if (!node_caches)
> +		return 1;
> +
> +	cpus_read_lock();
> +	for_each_node(node) {
> +		cpu = cpumask_first(cpumask_of_node(node));
> +		if (cpu < nr_cpu_ids)
> +			set_bit(get_cpu_cacheinfo_id(cpu, 3), node_caches);
> +		else
> +			mem_only_nodes++;
> +	}
> +	cpus_read_unlock();
> +
> +	ret = (nr_node_ids - mem_only_nodes) / bitmap_weight(node_caches, nr_node_ids);
> +	kfree(node_caches);
> +
> +	if (ret > 1)
> +		rdt_resources_all[RDT_RESOURCE_L3].r_resctrl.mon_scope = RESCTRL_NODE;
> +
> +	return ret;
> +}
> +
>  static __init void rdt_init_res_defs_intel(void)
>  {
>  	struct rdt_hw_resource *hw_res;
>  	struct rdt_resource *r;
>  
> +	snc_nodes_per_l3_cache = get_snc_config();
> +
>  	for_each_rdt_resource(r) {
>  		hw_res = resctrl_to_arch_res(r);
>  

-- 
Thanks
Babu Moger

>> +#include <linux/mod_devicetable.h>
>
> I didn't see the need for this include.

struct x86_cpu_id is defined in this #include file.

>> +static void snc_remap_rmids(int cpu)
>
> While adding the new functions, i see that new function names start with
> resctrl_ prefix.  However, we are all not very consistent. Can ypu rename
> this function to resctrl_snc_remap_rmids?

I try to put a subsystem prefix on any global symbols to avoid random
conflicts in other parts of the kernel. But I'm less sure of the value for
static functions and variables that are only visible inside a single ".c"
file.

If it must have a prefix, should it be "intel_" rather than "resctrl_" to
indicate that it is an Intel specific function?


>> +static __init int get_snc_config(void)
>
> Same comment as above.

Same answer.


Reinette: Any opinions on these?

-Tony

Hi Tony,

On 9/26/2023 12:40 PM, Luck, Tony wrote:
>>> +#include <linux/mod_devicetable.h>
>>
>> I didn't see the need for this include.
> 
> struct x86_cpu_id is defined in this #include file.
> 
>>> +static void snc_remap_rmids(int cpu)
>>
>> While adding the new functions, i see that new function names start with
>> resctrl_ prefix.  However, we are all not very consistent. Can ypu rename
>> this function to resctrl_snc_remap_rmids?
> 
> I try to put a subsystem prefix on any global symbols to avoid random
> conflicts in other parts of the kernel. But I'm less sure of the value for
> static functions and variables that are only visible inside a single ".c"
> file.
> 
> If it must have a prefix, should it be "intel_" rather than "resctrl_" to
> indicate that it is an Intel specific function?
> 
> 
>>> +static __init int get_snc_config(void)
>>
>> Same comment as above.
> 
> Same answer.
> 
> 
> Reinette: Any opinions on these?
> 

I agree with you that a consistent prefix is expected from the global
symbols and a prefix of resctrl_ is indeed the goal as can be seen in
the growing include/linux/resctrl.h. resctrl has no established pattern
for static functions (look at the other static functions in this file
being changed) or even those non-static functions and data
structures shared between the resctrl files (see
arch/x86/kernel/cpu/resctrl/internal.h).

I'm ok with the static functions named snc_remap_rmids() and get_snc_config().

We can surely start a discussion if there is concern about resctrl static
function prefixes but that discussion will have larger scope than
this series.

If we do want to focus on function naming I do think a change that may
benefit this work is to be consistent with verb placement in the function
names. i.e either always have verb first or have a consistent snc_ prefix
followed by a verb. I do not recall if there are x86 requirements in this
regard.

Reinette

On 9/26/23 14:40, Luck, Tony wrote:
>>> +#include <linux/mod_devicetable.h>
>>
>> I didn't see the need for this include.
> 
> struct x86_cpu_id is defined in this #include file.

Actually, the file <asm/cpu_device_id.h> already includes the above file.

> 
>>> +static void snc_remap_rmids(int cpu)
>>
>> While adding the new functions, i see that new function names start with
>> resctrl_ prefix.  However, we are all not very consistent. Can ypu rename
>> this function to resctrl_snc_remap_rmids?
> 
> I try to put a subsystem prefix on any global symbols to avoid random
> conflicts in other parts of the kernel. But I'm less sure of the value for
> static functions and variables that are only visible inside a single ".c"
> file.
> 
> If it must have a prefix, should it be "intel_" rather than "resctrl_" to
> indicate that it is an Intel specific function?

If you add it it should be resctrl_.

Thanks
Babu
> 
> 
>>> +static __init int get_snc_config(void)
>>
>> Same comment as above.
> 
> Same answer.
> 
> 
> Reinette: Any opinions on these?
> 
> -Tony

>>>> +#include <linux/mod_devicetable.h>
>>>
>>> I didn't see the need for this include.
>> 
>> struct x86_cpu_id is defined in this #include file.
>
> Actually, the file <asm/cpu_device_id.h> already includes the above file.

It does today. Will it include it next week when somebody has to
re-arrange things to resolve some #include dependency?

I thought there was a guideline somewhere that says to #include
the files that define the things you use. Not just rely on chains of
#includes. But some simple grep's haven't found that written in
Documentation/*

-Tony

On 9/26/23 15:02, Luck, Tony wrote:
>>>>> +#include <linux/mod_devicetable.h>
>>>>
>>>> I didn't see the need for this include.
>>>
>>> struct x86_cpu_id is defined in this #include file.
>>
>> Actually, the file <asm/cpu_device_id.h> already includes the above file.
> 
> It does today. Will it include it next week when somebody has to
> re-arrange things to resolve some #include dependency?
> 
> I thought there was a guideline somewhere that says to #include
> the files that define the things you use. Not just rely on chains of
> #includes. But some simple grep's haven't found that written in
> Documentation/*

Yea. correct. There is no guidelines. Lets keep the #include
<linux/mod_devicetable.h>

-- 
Thanks
Babu Moger

Hi Tony,

On 8/29/2023 4:44 PM, Tony Luck wrote:
> There isn't a simple h/w bit that indicates whether a CPU is
> running in Sub NUMA Cluster mode. Infer the state by comparing

Sub NUMA Cluster (SNC)

> the ratio of NUMA nodes to L3 cache instances.
> 
> When SNC mode is detected, reconfigure the RMID counters by updating
> the MSR_RMID_SNC_CONFIG MSR on each socket as CPUs are seen.

Can it be explained how RMID counters are reconfigured when
the MSR is updated?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/msr-index.h   |  1 +
>  arch/x86/kernel/cpu/resctrl/core.c | 68 ++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 1d111350197f..393d1b047617 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1100,6 +1100,7 @@
>  #define MSR_IA32_QM_CTR			0xc8e
>  #define MSR_IA32_PQR_ASSOC		0xc8f
>  #define MSR_IA32_L3_CBM_BASE		0xc90
> +#define MSR_RMID_SNC_CONFIG		0xca0
>  #define MSR_IA32_L2_CBM_BASE		0xd10
>  #define MSR_IA32_MBA_THRTL_BASE		0xd50
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index ed4f55b3e5e4..9f0ac9721fab 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -16,11 +16,14 @@
>  
>  #define pr_fmt(fmt)	"resctrl: " fmt
>  
> +#include <linux/cpu.h>
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/cacheinfo.h>
>  #include <linux/cpuhotplug.h>
> +#include <linux/mod_devicetable.h>
>  
> +#include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
>  #include <asm/resctrl.h>
>  #include "internal.h"
> @@ -724,11 +727,34 @@ static void clear_closid_rmid(int cpu)
>  	wrmsr(MSR_IA32_PQR_ASSOC, 0, 0);
>  }
>  
> +/*
> + * The power-on reset value of MSR_RMID_SNC_CONFIG is 0x1
> + * which indicates that RMIDs are configured in legacy mode.
> + * Clearing bit 0 reconfigures the RMID counters for use
> + * in Sub NUMA Cluster mode.
> + */

I think I missed where "legacy mode" and "Sub NUMA Cluster mode"
are explained. 

> +static void snc_remap_rmids(int cpu)
> +{
> +	u64 val;
> +
> +	/* Only need to enable once per package */

Sentence end with period.

> +	if (cpumask_first(topology_core_cpumask(cpu)) != cpu)
> +		return;
> +

This is an area I am not familiar with. The above code seems
to assume that CPUs are onlined in a particular numerical
order. For example, if I understand correctly, if CPUs
are onlined from higher number to lower number then
the above code may end up running on every CPU online.

> +	rdmsrl(MSR_RMID_SNC_CONFIG, val);
> +	val &= ~BIT_ULL(0);
> +	wrmsrl(MSR_RMID_SNC_CONFIG, val);
> +}
> +
>  static int resctrl_online_cpu(unsigned int cpu)
>  {
>  	struct rdt_resource *r;
>  
>  	mutex_lock(&rdtgroup_mutex);
> +
> +	if (snc_nodes_per_l3_cache > 1)
> +		snc_remap_rmids(cpu);
> +
>  	for_each_capable_rdt_resource(r)
>  		domain_add_cpu(cpu, r);
>  	/* The cpu is set in default rdtgroup after online. */
> @@ -983,11 +1009,53 @@ static __init bool get_rdt_resources(void)
>  	return (rdt_mon_capable || rdt_alloc_capable);
>  }
>  
> +/* CPU models that support MSR_RMID_SNC_CONFIG */
> +static const struct x86_cpu_id snc_cpu_ids[] __initconst = {
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, 0),
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, 0),
> +	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, 0),
> +	{}
> +};
> +
> +static __init int get_snc_config(void)

Could this function please get a comment about what it does?
The first paragraph from the commit message should be ok.

> +{
> +	unsigned long *node_caches;
> +	int mem_only_nodes = 0;
> +	int cpu, node, ret;
> +
> +	if (!x86_match_cpu(snc_cpu_ids))
> +		return 1;
> +
> +	node_caches = kcalloc(BITS_TO_LONGS(nr_node_ids), sizeof(*node_caches), GFP_KERNEL);

How about bitmap_zalloc()?

> +	if (!node_caches)
> +		return 1;
> +
> +	cpus_read_lock();
> +	for_each_node(node) {
> +		cpu = cpumask_first(cpumask_of_node(node));
> +		if (cpu < nr_cpu_ids)
> +			set_bit(get_cpu_cacheinfo_id(cpu, 3), node_caches);
> +		else
> +			mem_only_nodes++;
> +	}
> +	cpus_read_unlock();
> +
> +	ret = (nr_node_ids - mem_only_nodes) / bitmap_weight(node_caches, nr_node_ids);

My static checker complains about the above line risking
a "divide by zero" that may be the case if the bitmap_weight() returns
zero. You may need to ensure this is safe here before more static checkers
start analyzing this code.

> +	kfree(node_caches);
> +
> +	if (ret > 1)
> +		rdt_resources_all[RDT_RESOURCE_L3].r_resctrl.mon_scope = RESCTRL_NODE;
> +
> +	return ret;
> +}

As discussed there are scenarios where the above may not provide the
correct value. Could you please document the assumptions of this code to
highlight this to x86 maintainers for their opinions?

> +
>  static __init void rdt_init_res_defs_intel(void)
>  {
>  	struct rdt_hw_resource *hw_res;
>  	struct rdt_resource *r;
>  
> +	snc_nodes_per_l3_cache = get_snc_config();
> +
>  	for_each_rdt_resource(r) {
>  		hw_res = resctrl_to_arch_res(r);
>  


Reinette

On Mon, Sep 25, 2023 at 04:29:44PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 8/29/2023 4:44 PM, Tony Luck wrote:
> > There isn't a simple h/w bit that indicates whether a CPU is
> > running in Sub NUMA Cluster mode. Infer the state by comparing
> 
> Sub NUMA Cluster (SNC)

Fixed.

> 
> > the ratio of NUMA nodes to L3 cache instances.
> > 
> > When SNC mode is detected, reconfigure the RMID counters by updating
> > the MSR_RMID_SNC_CONFIG MSR on each socket as CPUs are seen.
> 
> Can it be explained how RMID counters are reconfigured when
> the MSR is updated?

Added some in previous patch (which implements the changes needed
by the reconfuration). Small note in ths patch to refer to that.

> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  arch/x86/include/asm/msr-index.h   |  1 +
> >  arch/x86/kernel/cpu/resctrl/core.c | 68 ++++++++++++++++++++++++++++++
> >  2 files changed, 69 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index 1d111350197f..393d1b047617 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -1100,6 +1100,7 @@
> >  #define MSR_IA32_QM_CTR			0xc8e
> >  #define MSR_IA32_PQR_ASSOC		0xc8f
> >  #define MSR_IA32_L3_CBM_BASE		0xc90
> > +#define MSR_RMID_SNC_CONFIG		0xca0
> >  #define MSR_IA32_L2_CBM_BASE		0xd10
> >  #define MSR_IA32_MBA_THRTL_BASE		0xd50
> >  
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index ed4f55b3e5e4..9f0ac9721fab 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -16,11 +16,14 @@
> >  
> >  #define pr_fmt(fmt)	"resctrl: " fmt
> >  
> > +#include <linux/cpu.h>
> >  #include <linux/slab.h>
> >  #include <linux/err.h>
> >  #include <linux/cacheinfo.h>
> >  #include <linux/cpuhotplug.h>
> > +#include <linux/mod_devicetable.h>
> >  
> > +#include <asm/cpu_device_id.h>
> >  #include <asm/intel-family.h>
> >  #include <asm/resctrl.h>
> >  #include "internal.h"
> > @@ -724,11 +727,34 @@ static void clear_closid_rmid(int cpu)
> >  	wrmsr(MSR_IA32_PQR_ASSOC, 0, 0);
> >  }
> >  
> > +/*
> > + * The power-on reset value of MSR_RMID_SNC_CONFIG is 0x1
> > + * which indicates that RMIDs are configured in legacy mode.
> > + * Clearing bit 0 reconfigures the RMID counters for use
> > + * in Sub NUMA Cluster mode.
> > + */
> 
> I think I missed where "legacy mode" and "Sub NUMA Cluster mode"
> are explained. 

Updated this comment to cover this better.

> 
> > +static void snc_remap_rmids(int cpu)
> > +{
> > +	u64 val;
> > +
> > +	/* Only need to enable once per package */
> 
> Sentence end with period.

Period added.
> 
> > +	if (cpumask_first(topology_core_cpumask(cpu)) != cpu)
> > +		return;
> > +
> 
> This is an area I am not familiar with. The above code seems
> to assume that CPUs are onlined in a particular numerical
> order. For example, if I understand correctly, if CPUs
> are onlined from higher number to lower number then
> the above code may end up running on every CPU online.

This sent me on a voyage of exploration into early Linux
bringup. There's a CONFIG_HOTPLUG_PARALLEL option to bring
CPUs up in parallel. I have it set on my kernel, but I still
see the "announce_cpu()" console messages show up in monotonic
increasing order:

[    3.148423] smpboot: x86: Booting SMP configuration:
[    3.148940] .... node  #0, CPUs:          #1   #2   #3   #4   #5   #6 #7 and so on

But, without solving this mystery, I realized this doesn't matter.
Whatever order the CPUs come online was all completed long before
resctrl is initialized:

	late_initcall(resctrl_late_init);

So the order that resctrl sees CPUs is dependent on the callbacks
from the registration with cpuhp_setup_state(). That happens with:

        /*
         * Try to call the startup callback for each present cpu
         * depending on the hotplug state of the cpu.
         */
        for_each_present_cpu(cpu) {

which is going to call in increasing numerical order as the bitmap
of present CPUs is traversed.

If someone changed this, the only ill effect on the code I'm
adding would be to set the MSR multiple times (which is
inefficient, but won't break anything).


> 
> > +	rdmsrl(MSR_RMID_SNC_CONFIG, val);
> > +	val &= ~BIT_ULL(0);
> > +	wrmsrl(MSR_RMID_SNC_CONFIG, val);
> > +}
> > +
> >  static int resctrl_online_cpu(unsigned int cpu)
> >  {
> >  	struct rdt_resource *r;
> >  
> >  	mutex_lock(&rdtgroup_mutex);
> > +
> > +	if (snc_nodes_per_l3_cache > 1)
> > +		snc_remap_rmids(cpu);
> > +
> >  	for_each_capable_rdt_resource(r)
> >  		domain_add_cpu(cpu, r);
> >  	/* The cpu is set in default rdtgroup after online. */
> > @@ -983,11 +1009,53 @@ static __init bool get_rdt_resources(void)
> >  	return (rdt_mon_capable || rdt_alloc_capable);
> >  }
> >  
> > +/* CPU models that support MSR_RMID_SNC_CONFIG */
> > +static const struct x86_cpu_id snc_cpu_ids[] __initconst = {
> > +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, 0),
> > +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, 0),
> > +	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, 0),
> > +	{}
> > +};
> > +
> > +static __init int get_snc_config(void)
> 
> Could this function please get a comment about what it does?
> The first paragraph from the commit message should be ok.

Comment added. Also renamed to snc_get_config() so that these
SNC related functions use the same naming convention.
> 
> > +{
> > +	unsigned long *node_caches;
> > +	int mem_only_nodes = 0;
> > +	int cpu, node, ret;
> > +
> > +	if (!x86_match_cpu(snc_cpu_ids))
> > +		return 1;
> > +
> > +	node_caches = kcalloc(BITS_TO_LONGS(nr_node_ids), sizeof(*node_caches), GFP_KERNEL);
> 
> How about bitmap_zalloc()?

Excellent! Now using it.

> 
> > +	if (!node_caches)
> > +		return 1;
> > +
> > +	cpus_read_lock();
> > +	for_each_node(node) {
> > +		cpu = cpumask_first(cpumask_of_node(node));
> > +		if (cpu < nr_cpu_ids)
> > +			set_bit(get_cpu_cacheinfo_id(cpu, 3), node_caches);
> > +		else
> > +			mem_only_nodes++;
> > +	}
> > +	cpus_read_unlock();
> > +
> > +	ret = (nr_node_ids - mem_only_nodes) / bitmap_weight(node_caches, nr_node_ids);
> 
> My static checker complains about the above line risking
> a "divide by zero" that may be the case if the bitmap_weight() returns
> zero. You may need to ensure this is safe here before more static checkers
> start analyzing this code.

Added defensive code that checks result of bitmap_weight() for a zero
return. If it does (somehow ... that means there are no nodes with
CPUs that have an L3 cache!) then the code will assume SNC is not
enabled.

> 
> > +	kfree(node_caches);
> > +
> > +	if (ret > 1)
> > +		rdt_resources_all[RDT_RESOURCE_L3].r_resctrl.mon_scope = RESCTRL_NODE;
> > +
> > +	return ret;
> > +}
> 
> As discussed there are scenarios where the above may not provide the
> correct value. Could you please document the assumptions of this code to
> highlight this to x86 maintainers for their opinions?

Added a note that booting with maxcpus=N boot parameter will break this
check in ways that cannot be worked around. Early boot code doesn't have
to worry about user taking CPUs offline, because it runs before users
can do that.

> > +
> >  static __init void rdt_init_res_defs_intel(void)
> >  {
> >  	struct rdt_hw_resource *hw_res;
> >  	struct rdt_resource *r;
> >  
> > +	snc_nodes_per_l3_cache = get_snc_config();
> > +
> >  	for_each_rdt_resource(r) {
> >  		hw_res = resctrl_to_arch_res(r);
> >  
> 
> 
> Reinette

Hi Tony,

On 9/28/2023 11:12 AM, Tony Luck wrote:
> On Mon, Sep 25, 2023 at 04:29:44PM -0700, Reinette Chatre wrote:
>> On 8/29/2023 4:44 PM, Tony Luck wrote:

>>> +	if (cpumask_first(topology_core_cpumask(cpu)) != cpu)
>>> +		return;
>>> +
>>
>> This is an area I am not familiar with. The above code seems
>> to assume that CPUs are onlined in a particular numerical
>> order. For example, if I understand correctly, if CPUs
>> are onlined from higher number to lower number then
>> the above code may end up running on every CPU online.
> 
> This sent me on a voyage of exploration into early Linux
> bringup. There's a CONFIG_HOTPLUG_PARALLEL option to bring
> CPUs up in parallel. I have it set on my kernel, but I still
> see the "announce_cpu()" console messages show up in monotonic
> increasing order:
> 
> [    3.148423] smpboot: x86: Booting SMP configuration:
> [    3.148940] .... node  #0, CPUs:          #1   #2   #3   #4   #5   #6 #7 and so on
> 
> But, without solving this mystery, I realized this doesn't matter.
> Whatever order the CPUs come online was all completed long before
> resctrl is initialized:
> 
> 	late_initcall(resctrl_late_init);
> 
> So the order that resctrl sees CPUs is dependent on the callbacks
> from the registration with cpuhp_setup_state(). That happens with:
> 
>         /*
>          * Try to call the startup callback for each present cpu
>          * depending on the hotplug state of the cpu.
>          */
>         for_each_present_cpu(cpu) {
> 
> which is going to call in increasing numerical order as the bitmap
> of present CPUs is traversed.
> 
> If someone changed this, the only ill effect on the code I'm
> adding would be to set the MSR multiple times (which is
> inefficient, but won't break anything).
> 
> 

Thank you very much for investigating this.


Reinette