Received-SPF: pass (google.com: domain of linux-kernel+bounces-24870-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Jiri Wiesner <jwiesner@suse.de>, linux-kernel@vger.kernel.org
Cc: John Stultz <jstultz@google.com>, Stephen Boyd <sboyd@kernel.org>, "Paul
 E. McKenney" <paulmck@kernel.org>, Feng Tang <feng.tang@intel.com>
Subject: Re: [PATCH v2] clocksource: Skip watchdog check for large watchdog
 intervals
In-Reply-To: <20240110192623.GA7158@incl>
References: <20240110192623.GA7158@incl>
Date: Fri, 12 Jan 2024 17:48:22 +0100
Message-ID: <875xzyijl5.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Wed, Jan 10 2024 at 20:26, Jiri Wiesner wrote:
> 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.

This really took some time to decode. What you are trying to explain is:

   The comparison between the clocksource and the watchdog is not
   working for large readout intervals because the conversion to
   nanoseconds is imprecise. The reason is that the initialization
   values of the shift/mult pairs which are used for conversion are not
   sufficiently accurate and the accumulated inaccuracy causes the
   comparison to exceed the threshold.

Right?

So yes, limiting the maximum readout interval and skipping the check is
sensible.

> 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

  s/The solution in this patch skips/Prevent this by skipping/

We already know that this is a patch, no?

> v2: fixed interger overflow in WATCHDOG_INTR_MAX_NS on i386

Please put the version log after the --- separator. It's not part of the
changelog.

> Fixes: 2e27e793e280 ("clocksource: Reduce clocksource-skew threshold")
> Suggested-by: Feng Tang <feng.tang@intel.com>
> Reviewed-by: Feng Tang <feng.tang@intel.com>
> Tested-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Jiri Wiesner <jwiesner@suse.de>
> ---
>  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..e7f8d0a1b95c 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))

That 1.5 * WATCHDOG_INTERVAL seems to be rather arbitrary. One second
should be safe enough, no?
>  
> +		/*
> +		 * 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.

What ensures that the watchdog did not wrap around then?

> +		 * 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 is a misnomer. Why not naming it watchdog_max_interval
to make it entirly clear what this is about?

> +				watchdog_max_intr = interval;
> +				pr_warn("Skipping watchdog check: cs_nsec: %lld wd_nsec: %lld\n",
> +					cs_nsec, wd_nsec);

This really wants to have a proper indication why the check was skipped,
i,e. due to a long readout interval, no?

Thanks,

        tglx