Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Dec 2022 22:14:29 -0800
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Feng Tang <feng.tang@intel.com>
Cc:     Waiman Long <longman@redhat.com>, John Stultz <jstultz@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>, x86@kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org, Tim Chen <tim.c.chen@intel.com>
Subject: Re: [RFC PATCH] clocksource: Suspend the watchdog temporarily when
 high read lantency detected
Message-ID: <20221222061429.GL4001@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20221220082512.186283-1-feng.tang@intel.com>
 <6fb04ee9-ce77-4835-2ad1-b7f8419cfb77@redhat.com>
 <20221220183400.GY4001@paulmck-ThinkPad-P17-Gen-1>
 <Y6Ja+kYQAi4pppV6@feng-clx>
 <8a9bed0d-c166-37e9-24c3-8cea7a336c76@redhat.com>
 <20221222004032.GI4001@paulmck-ThinkPad-P17-Gen-1>
 <a82092f5-abc8-584f-b2ba-f06c82ffbe7d@redhat.com>
 <20221222055515.GJ4001@paulmck-ThinkPad-P17-Gen-1>
 <Y6PyisHYYtde/6Xk@feng-clx>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y6PyisHYYtde/6Xk@feng-clx>
Precedence: bulk

On Thu, Dec 22, 2022 at 02:00:42PM +0800, Feng Tang wrote:
> On Wed, Dec 21, 2022 at 09:55:15PM -0800, Paul E. McKenney wrote:
> > On Wed, Dec 21, 2022 at 10:39:53PM -0500, Waiman Long wrote:
> > > On 12/21/22 19:40, Paul E. McKenney wrote:
> > > > commit 199dfa2ba23dd0d650b1482a091e2e15457698b7
> > > > Author: Paul E. McKenney<paulmck@kernel.org>
> > > > Date:   Wed Dec 21 16:20:25 2022 -0800
> > > > 
> > > >      clocksource: Verify HPET and PMTMR when TSC unverified
> > > >      On systems with two or fewer sockets, when the boot CPU has CONSTANT_TSC,
> > > >      NONSTOP_TSC, and TSC_ADJUST, clocksource watchdog verification of the
> > > >      TSC is disabled.  This works well much of the time, but there is the
> > > >      occasional system that meets all of these criteria, but which still
> > > >      has a TSC that skews significantly from atomic-clock time.  This is
> > > >      usually attributed to a firmware or hardware fault.  Yes, the various
> > > >      NTP daemons do express their opinions of userspace-to-atomic-clock time
> > > >      skew, but they put them in various places, depending on the daemon and
> > > >      distro in question.  It would therefore be good for the kernel to have
> > > >      some clue that there is a problem.
> > > >      The old behavior of marking the TSC unstable is a non-starter because a
> > > >      great many workloads simply cannot tolerate the overheads and latencies
> > > >      of the various non-TSC clocksources.  In addition, NTP-corrected systems
> > > >      often seem to be able to tolerate significant kernel-space time skew as
> > > >      long as the userspace time sources are within epsilon of atomic-clock
> > > >      time.
> > > >      Therefore, when watchdog verification of TSC is disabled, enable it for
> > > >      HPET and PMTMR (AKA ACPI PM timer).  This provides the needed in-kernel
> > > >      time-skew diagnostic without degrading the system's performance.
> > > >      Signed-off-by: Paul E. McKenney<paulmck@kernel.org>
> > > >      Cc: Thomas Gleixner<tglx@linutronix.de>
> > > >      Cc: Ingo Molnar<mingo@redhat.com>
> > > >      Cc: Borislav Petkov<bp@alien8.de>
> > > >      Cc: Dave Hansen<dave.hansen@linux.intel.com>
> > > >      Cc: "H. Peter Anvin"<hpa@zytor.com>
> > > >      Cc: Daniel Lezcano<daniel.lezcano@linaro.org>
> > > >      Cc: Feng Tang<feng.tang@intel.com>
> > > >      Cc: Waiman Long <longman@redhat.com
> > > >      Cc:<x86@kernel.org>
> > > 
> > > As I currently understand, you are trying to use TSC as a watchdog to check
> > > against HPET and PMTMR. I do have 2 questions about this patch.
> > > 
> > > First of all, why you need to use both HPET and PMTMR? Can you just use one
> > > of those that are available. Secondly, is it possible to enable this
> > > time-skew diagnostic for a limit amount of time instead running
> > > indefinitely? The running of the clocksource watchdog itself will still
> > > consume a tiny amount of CPU cycles.
> > 
> > I could certainly do something so that only the first of HPET and PMTMR
> > is checked.  Could you give me a quick run-through of the advantages of
> > using only one?  I would need to explain that in the commit log.
> > 
> > Would it make sense to have a kernel boot variable giving the number of
> > minutes for which the watchdog was to run, with a default of zero
> > meaning "indefinitely"?
> 
> We've discussed about the "os noise", which customer may really care.
> IIUC, this patch intends to test if HPET/PMTIMER HW is broken, so how
> about making it run for a number of minutes the default behavior.   

It is also intended to determine if TSC is broken, with NTP drift rates
used to determine which timer is at fault.

OK, how about a Kconfig option for the number of minutes, set to whatever
you guys tell me?  (Three minutes?  Five minutes?  Something else?)
People wanting to run it continuously could then build their kernels
with that Kconfig option set to zero.

> Also I've run the patch on a Alderlake system, with a fine acpi pm_timer
> and a fake broken pm_timer, and they both works without errors.

Thank you!  Did it correctly identify the fake broken pm_timer as being
broken?  If so, may I have your Tested-by?

							Thanx, Paul