Re: [PATCH v19 19/20] x86/resctrl: Sub-NUMA Cluster (SNC) detection and enabling

[Reinette Chatre]

Hi Tony,

should "and enabling" be dropped from shortlog?

On 5/28/24 3:20 PM, Tony Luck wrote:
> There isn't a simple hardware bit that indicates whether a CPU is
> running in Sub-NUMA Cluster (SNC) mode. Infer the state by comparing
> number CPUs sharing the L3 cache with CPU0 to the number of CPUs in
> the same NUMA node as CPU0.
>
> If SNC mode is detected, print a single informational message to the
> console.
>
> Add the missing definition of pr_fmt() to monitor.c. This wasn't
> noticed before as there are only "can't happen" console messages
> from this file.
>
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 59 +++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index c7559735e33a..1c5162a68461 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -15,6 +15,8 @@
>    * Software Developer Manual June 2016, volume 3, section 17.17.
>    */
>   
> +#define pr_fmt(fmt)	"resctrl: " fmt
> +
>   #include <linux/cpu.h>
>   #include <linux/module.h>
>   #include <linux/sizes.h>
> @@ -1095,6 +1097,61 @@ void arch_mon_domain_online(struct rdt_resource *r, struct rdt_mon_domain *d)
>   	wrmsrl(MSR_RMID_SNC_CONFIG, val);
>   }
>   
> +/* CPU models that support MSR_RMID_SNC_CONFIG */
> +static const struct x86_cpu_id snc_cpu_ids[] __initconst = {
> +	X86_MATCH_VFM(INTEL_ICELAKE_X, 0),
> +	X86_MATCH_VFM(INTEL_SAPPHIRERAPIDS_X, 0),
> +	X86_MATCH_VFM(INTEL_EMERALDRAPIDS_X, 0),
> +	X86_MATCH_VFM(INTEL_GRANITERAPIDS_X, 0),
> +	X86_MATCH_VFM(INTEL_ATOM_CRESTMONT_X, 0),
> +	{}
> +};
> +
> +/*
> + * There isn't a simple hardware bit that indicates whether a CPU is running
> + * in Sub-NUMA Cluster (SNC) mode. Infer the state by comparing the
> + * number CPUs sharing the L3 cache with CPU0 to the number of CPUs in
> + * the same NUMA node as CPU0.
> + * It is not possible to accurately determine SNC state if the system is
> + * booted with a maxcpus=N parameter. That distorts the ratio of SNC nodes
> + * to L3 caches. It will be OK if system is booted with hyperthreading
> + * disabled (since this doesn't affect the ratio).
> + */
> +static __init int snc_get_config(void)
> +{
> +	struct cacheinfo *ci = get_cpu_cacheinfo_level(0, RESCTRL_L3_CACHE);
> +	const cpumask_t	*node0_cpumask;

Stray tab

> +	int ret;
> +
> +	if (!x86_match_cpu(snc_cpu_ids) || !ci)
> +		return 1;
> +
> +	cpus_read_lock();
> +	if (num_online_cpus() != num_present_cpus())
> +		pr_warn("Some CPUs offline, SNC detection may be incorrect\n");
> +	cpus_read_unlock();
> +
> +	node0_cpumask = cpumask_of_node(cpu_to_node(0));
> +
> +	ret = cpumask_weight(&ci->shared_cpu_map) / cpumask_weight(node0_cpumask);

It is not obvious to the static checker I tried that cpumask_weight(node0_cpumask)
cannot be zero. Can you please insert a check to make static checkers happy?

> +
> +	/* sanity check: Only valid results are 1, 2, 3, 4 */
> +	switch (ret) {
> +	case 1:
> +		break;
> +	case 2 ... 4:
> +		pr_info("Sub-NUMA Cluster mode detected with %d nodes per L3 cache\n", ret);
> +		rdt_resources_all[RDT_RESOURCE_L3].r_resctrl.mon_scope = RESCTRL_L3_NODE;
> +		break;
> +	default:
> +		pr_warn("Ignore improbable SNC node count %d\n", ret);
> +		ret = 1;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>   int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>   {
>   	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
> @@ -1102,6 +1159,8 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>   	unsigned int threshold;
>   	int ret;
>   
> +	snc_nodes_per_l3_cache = snc_get_config();
> +
>   	resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
>   	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;

Reinette