Received-SPF: pass (google.com: domain of linux-kernel+bounces-73970-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 21 Feb 2024 10:27:19 +0800
From: Feng Tang <feng.tang@intel.com>
To: "Paul E. McKenney" <paulmck@kernel.org>
CC: John Stultz <jstultz@google.com>, Thomas Gleixner <tglx@linutronix.de>,
	Stephen Boyd <sboyd@kernel.org>, Jonathan Corbet <corbet@lwn.net>, "Peter
 Zijlstra" <peterz@infradead.org>, Waiman Long <longman@redhat.com>,
	<linux-kernel@vger.kernel.org>, Jin Wang <jin1.wang@intel.com>
Subject: Re: [PATCH v4] clocksource: Scale the max retry number of watchdog
 read according to CPU numbers
Message-ID: <ZdVfh1yeDEplFPzR@feng-clx.sh.intel.com>
References: <20240220154302.848412-1-feng.tang@intel.com>
 <2996a3b1-33db-4a58-aa69-fd13cdbb1eee@paulmck-laptop>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <2996a3b1-33db-4a58-aa69-fd13cdbb1eee@paulmck-laptop>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?/e6kH1qeNKPGKY78QRAMMxpCIMpCdVzGMnFKBela8YLyu/D65DgMtskKmNJ8?=
 =?us-ascii?Q?JIOqQfGTj6YTMfcTDtuL6BrM3H21AASoVvj2zlbZZVUrjfyV4PFA1qv/Uwcd?=
 =?us-ascii?Q?L0RMn433X2CcIQLj+AXl+8+b0WcY7I/Wx/BmuSI8G52uyVkJo2142AK/tucP?=
 =?us-ascii?Q?P9gw3pviWJ5t+oWdA80FWwFuvLmzJHNhCacyhINFCpbTx/m+OgeeIQKB8Red?=
 =?us-ascii?Q?93PxWO03PcJGxqCmyV7PGkffh9ZSUBWW3oPxAPNn1nCxMDmhSAeDMCjrNHib?=
 =?us-ascii?Q?mR0YJQd6Vc0bpXfzgvhfc1JgrRd5sk/deM+gFtL67TSPz5wRGh3gOqOsrr6L?=
 =?us-ascii?Q?1y67nsx7/WjQYn1qFo0eOe9m7EExmHLb47lUFnAlfOufh96yiKfGehxrC18n?=
 =?us-ascii?Q?B2JrTLOuRzMAszca5iivbwmPikKWerKqLk3iLDqZNxTAYC8hKVeBIBTBzTPe?=
 =?us-ascii?Q?Gp1fbj8UYp6BR2jNMKS4UpqWXMkp3OyNK+HDllAY86CwHRZfVjtsZVyACCI9?=
 =?us-ascii?Q?lKkXHvuId5sTUpql19GOBwVjlOKSyPOt/F3KzvHXVDJwz3zoM7S4//xPUy0I?=
 =?us-ascii?Q?Hq4G3TJXzWEO5MIcmfMiNr/bPDWFAoQsZP8g3E/uG6fxCXKz1nF17+8123sp?=
 =?us-ascii?Q?Qo9W+p91gaBQyEhrkLDdZ7+Jsn4KqU4+jv6yndtdGL02lFGJh320iKwNlxzz?=
 =?us-ascii?Q?bMyMsJaw+rQsNhReWNTBRwNk/VgERxMxkaS9rdqKzOYuSQeerknWZpobg/Od?=
 =?us-ascii?Q?3PcotTk+jOI8FaYgxPtn8MQmPvL9eYys/Nend/bAQ8HtPHG7QMSm438odf7P?=
 =?us-ascii?Q?EIZ6tWbbErN2DC87cuNnv/hnUXeiJxClBA9ZEY03UtVwmMGi82pYRLEQNVku?=
 =?us-ascii?Q?GOzFKIIWklg1rR3aJfBc/7zdmmj2sRmlFrBREyVHJw4ezpmQKXJIQ5S3Kh/y?=
 =?us-ascii?Q?CAvfo7uVjhVWnxUU7OQgZ8NQJBgbC9TkjFiF3RzUTgJIzqp4C9+E6CW14pHV?=
 =?us-ascii?Q?YeEHF7O2pWpPQLTxO9XuMUexs8udA8hudVYCpp4ws9jFbzGVHRg36XrCc9aZ?=
 =?us-ascii?Q?q9QktbVBuNQ8hNXMPPpRBd5E7KiyL86g6LtzaPPTG0Wwc0ps5wdfMG8Y6b1m?=
 =?us-ascii?Q?brYJl73jnAUGH4o38k5rfdltMERR324qPOT9+r7E/luXYGny50MWbOOskbOf?=
 =?us-ascii?Q?aHxtOGlXINPxA93/A4u5iNofLw0ttjDC6ioGz+fkXkSUPnuCEJJSL5ymO79F?=
 =?us-ascii?Q?Ong1QUxUn+EAoJb9dNQm2OvHk9JfwD6bSpeuU8/IXqQFbgiYd7+7u3By43hC?=
 =?us-ascii?Q?pjcUGRoi81XpZbscQObQA7x+RIbhxqwuwFXINa09EwacJG23A/21LR5u0Lik?=
 =?us-ascii?Q?xRUF6XCdwhZTwYhehla6u46QbYqpBJVyDYGLOc7gxFhvt9LtVpxNh9X9zvFF?=
 =?us-ascii?Q?LduyVrn2YkoAVRaaiyI83sMR+AK5CMc2F+KntfYXhJbhHQt6UWk7fTc227SQ?=
 =?us-ascii?Q?rcSNmTsdee9ivMZOhD6XHe3YrXST1cqH5c4dmBELwrje5sPTPLtwNPVHF/kF?=
 =?us-ascii?Q?WpvHbAM7pe6hXJ3hmA9d0x9HFyJfV8V9+YXq/63V?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1ad4b69c-d1d2-4b27-8625-08dc32869980
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2024 02:41:26.5312
 (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: VlRfTd532CJZWY5F58fAM79jQAQ01M8yP3hSTBbOAcVnZt8bMrOg9oJTlXD/8FRFEECw5kWev51d2wALLGRE6A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8585
X-OriginatorOrg: intel.com

On Tue, Feb 20, 2024 at 09:47:11AM -0800, Paul E. McKenney wrote:
> On Tue, Feb 20, 2024 at 11:43:02PM +0800, Feng Tang wrote:
> > There was a bug on one 8-socket server that the TSC is wrongly marked
> > as 'unstable' and disabled during boot time (reproduce rate is about
> > every 120 rounds of reboot tests), with log:
> > 
> >     clocksource: timekeeping watchdog on CPU227: wd-tsc-wd excessive read-back delay of 153560ns vs. limit of 125000ns,
> >     wd-wd read-back delay only 11440ns, attempt 3, marking tsc unstable
> >     tsc: Marking TSC unstable due to clocksource watchdog
> >     TSC found unstable after boot, most likely due to broken BIOS. Use 'tsc=unstable'.
> >     sched_clock: Marking unstable (119294969739, 159204297)<-(125446229205, -5992055152)
> >     clocksource: Checking clocksource tsc synchronization from CPU 319 to CPUs 0,99,136,180,210,542,601,896.
> >     clocksource: Switched to clocksource hpet
> > 
> > The reason is for platform with lots of CPU, there are sporadic big or
> > huge read latency of read watchog/clocksource during boot or when system
> > is under stress work load, and the frequency and maximum value of the
> > latency goes up with the increasing of CPU numbers. Current code already
> > has logic to detect and filter such high latency case by reading 3 times
> > of watchdog, and check the 2 deltas. Due to the randomness of the
> > latency, there is a low possibility situation that the first delta
> > (latency) is big, but the second delta is small and looks valid, which
> > can escape from the check, and there is a 'max_cswd_read_retries' for
> > retrying that check covering this case, whose default value is only 2
> > and may be not enough for machines with huge number of CPUs.
> > 
> > So scale and enlarge the max retry number according to CPU number to
> > better filter those latency noise for large systems, which has been
> > verified fine in 4 days reboot test on the 8-socket machine.
> > 
> > Also as suggested by Thomas, remove parameter 'max_cswd_read_retries'
> > which was originally introduced to cover this.
> > 
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > Tested-by: Jin Wang <jin1.wang@intel.com>
> > Tested-by: Paul E. McKenney <paulmck@kernel.org>
> > Reviewed-by: Waiman Long <longman@redhat.com>
> > ---
> >  
> > Hi Paul, Waiman,
> > 
> > I keep your 'Tested-by' and 'Reviewed-by' tag for v3, as I think the
> > core logic of the patch isn't changed. Please let me know if you
> > think otherwise. thanks!
> 
> I retested, and all went well, so please keep my Tested-by.
 
Thanks for the testing, again!

> One nit below...
> 
> 							Thanx, Paul
> 
> > Changelog:
> > 
> >     since v3:
> >       * Remove clocksource's module parameter 'max_cswd_read_retries' (Thomas)
> >       * Use "ilog4" instead of ilog2 for max retry calculation, and
> >         may be adjusted later (Paul)
> > 
> >     since v2:
> >       * Fix the unexported symbol of helper function being used by
> >         kernel module issue (Waiman)
> > 
> >     since v1:
> >       * Add santity check for user input value of 'max_cswd_read_retries'
> >         and a helper function for getting max retry nubmer (Paul)
> >       * Apply the same logic to watchdog test code (Waiman)
> > 
> > 
> >  Documentation/admin-guide/kernel-parameters.txt  |  6 ------
> >  include/linux/clocksource.h                      |  1 -
> >  kernel/time/clocksource-wdtest.c                 | 13 +++++++------
> >  kernel/time/clocksource.c                        | 16 +++++++++++-----
> >  .../testing/selftests/rcutorture/bin/torture.sh  |  2 +-
> >  5 files changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 31b3a25680d0..763e96dcf8b1 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -679,12 +679,6 @@
> >  			loops can be debugged more effectively on production
> >  			systems.
> >  
> > -	clocksource.max_cswd_read_retries= [KNL]
> > -			Number of clocksource_watchdog() retries due to
> > -			external delays before the clock will be marked
> > -			unstable.  Defaults to two retries, that is,
> > -			three attempts to read the clock under test.
> > -
> >  	clocksource.verify_n_cpus= [KNL]
> >  			Limit the number of CPUs checked for clocksources
> >  			marked with CLOCK_SOURCE_VERIFY_PERCPU that
> > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> > index 1d42d4b17327..b93f18270b5c 100644
> > --- a/include/linux/clocksource.h
> > +++ b/include/linux/clocksource.h
> > @@ -291,7 +291,6 @@ static inline void timer_probe(void) {}
> >  #define TIMER_ACPI_DECLARE(name, table_id, fn)		\
> >  	ACPI_DECLARE_PROBE_ENTRY(timer, name, table_id, 0, NULL, 0, fn)
> >  
> > -extern ulong max_cswd_read_retries;
> >  void clocksource_verify_percpu(struct clocksource *cs);
> >  
> >  #endif /* _LINUX_CLOCKSOURCE_H */
> > diff --git a/kernel/time/clocksource-wdtest.c b/kernel/time/clocksource-wdtest.c
> > index df922f49d171..d1025f956fab 100644
> > --- a/kernel/time/clocksource-wdtest.c
> > +++ b/kernel/time/clocksource-wdtest.c
> > @@ -105,7 +105,7 @@ static int wdtest_func(void *arg)
> >  {
> >  	unsigned long j1, j2;
> >  	char *s;
> > -	int i;
> > +	int i, max_retries;
> >  
> >  	schedule_timeout_uninterruptible(holdoff * HZ);
> >  
> > @@ -139,18 +139,19 @@ static int wdtest_func(void *arg)
> >  	WARN_ON_ONCE(time_before(j2, j1 + NSEC_PER_USEC));
> >  
> >  	/* Verify tsc-like stability with various numbers of errors injected. */
> > -	for (i = 0; i <= max_cswd_read_retries + 1; i++) {
> > -		if (i <= 1 && i < max_cswd_read_retries)
> > +	max_retries = ilog2(num_online_cpus()) / 2 + 1;
> 
> Please pull this into a function so that the two calculations of
> max_retries are automatically in synchronization with each other.

Will do, thanks.

- Feng