Re: [PATCH] clocksource: Make clocksource watchdog check with WATCHDOG_INTERVAL period

[Waiman Long <longman@redhat.com>]

On 5/27/22 21:57, Waiman Long wrote:
> Since commit c86ff8c55b8a ("clocksource: Avoid accidental unstable
> marking of clocksource"), a new WD_READ_SKIP value was introduced
> as a possible return value of cs_watchdog_read() to skip the current
> check. However, this has an undesriable side effect of extending the
> time gap between csnow and cs_last to more than one WATCHDOG_INTERVAL
> (0.5s) in case of intermittent WD_READ_SKIP's.
>
> There was an instance of reported clocksource watchdog failure with
> the time skew of 485us where the uncertainly threshold is 400us. In
> that particular case, the (now - last) gap was about 2s. Looking at
> the dmesg log, it was clear there was a successful cs_watchdog_read()
> followed by 3 skips and then another successful cs_watchdog_read().
>
> If there is an existing skew between the hpet (watchdog) and tsc
> clocksource, enlarging the period by 4x will certainly increase the
> measured skew causing it to exceed the threshold in this case. Fix
> this variable period problem by resetting the CLOCK_SOURCE_WATCHDOG bit
> after each WD_READ_SKIP to force the reloading of wd_last and cs_last
> in the next round. This ensures that we have two consecutive successful
> cs_watchdog_read()'s before checking the clock skew.
>
> Fixes: c86ff8c55b8a ("clocksource: Avoid accidental unstable marking of clocksource")
> Reported-by: Michael Larabel <Michael@phoronix.com>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>   kernel/time/clocksource.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index cee5da1e54c4..173e052c12b6 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -411,9 +411,18 @@ static void clocksource_watchdog(struct timer_list *unused)
>   		read_ret = cs_watchdog_read(cs, &csnow, &wdnow);
>   
>   		if (read_ret != WD_READ_SUCCESS) {
> -			if (read_ret == WD_READ_UNSTABLE)
> +			if (read_ret == WD_READ_UNSTABLE) {
>   				/* Clock readout unreliable, so give it up. */
>   				__clocksource_unstable(cs);
> +			} else { /* WD_READ_SKIP */
> +				/*
> +				 * Watchdog clock unstable at the moment,
> +				 * discard the stored wd_last and cs_last to
> +				 * make sure the gap between now and last
> +				 * is always one WATCHDOG_INTERVAL.
> +				 */
> +				cs->flags &= ~CLOCK_SOURCE_WATCHDOG;
> +			}
>   			continue;
>   		}
>   

Sorry, I accidentally use the old email address for John.

Cheers,
Longman