Received-SPF: pass (google.com: domain of linux-kernel+bounces-118252-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 26 Mar 2024 09:14:16 +0800
From: Feng Tang <feng.tang@intel.com>
To: Waiman Long <longman@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@intel.com>, "H .
 Peter Anvin" <hpa@zytor.com>, Peter Zijlstra <peterz@infradead.org>,
	<x86@kernel.org>, <paulmck@kernel.org>, <rui.zhang@intel.com>,
	<linux-kernel@vger.kernel.org>, Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v2] x86/tsc: Use topology_max_packages() to get package
 number
Message-ID: <ZgIhaPzPNDXO7LMK@feng-clx.sh.intel.com>
References: <20240325030928.4190408-1-feng.tang@intel.com>
 <ae9eafa0-3c6a-4130-9890-a7463c8286a9@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ae9eafa0-3c6a-4130-9890-a7463c8286a9@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ji35BKZtV5FbwvjBSFoDxN9OWCeJ4l+SLHMx//MiaA/RhvF0jGqXBqF+opzm?=
 =?us-ascii?Q?1EROdhl25P4Oe8ZQZFygya+uQqPWPPgwz8EonIrQSrGnHUmR9IB8XbfdbHIE?=
 =?us-ascii?Q?vm3enhGyRy11c5gyabrmrq3dkCOLxrjJE5qjjz1f/kORACQVrgQHcM9lgj22?=
 =?us-ascii?Q?aB4ES2wpr9eJi7Ygyqz8sDzvvukFJvvr6dtlliM6d7WgyvDNHaGxKR0wj6BY?=
 =?us-ascii?Q?YbKIjHI/RblQhU7WiJLyf6SVc+6DVVygo7hkCU7NuVXHlk94mp58FL3zyeGf?=
 =?us-ascii?Q?70zvasONeLK4Dse7EH+zpDBjlLv6zy90jh7RRKk387WhmORKp6pOUvT/mWFB?=
 =?us-ascii?Q?lYObHKS5i0cODzRxWpyC1uLqbzpygOfsosNpSV903YLMNA0V+HMycd3JOJEt?=
 =?us-ascii?Q?sVZ6RqC7vfZUYl+Rv1Dz6RASkFnP/SR3H/6H4HYDG9I9JefGTLK7sTmAL/3Y?=
 =?us-ascii?Q?Vfw0JrS8SeNpAGPD1zZjUnZQ+hdvqtccpo+SLyZkzWduRHmyArl2tRkhjheA?=
 =?us-ascii?Q?ET6tqk68SZa8ZZkveyIXykDq48FSt0dpKG/9ao1OVg1KjGatsFzqbVmUgxvc?=
 =?us-ascii?Q?BpcHP58NX35Bbt5LzU80ISgpBKo+vsQ7d+Y0dIfesp1fxnUsyTLUi3qNnsY3?=
 =?us-ascii?Q?d09U9V4g/2jKIw5nMyFB/i1l8CuzrHzPYHXLmV4k9JKEp8h2G+sOXdXQueYF?=
 =?us-ascii?Q?R2oXQlNEPI+m84HU4Kfr+3pQTG50ThixwCrQScP9HFwEYzGFBZ4unfAhOPPA?=
 =?us-ascii?Q?0zsOXbupB1EKnRTgJpFJfSi3vRLaktEZy9/vQ5yb3sLq3Mb/I3UV82EOa4Ph?=
 =?us-ascii?Q?Q98VT6EmsIZ5IovVq0iwD+HxtWZ272LwBSIHnqCDTvXtIYgNV/sCxqdM7A+R?=
 =?us-ascii?Q?DrQx7zKbBsMSOlrZeQ6MpuVzfvfYpQGbPdLGUiuTAERt1OfIbOJigq9jZLMf?=
 =?us-ascii?Q?+8+g6RXPsmKZlVVxlbZrJnFKAGzKh9UpwH2XZTToLUfoOS4oCTH2+c0gqORH?=
 =?us-ascii?Q?LrwwHxZEz4AufCq/4ANDjmVAf0FXpQjjTScbwTVRtHk7RCPnvxwERlJbpIYD?=
 =?us-ascii?Q?E/CVTk1Hg3rv1hUk6In1a9Mm82PXGC6skcAcUUwFAbzRqpoqD0EyCGCGt5kr?=
 =?us-ascii?Q?PpRRjmmrPSUhvHb9D7SLpQhWQlD5ddmrGJFOn34sF8vGdNqK17wPoRS3NlAt?=
 =?us-ascii?Q?0kNaERewHHy/XNaaASg8ghMlzwEF7lPyAc5gD9oHk3OgHQTXP6xOC2iqtoND?=
 =?us-ascii?Q?XVtPtrWEkbBH8PTWvgBXY8ug7hBI9D7LupzUAyglUIJcPrHYWn2BBkN/k1i7?=
 =?us-ascii?Q?AxrfQ9Phzrr9OAKkOIw8NjfmENqqHft6dY5vdXieQjpA1YG6nQnARzpRb2GJ?=
 =?us-ascii?Q?uCN73AofZNtxT19KXz4ol7QChF3+PsLgwaMCrNjVR5q4gMeI1VJhjswZyjak?=
 =?us-ascii?Q?rPRZVkZ/TcYUSczTNAVuNx6aCerm7IdsG/rmIxtlWCvlUVJytzPLZPzyRKTC?=
 =?us-ascii?Q?II3ATAuHPNXalb1vpAj3mGVLr9QpsoflYav8RNfDBwtKSshUOs2x1FZR0iCf?=
 =?us-ascii?Q?bfM1+bBkwzQ55plAL0jwrQlsStluJ2akX+Se6fIn?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 57a5a402-f1b2-4bd3-c59b-08dc4d33ffef
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Mar 2024 01:28:11.5475
 (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: SidnDB1CEmFcxFQXTGIQ1YaDbkSsn4OWBXh/Csx0A+ETW/ZygPGEvcZXXIw3zQryXHjZSR0qxG6Z5bFVHPodug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6555
X-OriginatorOrg: intel.com

On Mon, Mar 25, 2024 at 09:17:38PM -0400, Waiman Long wrote:
> On 3/24/24 23:09, Feng Tang wrote:
> > Commit b50db7095fe0 ("x86/tsc: Disable clocksource watchdog for TSC
> > on qualified platorms") was introduced to solve problem that
> > sometimes TSC clocksource is wrongly judged as unstable by watchdog
> > like 'jiffies', HPET, etc.
> > 
> > In it, the hardware package number is a key factor for judging whether
> > to disable the watchdog for TSC, and 'nr_online_nodes' was chosen due
> > to it is available in early boot phase before registering 'tsc-early'
> > clocksource, where all non-boot CPUs are not brought up yet.
> > 
> > Dave and Rui pointed out there are many cases in which 'nr_online_nodes'
> > is cheated and not accurate, like:
> > 
> > * numa emulation (numa=fake=8 etc.)
> > * numa=off
> > * platforms with CPU-less HBM nodes, CPU-less Optane memory nodes.
> > * SNC (sub-numa cluster) mode enabled
> > * 'maxcpus=' cmdline setup, where chopped CPUs could be onlined later
> > * 'nr_cpus=', 'possible_cpus=' cmdline setup, where chopped CPUs can
> >    not be onlined after boot
> > 
> > Thomas' recent patchset of refactoring x86 topology code improves
> > topology_max_packages(), by making it more accurate and available in
> > early boot phase, which works well in most of the above cases.
> > 
> > The only exceptions are 'nr_cpus=' and 'possible_cpus=' setup.  And
> > the reason is, during topology setup, 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_packages() 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_packages().  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.
> > 
> > So use topology_max_packages() to replace nr_online_nodes().
> > 
> > Reported-by: Dave Hansen <dave.hansen@linux.intel.com>
> > Closes: https://lore.kernel.org/lkml/a4860054-0f16-6513-f121-501048431086@intel.com/
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> > 
> > Hi all,
> > 
> > For warning about possible compromise due to 'nr_cpus=' and 'possible_cpus=',
> > another alternative could be checking whether these has been setup in cmdline
> > inside tsc.c and warn there.
> > 
> > Changelog:
> > 
> >    Since v1:
> > 
> >    * Use Dave's detailed elaboration about 'nr_cpus=', 'possible_cpus='
> >      possibly compromising '__max_logical_packages' in commit log
> >    * Fix typos and inaccuracy pointed out by Rui and Longman
> > 
> >   arch/x86/kernel/cpu/topology.c | 5 ++++-
> >   arch/x86/kernel/tsc.c          | 7 ++-----
> >   2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
> > index 3259b1d4fefe..2db03b00e29b 100644
> > --- 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_warn("TSC might be buggered due to the rejected CPUs\n");
> > +	}
> 
> People may sometimes use kernel option like "panic_on_warn=1" to cause a
> crash dump on warning. Maybe we should just use pr_info() as the presence of
> rejected CPUs are unlikely to be a real problem from the TSC stability point
> of view. To emphasize it a bit more, we could add a "WARNING: " prefix, for
> example.

Good catch! will chagne it to pr_info. The possible hurt of these
debug options are relatively small. In the past several years, I
haven't got a real case that TSC is really broken. 

Thanks,
Feng

> 
> Cheers,
> Longman
>