Received-SPF: pass (google.com: domain of linux-kernel+bounces-42092-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Jan 2024 10:17:56 +0800
From: Feng Tang <feng.tang@intel.com>
To: Waiman Long <longman@redhat.com>
CC: John Stultz <jstultz@google.com>, Thomas Gleixner <tglx@linutronix.de>,
	Stephen Boyd <sboyd@kernel.org>, <linux-kernel@vger.kernel.org>,
	<paulmck@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Jin Wang
	<jin1.wang@intel.com>
Subject: Re: [PATCH v2] clocksource: Scale the max retry number of watchdog
 read according to CPU numbers
Message-ID: <ZbcK1Ixk2bjnYxdf@feng-clx.sh.intel.com>
References: <20240128143143.3021280-1-feng.tang@intel.com>
 <9ca4d7de-d022-4a5b-b114-3ffe5f4b4bd1@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <9ca4d7de-d022-4a5b-b114-3ffe5f4b4bd1@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ilNEfrcoB9/RPHigBb5FhVcIOpYa1MCfNnEpBs0GoUKEPJEjKtUkoUKMXHMh?=
 =?us-ascii?Q?embiBDjjWtV3xo3TgeDISLMl0YRp2konLBWGZid8pLKJ1Q0KfcZvR3ZG8Yxk?=
 =?us-ascii?Q?cwr4xrSyZDiOPxglG2RHl9uSfZctgzZMD02BX9dQWRu/SzBKQYX/PqAz5MsN?=
 =?us-ascii?Q?jtFJCmMaOymPf+7jYPKQg9UT0EGa8tpAz1riDgVz+q0PpSa7F2i3YdjywadX?=
 =?us-ascii?Q?KzD8kq8JjmFEsqz0zU1Pok/WFfhPnZpxFFXMLdl5KH8uFB/kzHbK+WHOrk/W?=
 =?us-ascii?Q?EMS55d+8aXMI7/croGz/FmM3PlVQN7riCP1Jg2DLDxScJdRkCwRIvP9HL6Cf?=
 =?us-ascii?Q?2PkOJcXirbNbtNdGRP71HaSkmUuK4fXZ0YtjslXcOBs0fLly8Oog6nzgpT7k?=
 =?us-ascii?Q?E3UHZtMgIPuPJtIIaV+3ysswn1x1QOy94ljUeE8FDAHYavf0/dGJyUrHe2FG?=
 =?us-ascii?Q?uzLNeKYjYGbfXIUlaRNEGaoWSwatRhUr1bDLUCvTxnKO5K1S9wl/egtWlb8t?=
 =?us-ascii?Q?7CtKDr3aM2Kis+oKJKVFBrwvEFyqu7b7cdmn24qznQz2JylHh+3ZQP9WrD0Y?=
 =?us-ascii?Q?y+5mfJqpmdp1setr4atICTnKLDFQAwXU3aB3ocXCpDwFMiCiC/4+dOTQKaCd?=
 =?us-ascii?Q?TxNQdTaqYVUYEGRWTI3jTE8qZ9XhdFFv4SH1GHD257ynINiZEPl3YKx+VMpv?=
 =?us-ascii?Q?8OIy51bHWS0I/aIokQZt9KzpY1yj08QzAm16MhFf+GYgeOJLnUnpcKHbmbh1?=
 =?us-ascii?Q?0zcF2QNlANiKWXQPrDtuHKd/oyLya8640dBkkv0GVFCXhPwYRK4R0SNGZm3a?=
 =?us-ascii?Q?K/JM8ZrJTUveOiwXR6Vgz5pT9VZdhV9rwJU5WSVtVJ1D70OaYPEaqv/zS+yF?=
 =?us-ascii?Q?3wxkVH1LokQE9snXcGHM5/hCgtN/tNOspIMGh2Rji81yjbQmZFKb92VIMhfE?=
 =?us-ascii?Q?Ky2xZq/ygc/IUprotT7lFRxg2fO51gHw7HWIiWpLvwc0Kf6FbSS2XqD7kvF4?=
 =?us-ascii?Q?xIL7uO7srnDvotzj1IVWa4cBdysaXgyNT0CAxR/U9d2ftba+6X+1SbW9shmt?=
 =?us-ascii?Q?LrbSpEsKHOoXeO2yKzF/AXwmjGtyfsjXWTntSvLT62m0dxsdaCFdLJPdFEJx?=
 =?us-ascii?Q?VvCdiu5oqs79H5U6ZOZJ1xnSIhWV76dbodI6VdjFYv5cVput0uyYydONdQur?=
 =?us-ascii?Q?b8IE+MGqKFrteW81KSPJ2VS9ZS9ClISWTfJX2i1sjvBhV16pyLEFk7YIOgfn?=
 =?us-ascii?Q?h357n1kUvqeb1Rv/CBI2drSEtbI35mc+jqs2ALqRCVXKPllft12dd4QfBXMu?=
 =?us-ascii?Q?ZWukPjXwa0r76aRyHdOpeFJehfBFlyAW97P37MTJtV+tjBhvjBZAP6zExVPY?=
 =?us-ascii?Q?Ai5MmQ8JHrV8U78TixUbqdrQFQJk2yWg05hroX36sADuB5jXlR2KOIMcuoDD?=
 =?us-ascii?Q?Yp+K7T2sOWgEN6AW9eOw48xQZxn/46mzQQRZvqA6hnX2gubLe0HsoDQveOac?=
 =?us-ascii?Q?L3UkCGPPe7sCOIOdDy4cUxq3dNTvIDNVdKG6rTjuCzHIONNGzpKN1NGxk47d?=
 =?us-ascii?Q?gCIPGppXC70EmcIC2pPU1WkPH+ow8dHLApmLM8XP?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e2fc07c5-ee71-49f2-92b9-08dc2071b5b5
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 02:26:33.5377
 (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: rwA6HdvjpU4R/GKorUjzwbDUmqPo/oTJfMhbRqYSIt74943qcw8dIqdMx7FzLb/clTVahItOTQrOHTAirgv39g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6119
X-OriginatorOrg: intel.com

On Sun, Jan 28, 2024 at 01:15:31PM -0500, Waiman Long wrote:
> 
> On 1/28/24 09:31, 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 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 on large system, which has been verified fine in
> > 4 days and 670 rounds of reboot test on the 8-socket machine.
> > 
> > Also add sanity check for user input value for 'max_cswd_read_retries', and
> > provide a general helper for getting this max retry number as suggested by
> > Paul and Waiman.
> > 
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Waiman Long <longman@redhat.com>
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > Tested-by: Jin Wang <jin1.wang@intel.com>
> > ---
> > Changelog:
> > 
> >      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)
> > 
> >   include/linux/clocksource.h      |  2 +-
> >   kernel/time/clocksource-wdtest.c | 12 +++++++-----
> >   kernel/time/clocksource.c        | 26 +++++++++++++++++++++-----
> >   3 files changed, 29 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> > index 1d42d4b17327..8d84d2f79622 100644
> > --- a/include/linux/clocksource.h
> > +++ b/include/linux/clocksource.h
> > @@ -291,7 +291,7 @@ 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;
> > +long clocksource_max_watchdog_read_retries(void);
> >   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..c70cea3c44a1 100644
> > --- a/kernel/time/clocksource-wdtest.c
> > +++ b/kernel/time/clocksource-wdtest.c
> > @@ -106,6 +106,7 @@ static int wdtest_func(void *arg)
> >   	unsigned long j1, j2;
> >   	char *s;
> >   	int i;
> > +	long max_retries;
> >   	schedule_timeout_uninterruptible(holdoff * HZ);
> > @@ -139,18 +140,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 = clocksource_max_watchdog_read_retries();
> > +	for (i = 0; i <= max_retries + 1; i++) {
> > +		if (i <= 1 && i < max_retries)
> >   			s = "";
> > -		else if (i <= max_cswd_read_retries)
> > +		else if (i <= max_retries)
> >   			s = ", expect message";
> >   		else
> >   			s = ", expect clock skew";
> > -		pr_info("--- Watchdog with %dx error injection, %lu retries%s.\n", i, max_cswd_read_retries, s);
> > +		pr_info("--- Watchdog with %dx error injection, %ld retries%s.\n", i, max_retries, s);
> >   		WRITE_ONCE(wdtest_ktime_read_ndelays, i);
> >   		schedule_timeout_uninterruptible(2 * HZ);
> >   		WARN_ON_ONCE(READ_ONCE(wdtest_ktime_read_ndelays));
> > -		WARN_ON_ONCE((i <= max_cswd_read_retries) !=
> > +		WARN_ON_ONCE((i <= max_retries) !=
> >   			     !(clocksource_wdtest_ktime.flags & CLOCK_SOURCE_UNSTABLE));
> >   		wdtest_ktime_clocksource_reset();
> >   	}
> > diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> > index c108ed8a9804..00e359c80822 100644
> > --- a/kernel/time/clocksource.c
> > +++ b/kernel/time/clocksource.c
> > @@ -208,9 +208,8 @@ void clocksource_mark_unstable(struct clocksource *cs)
> >   	spin_unlock_irqrestore(&watchdog_lock, flags);
> >   }
> > -ulong max_cswd_read_retries = 2;
> > -module_param(max_cswd_read_retries, ulong, 0644);
> > -EXPORT_SYMBOL_GPL(max_cswd_read_retries);
> > +static long max_cswd_read_retries = -1;
> > +module_param(max_cswd_read_retries, long, 0644);
> >   static int verify_n_cpus = 8;
> >   module_param(verify_n_cpus, int, 0644);
> > @@ -220,13 +219,30 @@ enum wd_read_status {
> >   	WD_READ_SKIP
> >   };
> > +long clocksource_max_watchdog_read_retries(void)
> > +{
> > +	long max_retries = max_cswd_read_retries;
> > +
> > +	if (max_cswd_read_retries <= 0) {
> > +		/* santity check for user input value */
> > +		if (max_cswd_read_retries != -1)
> > +			pr_warn_once("max_cswd_read_retries has been set a invalid number: %ld\n",
> > +				max_cswd_read_retries);
> > +
> > +		max_retries = ilog2(num_online_cpus()) + 1;
> > +	}
> > +	return max_retries;
> > +}
> 
> Should you add "EXPORT_SYMBOL_GPL(clocksource_max_watchdog_read_retries)"
> since clocksource-wdtest.c is likely to be in a kernel module?

Yes, you are right. I tested the wdtest as built-in and missed this.

> Alternatively, you can make clocksource_max_watchdog_read_retries() an
> inline function in a header file and keep the max_cswd_read_retries export.

Yep, will go this way and restore exporting 'max_cswd_read_retries'. Thanks!

- Feng

> 
> Cheers,
> Longman
>