Received-SPF: pass (google.com: domain of linux-kernel+bounces-16272-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 4 Jan 2024 13:55:21 +0800
From: Feng Tang <feng.tang@intel.com>
To: Jiri Wiesner <jwiesner@suse.de>
CC: <linux-kernel@vger.kernel.org>, John Stultz <jstultz@google.com>, "Thomas
 Gleixner" <tglx@linutronix.de>, Stephen Boyd <sboyd@kernel.org>, "Paul E.
 McKenney" <paulmck@kernel.org>
Subject: Re: [PATCH] clocksource: Skip watchdog check for large watchdog
 intervals
Message-ID: <ZZZISRg9YGyT2eQ+@feng-clx>
References: <20240103112113.GA6108@incl>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240103112113.GA6108@incl>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tJVygJTlxwniY7SskWkNdA+OYfSo05ZWehgoXy6ytIFvLapVFn91uis1Lsbu?=
 =?us-ascii?Q?qUIRKvHqE9wfSK584WK12MAK93YnCVrr3ZGpvU9FHKJvXq0FTJzFvFhgKeq2?=
 =?us-ascii?Q?urzOj4qgty9TsZLi0AR4ky7mSbPEd1vbAClpFdK1ePY1uruwSZTwF1GrI9aM?=
 =?us-ascii?Q?uxKDKSCoS4yP/j2oLhxCwKqscA7ZPi7AnKYBl2DJ/i9nWt1llUjkpyCvsKQZ?=
 =?us-ascii?Q?vJ4mnvoMzZUnFxKgsXruI8TVOU946z7xo70trsU631Lmpbdflzr0iyuB227F?=
 =?us-ascii?Q?moLir9JUKuMvQUq0wfZcXJHiTdsJzL3c+PDJUythl0N1RHFc7UKINaCST//l?=
 =?us-ascii?Q?G6NnL0Udegzfre/WbIIyRE7amDTxEu5t4bht48rMOBk2r3lbf63p4Y9Y2jBp?=
 =?us-ascii?Q?CWt+mooooRdiKcgZJ8x3Hz95/1IiQoIxR82OLzQdajYHBnGNpB2t2ShNele3?=
 =?us-ascii?Q?iVpdeuFU+xIMpLW5oQZk9dOPN7eJC5d0TS9BZy0GnNe8Szppahe9gg+KKEuO?=
 =?us-ascii?Q?mGv8ar6Qa1grscRXwwkzUkYJgCa3V6AcJgbT+hDZW8TnbgWsT2WNQduri6Y/?=
 =?us-ascii?Q?9B8OxwjagjVQj68uY6yWEKUyc4xDoHi5UOi05t/2WvLfY+ZMQEJBBebv0kpn?=
 =?us-ascii?Q?U4lfVK0kghkflt0Tzvy+TpR4ExDvk2EnRHuEKEwpkacu9Gz3BeUKXTyzf73Z?=
 =?us-ascii?Q?llElqrzk6cFN0LBXsi2cHTvh/5GBk5HdCod11SRwAVsRZVzs6UWbvmPBsyb/?=
 =?us-ascii?Q?voj325ERl1Sjba87Z2+rjtxyzSr9CEs0MZA2yODm1xcd7ph4DLzPtriqDS0p?=
 =?us-ascii?Q?n2KaJmu2BVYq4Hh2QotJP8QGHaYjOAMJTUoIvwjnDo7Szdy/2dSLX8Xr/R7w?=
 =?us-ascii?Q?9wbTkkO/kVRrYqyaBU1WV3+lUMdfRf+sMT/7XMTknXCRW+qn/nAABQND6E1z?=
 =?us-ascii?Q?5NODi0L3cSzSSfipn0x2GDa+iF4Ewbh8/73NhHJ2tBOKoB2ipFZwzKh0K4pQ?=
 =?us-ascii?Q?zHztO7fBLTXfXaAiExRrSumnSwWHFAU/82sSML8j3i8Ea2ns2Gs8oNYvMU67?=
 =?us-ascii?Q?kXEAIsIjfQVY9Z4kr85Uh/2cJBI2FQjCxG0urDZPwZuw1jxpmwnBsP2U0xik?=
 =?us-ascii?Q?nmfkjpT+LfyUmHPx32mF46f902+O18WUg+G4JhLDKFGnTL0yxiOZq/VCzjif?=
 =?us-ascii?Q?ucfWIA2vThXN09T84HFJjIz0sFKgedCLAplGfzUpzVmCtqLj79Cv+lfhvxRx?=
 =?us-ascii?Q?eFDnCk2u9ejSDCs8SiiNUTQOXyBVdXruhbt5qfw4kYzLsgJynIsurw91jUEu?=
 =?us-ascii?Q?yfQvyzgo+jxzq1/T4rogzkYxVGcLcxXIjq/IE2ly8YVSeZ+IfwjZVGIJudIU?=
 =?us-ascii?Q?a9dOEPG8q3gCZUWI1d5py+z7HhEWEclsRedKO+p+hVxuUEAw1F7BzMzyFbRs?=
 =?us-ascii?Q?VVfrKxJQ3OkknGbX6o9ZQWUdEAIbdjEE52dq60ansABFeyhMST2Qbo5JAmyK?=
 =?us-ascii?Q?1lzeWH0yzD8GQFWof+3AUfm2mWr2KCPnqI/Kb5t9koEbxbyxycXbhNwxYrVM?=
 =?us-ascii?Q?4V3bomH/+AKod8wzH8ZN3roY4HFNN7Qva1oZCfRL?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b6c9c331-6f81-4dfb-b115-08dc0ceaf550
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2024 06:04:05.8778
 (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: WRZy9CVE5PSfKCuOCVHw1pxpYcEtKWg96w9uMaPuhndyPTO0eCOSMtkRqjOCHmDrE2h1kUojAALmsKrY/XeArw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB8192
X-OriginatorOrg: intel.com

On Wed, Jan 03, 2024 at 12:21:13PM +0100, Jiri Wiesner wrote:
> There have been reports of the watchdog marking clocksources unstable on
> machines with 8 NUMA nodes:
> > clocksource: timekeeping watchdog on CPU373: Marking clocksource 'tsc' as unstable because the skew is too large:
> > clocksource:   'hpet' wd_nsec: 14523447520 wd_now: 5a749706 wd_last: 45adf1e0 mask: ffffffff
> > clocksource:   'tsc' cs_nsec: 14524115132 cs_now: 515ce2c5a96caa cs_last: 515cd9a9d83918 mask: ffffffffffffffff
> > clocksource:   'tsc' is current clocksource.
> > 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 (1950347883333462, 79649632569)<-(1950428279338308, -745776594)
> > clocksource: Checking clocksource tsc synchronization from CPU 400 to CPUs 0,46,52,54,138,208,392,397.
> > clocksource: Switched to clocksource hpet
> 
> The measured clocksource skew - the absolute difference between cs_nsec
> and wd_nsec - was 668 microseconds:
> > cs_nsec - wd_nsec = 14524115132 - 14523447520 = 667612
> 
> The kernel (based on 5.14.21) used 200 microseconds for the
> uncertainty_margin of both the clocksource and watchdog, resulting in a
> threshold of 400 microseconds.  The discrepancy is that the measured
> clocksource skew was evaluated against a threshold suited for watchdog
> intervals of roughly WATCHDOG_INTERVAL, i.e. HZ >> 1, which is 0.5 second.
> Both the cs_nsec and the wd_nsec value indicate that the actual watchdog
> interval was circa 14.5 seconds. Since the watchdog is executed in softirq
> context the expiration of the watchdog timer can get severely delayed on
> account of a ksoftirqd thread not getting to run in a timely manner.
> Surely, a system with such belated softirq execution is not working well
> and the scheduling issue should be looked into but the clocksource
> watchdog should be able to deal with it accordingly.
> 
> The solution in this patch skips the current watchdog check if the
> watchdog interval exceeds 1.5 * WATCHDOG_INTERVAL. Considering the maximum
> watchdog interval of 1.5 * WATCHDOG_INTERVAL, the current default
> uncertainty margin (of the TSC and HPET) corresponds to a limit on
> clocksource skew of 333 ppm (microseconds of skew per second). To keep the
> limit imposed by NTP (500 microseconds of skew per second) for all
> possible watchdog intervals, the margins would have to be scaled so that
> the threshold value is proportional to the length of the actual watchdog
> interval.
> 
> Fixes: 2e27e793e280 ("clocksource: Reduce clocksource-skew threshold")

IIUC, the issue could happen even before this commit? Though I do think 
this is good stuff for stable kernel.

> Suggested-by: Feng Tang <feng.tang@intel.com>
> Signed-off-by: Jiri Wiesner <jwiesner@suse.de>

0Day robot reported some compiling issue. Other than that, it looks
good to me, thanks!

Reviewed-by: Feng Tang <feng.tang@intel.com>

>
> ---
>  kernel/time/clocksource.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index c108ed8a9804..ac5cb0ff278b 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -98,7 +98,9 @@ static u64 suspend_start;
>  /*
>   * Interval: 0.5sec.
>   */
> -#define WATCHDOG_INTERVAL (HZ >> 1)
> +#define WATCHDOG_INTERVAL	(HZ >> 1)
> +#define WATCHDOG_INTR_MAX_NS	((WATCHDOG_INTERVAL + (WATCHDOG_INTERVAL >> 1))\
> +				 * NSEC_PER_SEC / HZ)
>  
>  /*
>   * Threshold: 0.0312s, when doubled: 0.0625s.
> @@ -134,6 +136,7 @@ static DECLARE_WORK(watchdog_work, clocksource_watchdog_work);
>  static DEFINE_SPINLOCK(watchdog_lock);
>  static int watchdog_running;
>  static atomic_t watchdog_reset_pending;
> +static int64_t watchdog_max_intr;
>  
>  static inline void clocksource_watchdog_lock(unsigned long *flags)
>  {
> @@ -400,7 +403,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  {
>  	u64 csnow, wdnow, cslast, wdlast, delta;
>  	int next_cpu, reset_pending;
> -	int64_t wd_nsec, cs_nsec;
> +	int64_t wd_nsec, cs_nsec, interval;
>  	struct clocksource *cs;
>  	enum wd_read_status read_ret;
>  	unsigned long extra_wait = 0;
> @@ -470,6 +473,27 @@ static void clocksource_watchdog(struct timer_list *unused)
>  		if (atomic_read(&watchdog_reset_pending))
>  			continue;
>  
> +		/*
> +		 * The processing of timer softirqs can get delayed (usually
> +		 * on account of ksoftirqd not getting to run in a timely
> +		 * manner), which causes the watchdog interval to stretch.
> +		 * Some clocksources, e.g. acpi_pm, cannot tolerate
> +		 * watchdog intervals longer than a few seconds.
> +		 * Skew detection may fail for longer watchdog intervals
> +		 * on account of fixed margins being used.
> +		 */
> +		interval = max(cs_nsec, wd_nsec);
> +		if (unlikely(interval > WATCHDOG_INTR_MAX_NS)) {
> +			if (system_state > SYSTEM_SCHEDULING &&
> +			    interval > 2 * watchdog_max_intr) {
> +				watchdog_max_intr = interval;
> +				pr_warn("Skipping watchdog check: cs_nsec: %lld wd_nsec: %lld\n",
> +					cs_nsec, wd_nsec);
> +			}
> +			watchdog_timer.expires = jiffies;
> +			continue;
> +		}
> +
>  		/* Check the deviation from the watchdog clocksource. */
>  		md = cs->uncertainty_margin + watchdog->uncertainty_margin;
>  		if (abs(cs_nsec - wd_nsec) > md) {
> -- 
> 2.35.3
> 
> 
> -- 
> Jiri Wiesner
> SUSE Labs