Re: [RFC PATCH] clocksource: Suspend the watchdog temporarily when high read lantency detected

[Paul E. McKenney]

On Fri, Dec 23, 2022 at 12:14:57PM +0800, Feng Tang wrote:
> On Thu, Dec 22, 2022 at 10:49:23PM -0500, Waiman Long wrote:
> > On 12/22/22 22:37, Paul E. McKenney wrote:
> > > > > commit dfbf67806c4c7f2bdd79cdefe86a2bea6e7afcab
> > > > > Author: Paul E. McKenney<paulmck@xxxxxxxxxx>
> > > > > Date:   Thu Dec 22 13:21:47 2022 -0800
> > > > > 
> > > > >      clocksource: Permit limited-duration clocksource watchdogging
> > > > >      Some systems want regular clocksource checking, but their workloads
> > > > >      cannot tolerate the overhead of full-time clocksource checking.
> > > > >      Therefore, add a CLOCKSOURCE_WATCHDOG_DURATION Kconfig option and a
> > > > >      clocksource.watchdog_duration kernel-boot parameter that limits the
> > > > >      clocksource watchdog to the specified number of minutes past boot.
> > > > >      Values of zero disable checking, and a value of -1 restores the
> > > > >      traditional behavior of always checking.
> > > > >      This does change behavior compared to older kernels, but recent kernels
> > > > >      disable the clocksource watchdog completely in the common case where the
> > > > >      TSC is judged to be trustworthy.  This change in behavior is therefore
> > > > >      not a real issue.
> > > > Yes, this changes the general semantics. Last year, I've posted a
> > > > patch to limit the watchdog to run for 10 minutes, and at that time
> > > > Thomas mentioned one of his machine may show tsc issue after running
> > > > for one day depending on work load [1].
> > > > 
> > > > As the intention is to validate HPET/PMTIMER, which are not as
> > > > delicate as TSC, maybe we can add a per-clocksource verify-period
> > > > field, and only set it for HPET/PMTIMER?
> > > > 
> > > > [1].https://lore.kernel.org/lkml/875z286xtk.fsf@xxxxxxxxxxxxxxxxxxxxxxx/
> > > Got it.
> > > 
> > > The workloads I am closest to are OK with the clocksource watchdog
> > > running indefinitely, but thus far the skew is visible very early.
> > > But broken hardware can do whatever it wants whenever it wants.  I could
> > > meet Thomas's request by making the default be indefinite, and allowing
> > > whoever cares to make it finite.  Or maybe the fact that the TSC is not
> > > marked unstable makes a difference.
> > > 
> > > Thoughts?
> > 
> > Sorry for the late reply.
> > 
> > Maybe the default should be an auto mode where if TSC is marked stable and
> > don't need to verify, we can run watchdog for HPET and PMTMR for 10 mins.
> > Otherwise, run it indefinitely to not change existing behavior.
> 
> Yes, sounds reasonable to me. 
> 
> btw, what I suggested in last mail is some code (untested) like this:
> 
> ---
> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> index c8eb1ac5125a..db20aac5d14d 100644
> --- a/arch/x86/kernel/hpet.c
> +++ b/arch/x86/kernel/hpet.c
> @@ -862,6 +862,7 @@ static struct clocksource clocksource_hpet = {
>  	.mask		= HPET_MASK,
>  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
>  	.resume		= hpet_resume_counter,
> +	.wd_limited	= true,
>  };
>  
>  /*
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 1d42d4b17327..2b6278f69516 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -125,6 +125,8 @@ struct clocksource {
>  	struct list_head	wd_list;
>  	u64			cs_last;
>  	u64			wd_last;
> +	bool			wd_limited;
> +	u64			wd_iters;
>  #endif
>  	struct module		*owner;
>  };
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 777a5eba68fd..eb2d9adf06b0 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -425,6 +425,8 @@ static void clocksource_watchdog(struct timer_list *unused)
>  			cs->flags |= CLOCK_SOURCE_WATCHDOG;
>  			cs->wd_last = wdnow;
>  			cs->cs_last = csnow;
> +			if (cs->wd_limited)
> +				cs->wd_iters = 1200;
>  			continue;
>  		}
>  
> @@ -492,6 +494,9 @@ static void clocksource_watchdog(struct timer_list *unused)
>  				tick_clock_notify();
>  			}
>  		}
> +
> +		if (cs->wd_limited && !(cs->wd_iters--))
> +			list_del_init(&cs->wd_list);
>  	}
>  
>  	/*

If this works for Waiman, and given a proper patch, I am happy to replace
my b90f0a5cfc0e ("clocksource: Permit limited-duration clocksource
watchdogging") with this that proper patch.

> Thanks,
> Feng
> 
> > Given such a default, I don't think we need your second patch to determine
> > if both HPET and PMTMR needs to be checked.

And indeed, this one is off to the side, not on track for mainline:

375e65d3055f ("clocksource: Limit the number of watchdogged clocksources")

But if I am confused about which patch you mean, please let me know!

							Thanx, Paul