Received-SPF: pass (google.com: domain of linux-kernel+bounces-195887-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: <c6651270-25a3-4d9e-91f8-4aa9c8652b98@intel.com>
Date: Thu, 30 May 2024 13:28:47 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 19/20] x86/resctrl: Sub-NUMA Cluster (SNC) detection
 and enabling
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: <20240528222006.58283-1-tony.luck@intel.com>
 <20240528222006.58283-20-tony.luck@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <20240528222006.58283-20-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
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?S2gvTVEwV1BLZEJ2OENTc1FUdWFjS2FuUEdZRzJxSDNUSDdLaFV0alRreFJl?=
 =?utf-8?B?Y1RyNHJvUVVqbG9hVUQ5b1dPUzkwSXBJZGlCYyt3dnZsTE1FbTJoUE0zVUxw?=
 =?utf-8?B?UmMyUmpaRWVRVG1oTy9vZytMN2ZjRnNmaFdUNU5LZDN4MDEwOTA3K0NtbHln?=
 =?utf-8?B?VXROcDNIQmsxUE5VRmZLbVd2NURtVC9qWDBZSG9tSnM5aTNJczRKRzlBMUg0?=
 =?utf-8?B?bXV6SkhDU2NWaXFPN0FKdFdiK3F1allaUmhvTWVyNGRTTElTbGV6MUtKOXc3?=
 =?utf-8?B?VHdsSkxTaXVzR0Q3M25nQVliZG9QTWswVHB6M0F4Q2Rvb251cmZ3SlRRUkpP?=
 =?utf-8?B?YXhhK1pVVWtyVytrM1Zna1k3Snh5bEFLVTE5YXF0QVhEWlAxM2lIQUdwZS9K?=
 =?utf-8?B?UXVuWFZPZUI2ejNkNnMyTjFUdHdINVdpeDJVeURxRDk1UjNHMzZMVE5PQlB3?=
 =?utf-8?B?ZlVkalpyalhsN2RMT0N1bVFIMWtzdUl6Q21xRkQ1M1lLcDZLNmREMzl6bzFr?=
 =?utf-8?B?TmZtQ2d1U25PeFNXNDg0VDZGNHJkazA2UnUyVTFDTTIvZ1lmaU5kcURydkdn?=
 =?utf-8?B?R0VkNVE4SUY4cmFrbVQ5THNFMWRCazY0QXNJSEFBc3pwQ3hZb0RrNUlYM1li?=
 =?utf-8?B?U09RY3lIM2taUFQ5dXpPK1VxVk1hc2JGd0ttT1ZSL1FkNUxheG0rREF5NFY0?=
 =?utf-8?B?TXNkcGNWN3pFaXJORTUvRkJqNGdsVDBEZzZvcVFzZ2FpRUJ5c0tEcGtGSDBI?=
 =?utf-8?B?NWZOMGhwMzRhNlRTcmtabWxnYzhFUDRNTTIveTFEdVo4RjZ4TVI1Wi9aMThm?=
 =?utf-8?B?Z05VYTdTVldSYVozMStZYTE4R3ZPTWVqOENSblpvNERZczZGR2w0SWFrdTBG?=
 =?utf-8?B?K0I3bFZRUzFXekFOdHB0TDFNa25icEJGbE1ZWWo0Nm9LNHZGRktlTno2d1Q0?=
 =?utf-8?B?OW5jSys3REEyemFEaTlGbE13UzdrZ1U1a2ZzL3hwTnAwbm5GeHhKVEZPaERJ?=
 =?utf-8?B?R1JXY1RTMG9jNjYzb0ZpcDBlR1FRUTRXdkJnUWIvQklReGtzQzQwSGQ5UG8x?=
 =?utf-8?B?SUVwN1ZVbC85SUpGVWJHRkZJQXc3L0FZN0RoN0ZhTmRObmJLZjRXcFdSb2w2?=
 =?utf-8?B?L0kyL3R2NzQyc0ZtUmJtMSt3by9DRnRCc3d3L0hXVU1xc2JHaXN6Y0lxME1i?=
 =?utf-8?B?ZTdLZlhtRWc0SWpxakZ1WnBtb2lSZThvM1dYYUNhYytYYXZQdkdjd0xKdlYx?=
 =?utf-8?B?bUkrYkROQ05rVzg2dWNiT0dOT0xwOVN0dDFiUEtQQjluRU1XbVlNaExyVkli?=
 =?utf-8?B?K3E5VkFtajZhb1dIOUdQRUtDd3JBVUFIK0tSQmdadWZ4TG5vT2l3MzdiSmJh?=
 =?utf-8?B?UWhQRGFOb1d5OXQvcU1nK09KTzhiZklOanh0TUF5d3hyWlBiMnFTMGI4d1hR?=
 =?utf-8?B?Vzd3YlR3bGRuSnlrSkVJVzFEcmJUME9pRDVsM0dvaVc2MFFuTEF5V0pDeHcx?=
 =?utf-8?B?TE84V0hWM3h5dzhYT0NZWFZzRVVDSERqUWNoc3Q3UWRRZHpOb29hZnFRR3NI?=
 =?utf-8?B?Y2x1ZXdVTnZiQnQ3ZjlYOXVhUENyajcwaFBwYVJMZ1NsRm15dFFSMXRsYjcx?=
 =?utf-8?B?NUZGaWZJK1NzKzcxVkM1Y1F2cGVZZGlSaGZOa29OSUJNZk5kekdFcVpGZ21w?=
 =?utf-8?B?M2tFSEN4SVdDMG9COTQ3YjBkbHZyZjZVOFJuQVZqRFBmWlpLeG5nVGFNbWlV?=
 =?utf-8?B?Uy9NazhEZEhDbWVxQWw5aUNnMkliYnRNZDlvOWxkQVNXYW5HZFpkYkhjU09N?=
 =?utf-8?B?djlIcUZEL3ZLOER4VDdDSUFwaU5ieUpSMVZUenNvSk56b1NtUnZUb0JPbnp2?=
 =?utf-8?B?SWNKMDVRZDg3ZGEvcG9CbFQrM0xnbmlXSWpkNitXRXY2aFYzbXlYbUxUait0?=
 =?utf-8?B?ZU5SakFnbkRoS3UvZENiWWx5a3RRL05XSThoVS9qL3doeWRMLzBYQTlaY2px?=
 =?utf-8?B?THE0RkFYZFNIQWZVblNBVmNSZkFXQnYzVzB5WHJMY0czV2NMTUNBbmpjRFlE?=
 =?utf-8?B?bG9UMjh3QjQrQWV2c3FQT0tGcTJLYXE0dTM4eVVENlNXclBQV0p2RGRMcHlo?=
 =?utf-8?B?VEVyZitJSmh0cHl6dmtCYWxlZjNsK0l1dTRxblhxWEhFUTNYUVZHeDJmL0V3?=
 =?utf-8?B?MFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ff40c397-7b20-4a5a-915c-08dc80e71d03
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 20:28:49.3517
 (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: IgjRpUa2JRecfSMwUMG36hQXBbqbTh4ZbEiviU2qlxuTEwuOxQAQ9Lm7fF2qCCNS0Isd5ySoGy4PxNJr1MF2xJvfpqB01eyrSQiDAdzQzng=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4581
X-OriginatorOrg: intel.com

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@intel.com>
> ---
>   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