Messages in this thread | | | Date | Fri, 27 May 2022 22:07:59 -0400 | Subject | Re: [PATCH] clocksource: Make clocksource watchdog check with WATCHDOG_INTERVAL period | From | Waiman Long <> |
| |
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
| |