Received-SPF: pass (google.com: domain of linux-kernel+bounces-106918-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 19 Mar 2024 10:11:06 +0800
From: Feng Tang <feng.tang@intel.com>
To: Dave Hansen <dave.hansen@intel.com>
CC: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>, "Peter
 Zijlstra" <peterz@infradead.org>, <x86@kernel.org>, <paulmck@kernel.org>,
	<rui.zhang@intel.com>, Waiman Long <longman@redhat.com>,
	<linux-kernel@vger.kernel.org>, Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH] x86/tsc: Use topology_max_packages() to get package
 number
Message-ID: <Zfj0OvSG8Ox9x8Tj@feng-clx.sh.intel.com>
References: <20240315112606.2248284-1-feng.tang@intel.com>
 <a2f38451-5c20-43f2-842e-dd4c2c038e1f@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <a2f38451-5c20-43f2-842e-dd4c2c038e1f@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?szLilWyR2SgMZSvwtKaveSw1sD2J38D0k9u5Ml+LNM9n8tw0P+BE6SlxGSdC?=
 =?us-ascii?Q?Y1wKbnbGnNSKrdTXfT5LuBIo+v9UcZL/3lsJSqQ0+SY6f9rHEGrUW4z2YZdc?=
 =?us-ascii?Q?mWavPtGtLuRHgiNkFTtvTbKPCxzzkS4sv7G7SQeQxkMhLr0vX55gQbEs5sIb?=
 =?us-ascii?Q?09NXWynBfAmkokR7n+KB2j0eHe44wQthb7i+PEARKjFVylsPIiZfwfrruyq5?=
 =?us-ascii?Q?utp0l6PcIHvoxEkIy50GPSfUkzRx0J315VmwPuu/PNPHApbXKiQw5nYSltDJ?=
 =?us-ascii?Q?zZaRzE1EMzggKerKwfUHAGSzX9a6LmO22+kQk9KNlkw1E3Rg7Z9UAs5KL0ee?=
 =?us-ascii?Q?OivLRieSGUIsxAKkU3AUiNDdib/2oRkfo6n2D25lq8QmZfkBv3vXKUTh8W8p?=
 =?us-ascii?Q?HASrOd47ANORCamNTP7OwvzqL7mcn/bxps0Lj3VEGyepdybofZ81YtMJzL8d?=
 =?us-ascii?Q?zeooMWxIeqf7Y3BbH820MggPT5W2GJbQeQJdRFMXexdzIDy6kDmafIOfmOCo?=
 =?us-ascii?Q?bDcMwIypOq3QA5RkOAmCzm/HzAckEJr8FOJHV5p0kzYcPVyLJjpCLjGIhgWy?=
 =?us-ascii?Q?HfYaTvpY2WutkWyFqTzeyb+rcgidYs3gd459Aey4qNJx1436xzIREJ5MpltW?=
 =?us-ascii?Q?D+z3+rsZ9FAs5IRwDJd/RTrqZzh7s33Xnx1KyJxtuVNkjuo7+cyLoaUqrj0i?=
 =?us-ascii?Q?Mw8ZwoiJrd19P8QVyZRICbH7K6eZzN6qS/aR3DHtCqJdsj2Zp96YDkVDgK/S?=
 =?us-ascii?Q?rsB/VWlartEU6b4DLcHYO7pG9jE39tkC623EP1jYtT51G8uHbnjM2M9XGSiq?=
 =?us-ascii?Q?Zs6CvRtY3oguKVTo/+pge016a18FU/cd1IZKp48cakl6Q8vTO28H0/lK7mE/?=
 =?us-ascii?Q?Jca15FkXpPmiWi3meqLCT/bHCdYhVunXGeg3vTZvwf8gsBUvz5Jo5FIxSL9w?=
 =?us-ascii?Q?Ps5jY1OLcsdrPp4FV1P7V71x+0xPhTuGIlCg693KZgIkpEnQ3aVbVfbuo9cZ?=
 =?us-ascii?Q?f0psqe2fnxaC9Jx5HTIFVAA77IkG6A1uCB0DAZ1xLdHHinR5hygpGHkdg9gq?=
 =?us-ascii?Q?oDehwmhUQBelFQKcqLBSwtnlGPEhhuviUJHSwu6dr9HQQaU9d94YXp33PjZQ?=
 =?us-ascii?Q?IFsZSsMPF/mX+0lbC2aQlyTHshOB74Pa09c5mjW0Dc+UT3+KiShNRiCAIlS+?=
 =?us-ascii?Q?MzZQlBa2ntDXP+4paRHv02PyoaZdMDPYrEt9OwgLj746cIjHSTwt7VCTEM2p?=
 =?us-ascii?Q?jN3jT2W2ZgtENm1Lw735Xo1U9KxHzjTtA3wLjCFD2tPK2zx+6JpE9Ug1Jj0E?=
 =?us-ascii?Q?lt83HZCUaYC1LI2JmgCWlyAS3hmis+/Krc0c8IqXfBLzk4zKS648ohezlOvm?=
 =?us-ascii?Q?0MnkmjXrw7WUYoOOYC0JcPpUYQKnh93/F1HLtVO+hzbxh8gLLb+J3tfeG2+N?=
 =?us-ascii?Q?roEppiMgrdQsrpgmjYWfkzykA2cOGocuzoVlev1lj/fqHrxT3tpOGJwzsPSu?=
 =?us-ascii?Q?7U7vhCElIhL9rPYVjR2L4Z4GpbZFEHTFrbkwYr7P2EA1zSBk/j1zRgEXJE0J?=
 =?us-ascii?Q?TUkTbAIcIkLMPXdDdFuu4DtIt25yuRXi8cjE+gZk?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e147af3d-bbce-45a5-2c81-08dc47bbc8f4
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Mar 2024 02:25:03.7061
 (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: fdfydA5IIv4p9x3LLSxYbTDC2Gdrro808LhaocFeaTjrS5HndInVp0EAUUtMcaJlwUcMTCGWLFDa6TkAoYOTGw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6140
X-OriginatorOrg: intel.com

On Mon, Mar 18, 2024 at 09:35:57AM -0700, Dave Hansen wrote:
> On 3/15/24 04:26, Feng Tang wrote:
> > Thomas' recent patchset of refactoring x86 topology code introduces
> > topology_max_package(), which works well in most of the above cases.
> > The only exceptions are 'nr_cpus=' and 'possible_cpus=' setup, which
> > sets up the 'nr_cpu_ids' and rejects the rest of the CPUs, and may
> > cause topology_max_package() less than the real package number, but
> > it's fine as it is rarely used debug option, and logical package
> > number really matters in this check. So use the more accurate
> > topology_max_package() to replace nr_online_nodes().
> 
> This is a lot longer than what you have in this changelog, but I hope it
> makes sense.  This is retreading some of what Thomas's set does, but I
> think it's worth re-explaining:
> 
> The boot CPU iterates through all enumerated APIC ids and either accepts
> or rejects the APIC id.  For accepted ids, it figures out which bits of
> the id map to the package number.  It tracks which package numbers have
> been seen in a bitmap.  topology_max_package() just returns the number
> of bits set in that bitmap.
> 
> 'nr_cpus=' and 'possible_cpus=' can cause more APIC ids to be rejected
> and can artificially lower the number of bits in the package bitmap and
> thus topology_max_package().
> 
> This means that, for example, a system with 8 physical packages might
> reject all the CPUs on 6 of those packages and be left with only 2
> packages and 2 bits set in the package bitmap.  It needs the TSC
> watchdog, but would disable it anyway.  This isn't ideal, but this only
> happens for debug-oriented options.
> 
> This is fixable by tracking the package numbers for rejected CPUs.  But
> it's not worth the trouble for debugging.

Will steal it for the v2 :). Many thanks!

> 
> --
> 
> The only thing that comes to mind is if we need something simple like:
> 
> 	if (topo_info.nr_rejected_cpus)
> 		pr_info("TSC might be buggered\n");
> 
> ... somewhere.

I think logically this should be together with the package checking
code in tsc.c, and we need to export the info from static data
'topo_info'. The other thought is adding in topology code:

--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -460,8 +460,11 @@ void __init topology_init_possible_cpus(void)
 	pr_info("Num. threads per package: %3u\n", __num_threads_per_package);
 
 	pr_info("Allowing %u present CPUs plus %u hotplug CPUs\n", assigned, disabled);
-	if (topo_info.nr_rejected_cpus)
+	if (topo_info.nr_rejected_cpus) {
 		pr_info("Rejected CPUs %u\n", topo_info.nr_rejected_cpus);
+		if (__max_logical_packages <= 4)
+			pr_warning("TSC might be bugged due to these rejected CPUs\n");
+	}

Thanks,
Feng