Received-SPF: pass (google.com: domain of linux-kernel+bounces-176230-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <570e0c1d-8df8-45c6-a9a8-972e1ff2cf66@intel.com>
Date: Fri, 10 May 2024 14:24:49 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v17 8/9] x86/resctrl: Sub NUMA Cluster detection and
 enable
To: Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
	"Maciej Wieczor-Retman" <maciej.wieczor-retman@intel.com>, Peter Newman
	<peternewman@google.com>, James Morse <james.morse@arm.com>, Babu Moger
	<babu.moger@amd.com>, Drew Fustini <dfustini@baylibre.com>, Dave Martin
	<Dave.Martin@arm.com>
CC: <x86@kernel.org>, <linux-kernel@vger.kernel.org>,
	<patches@lists.linux.dev>
References: <20240503203325.21512-1-tony.luck@intel.com>
 <20240503203325.21512-9-tony.luck@intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240503203325.21512-9-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SzNBZDc2UnNKMFMrN3orUHhzSCtZemhDenExWVoySkYydnBuZ3ZGVHhSa1NG?=
 =?utf-8?B?WFE3UlBRMXlJdFpjMUV0a2xwaFVIMG4veGZSOGYvcE1kWVpaRTZ3U1EyVi9W?=
 =?utf-8?B?ajJ3MVlEc0lTdk15RW52dlBFKzMxWXJXTEx3OTBRci8xZDhVRmhwdXpRRHZB?=
 =?utf-8?B?SDF4OElkeTFHQ25RWklYSU14ZXVtUFRHREIyZVIrV0dteVFEUGgrZjlBSW9m?=
 =?utf-8?B?aE9lWnA0bTYxdHBMZ1djSldoZks2WTd4RDQ0MURnb1ZzM3QyQi9ESFRMbzZp?=
 =?utf-8?B?a09hYk92OUcrSVdnK3BheGVaMkNyVVZOL2Nva1lVWWZWeVZLeDBBMXpzS2xs?=
 =?utf-8?B?TWtKWjR0d1p5VUZVdjdkcTJBb3lwZVNGZFBNRVhKRjVBTE4rZStjUjFjMjZ3?=
 =?utf-8?B?L3FYVUY0T29VVkVoWXFrRFkxM0R2Q3lvNGExZnNTY3dEd3l3WjB5cFFLVFZy?=
 =?utf-8?B?VkZlNXJyTVN3QnlWdG9aMVFoUkZNYUdQR3RuRmVBVExhVGhRMHAwOGRWU0Zo?=
 =?utf-8?B?Nk5PT0tCUUFhK2FNUHcyUzJKTUJ1V2tackRtOUo1ZEZlSU4wL3pvNVZKc215?=
 =?utf-8?B?YkJWRE4xNFN6alAzeUFscUN1TFlVaGM1cDZDbUlOUUpQLzRrbHluV0RJRFNo?=
 =?utf-8?B?MDdwbXZqdDZHcUd1UUZDRGJLWGVINWxWQktsQS9FUUxtTmRmMXVNUFdRL1JQ?=
 =?utf-8?B?VmU0dndtWlY2T0pOZVArKzBUSWlnWjBrTEN2Z2RRNW85bHpDRTZZeER2eEcy?=
 =?utf-8?B?MXcyTlpTZXFoam90a05hc0p1VnQvOWVFaGxGWmF3MUlZNkZoL1QxQVZFRlhi?=
 =?utf-8?B?eUo3V1dTbTRiZ3hCWUZmckF1cU1qRDZxaW1Icm9ZS0ZhbXY0YTlYWkQ5V3Bz?=
 =?utf-8?B?MGFCZHJSOTJFNERRaWJZWTgwVHdQYkVwcDNvcVJ3b0F6V2l0UEZUU3BKYzh1?=
 =?utf-8?B?ZGFNTmJjNVBMdC80dHFnZGxwbnNLR0d3czZoUkxnZ3lOUWlNYWEvYVNFNEs3?=
 =?utf-8?B?elVvUjJMRWxnclNOUDRqc2RBRFZocjBTZ0RiTHdRQmlkaTdpMjFVVWMrdnZU?=
 =?utf-8?B?dERNTWFYZFdaSGdPdm85MHVVQjhYM2JRMlY2Z0FUVEt6Yi9pcENIa0NhaDRY?=
 =?utf-8?B?TEhtZVJTMjYrWjIrYnlmUG5HL2FmbTU4Tno0NnhNNFJoaUlsZVYyNFZ5LzRG?=
 =?utf-8?B?SWZMQkxPVkdWRFlKUGlXdTIwNUJLUnBwQVZMTjFRdmNJdFl3NEt1dndnN2hm?=
 =?utf-8?B?VThRNnZyUVU5cXR4YkppRDNyVmlxam9hMGl0c05nR04vcUtReGpLK2FTN2x2?=
 =?utf-8?B?KzJ3d0RDdUJzWWlpZDJiRnl0dEFHZTNPQlpvUWRFeStEOU5mRWdCbk1WL3Uy?=
 =?utf-8?B?NVNhNVU5Zml6MXJZUFVjaEhEWmRjL0dQc0pLYXFrRkZGMFBTWS9ibjNxMGF1?=
 =?utf-8?B?dDZjM0xyVlR6eFhYd256VHJ0QitMMHQ2cDY1VVJ6U0VudzFoNENGUmJTMWp4?=
 =?utf-8?B?QlJJQnplSmFOUjZEMDZzTVNiV05KNzlZRnp6OHJrVUhtdDVJaldUVXA1SHFl?=
 =?utf-8?B?VTlvVVhpMExLMnNVSUdhdDJCSFJkNGRZMmM2QW5JZ29sYlFlZ0liSHpoNG0w?=
 =?utf-8?B?V3lGYmlmV0kxR2JEdTB4QjhYbFIwMC9RbmowT3V2UlFsdEtGV3RYM2c1bU9r?=
 =?utf-8?B?Z1prTEU5ZUY1SFFzVFFKckpzcGxhSHAwOFFSSytDbEc4VDdOT1hvNkhDR2ly?=
 =?utf-8?B?cGJEYUJlOFgwSzJJNkFWUHZONVVGUDlxVG5MaE5UVEJBem5lcmhCZ1ZkbUow?=
 =?utf-8?B?bi84bmx6WHdNUzNDaG9PNUVJVFM3ZkhVQjdYNFQ1WDNpT2JKUkx4TVN3Tjhh?=
 =?utf-8?B?WkpUVlV1SG1MOXEvUFdKNTh3OVVDQUIzRVF2V3lYRWVERFpjVllRU01WVGN5?=
 =?utf-8?B?Q2kvK0I5dXlwaVF1ZjN5YUpZT05ZeDZRTFR3SUhxZzE3QSttZTA5YVg4aXRC?=
 =?utf-8?B?YlNZQ1d3cWpLYk51M01NV2lvRlNHanVnMkJZNGIvNjlKMElVQzJQL2R2U1dT?=
 =?utf-8?B?RkdldS92Z0pTRDdkSzcwbm1tZzlKcG12QjNaU0Q1QWt6T2F3TDVjYnZFSjU3?=
 =?utf-8?B?YzV1MWNQajI4Wm5WWHludXpaNFpWY05WMERPbWFkajhQNjcrYzgxWFdFWGx1?=
 =?utf-8?B?a2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 320f6a83-4925-4486-f688-08dc7137a118
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2024 21:24:52.0702
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8R2o1JmGkm8Mn9Qnux3q5nuopVy6GnIZ+kUgy5JJ3HSAjXFvCFsuQBpfuv8lZmOU7HnNx0qMd9UyykxwCmqD1AS4HmMIIK3aYps3qc2KyVg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6914
X-OriginatorOrg: intel.com

