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

[Feng Tang]

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);
 	}
 
 	/*


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.
> 
> Cheers,
> Longman