Hi Tony,

On 5/3/2024 1:33 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
> 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.
> 
> Clearing bit zero of the MSR divides the RMIDs and renumbers the ones
> on the second SNC node to start from zero.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/msr-index.h   |   1 +
>  arch/x86/kernel/cpu/resctrl/core.c | 119 +++++++++++++++++++++++++++++
>  2 files changed, 120 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e72c2b872957..ce54a1ffe1e5 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1165,6 +1165,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 a949e69308cd..6a1727ea1dfe 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -21,7 +21,9 @@
>  #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"
> @@ -746,11 +748,42 @@ static void clear_closid_rmid(int cpu)
>  	      RESCTRL_RESERVED_CLOSID);
>  }
>  
> +/*
> + * The power-on reset value of MSR_RMID_SNC_CONFIG is 0x1
> + * which indicates that RMIDs are configured in legacy mode.
> + * This mode is incompatible with Linux resctrl semantics
> + * as RMIDs are partitioned between SNC nodes, which requires
> + * a user to know which RMID is allocated to a task.
> + * Clearing bit 0 reconfigures the RMID counters for use
> + * in Sub NUMA Cluster mode. This mode is better for Linux.
> + * The RMID space is divided between all SNC nodes with the
> + * RMIDs renumbered to start from zero in each node when
> + * couning operations from tasks. Code to read the counters
> + * must adjust RMID counter numbers based on SNC node. See
> + * __rmid_read() for code that does this.
> + */
> +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_arch_online_cpu(unsigned int cpu)
>  {
>  	struct rdt_resource *r;
>  
>  	mutex_lock(&domain_list_lock);
> +
> +	if (snc_nodes_per_l3_cache > 1)
> +		snc_remap_rmids(cpu);
> +
>  	for_each_capable_rdt_resource(r)
>  		domain_add_cpu(cpu, r);
>  	mutex_unlock(&domain_list_lock);
> @@ -990,11 +1023,97 @@ 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),
> +	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X, 0),
> +	X86_MATCH_INTEL_FAM6_MODEL(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
> + * ratio of NUMA nodes to L3 cache instances.
> + * 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)
> +{
> +	unsigned long *node_caches;
> +	int mem_only_nodes = 0;
> +	int cpu, node, ret;
> +	int num_l3_caches;
> +	int cache_id;
> +
> +	if (!x86_match_cpu(snc_cpu_ids))
> +		return 1;
> +
> +	node_caches = bitmap_zalloc(num_possible_cpus(), GFP_KERNEL);
> +	if (!node_caches)
> +		return 1;
> +
> +	cpus_read_lock();
> +
> +	if (num_online_cpus() != num_present_cpus())
> +		pr_warn("Some CPUs offline, SNC detection may be incorrect\n");
> +
> +	for_each_node(node) {
> +		cpu = cpumask_first(cpumask_of_node(node));
> +		if (cpu < nr_cpu_ids) {
> +			cache_id = get_cpu_cacheinfo_id(cpu, 3);
> +			if (cache_id != -1)
> +				set_bit(cache_id, node_caches);
> +		} else {
> +			mem_only_nodes++;
> +		}
> +	}
> +	cpus_read_unlock();
> +
> +	num_l3_caches = bitmap_weight(node_caches, num_possible_cpus());
> +	kfree(node_caches);
> +
> +	if (!num_l3_caches)
> +		goto insane;
> +
> +	/* sanity check #1: Number of CPU nodes must be multiple of num_l3_caches */
> +	if ((nr_node_ids - mem_only_nodes) % num_l3_caches)
> +		goto insane;
> +
> +	ret = (nr_node_ids - mem_only_nodes) / num_l3_caches;
> +
> +	/* sanity check #2: Only valid results are 1, 2, 3, 4 */
> +	switch (ret) {
> +	case 1:
> +		break;
> +	case 2:
> +	case 3:
> +	case 4:
> +		pr_info("Sub-NUMA cluster detected with %d nodes per L3 cache\n", ret);
> +		rdt_resources_all[RDT_RESOURCE_L3].r_resctrl.mon_scope = RESCTRL_NODE;
> +		break;
> +	default:
> +		goto insane;
> +	}
> +
> +	return ret;
> +insane:
> +	pr_warn("SNC insanity: CPU nodes = %d num_l3_caches = %d\n",
> +		(nr_node_ids - mem_only_nodes), num_l3_caches);
> +	return 1;
> +}

I find it confusing how dramatically this SNC detection code changed without
any explanations. This detection seems to match the SNC detection code from v16 but
after v16 you posted a new SNC detection implementation that did SNC detection totally
differently [1] from v16. Instead of keeping with the "new" detection this implements
what was in v16. Could you please help me understand what motivated the different
implementations and why the big differences?

Reinette


[1] https://lore.kernel.org/lkml/20240327200352.236835-11-tony.luck@intel.com/