Re: [PATCH clocksource 1/3] clocksource: Reject bogus watchdog clocksource measurements

From: Feng Tang <feng.tang@intel.com>
To: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	<linux-kernel@vger.kernel.org>, <john.stultz@linaro.org>,
	<sboyd@kernel.org>, <corbet@lwn.net>, <Mark.Rutland@arm.com>,
	<maz@kernel.org>, <kernel-team@meta.com>,
	<neeraju@codeaurora.org>, <ak@linux.intel.com>,
	<zhengjun.xing@intel.com>, Chris Mason <clm@meta.com>,
	John Stultz <jstultz@google.com>,
	Waiman Long <longman@redhat.com>
Subject: Re: [PATCH clocksource 1/3] clocksource: Reject bogus watchdog clocksource measurements
Date: Wed, 23 Nov 2022 10:36:04 +0800	[thread overview]
Message-ID: <Y32HFE+BaATh9+l/@feng-clx> (raw)
In-Reply-To: <20221122220712.GP4001@paulmck-ThinkPad-P17-Gen-1>

On Tue, Nov 22, 2022 at 02:07:12PM -0800, Paul E. McKenney wrote:
[...] 
> > > If PM_TIMER was involved, I would expect 'acpi_pm' instead of
> > > refined-jiffies.  Or am I misinterpreting the output and/or code?
> > 
> > It's about timing. On a typical server platform, the clocksources
> > init order could be:
> >   refined-jiffies --> hpet --> tsc-early --> acpi_pm --> tsc 
> > 
> > >From your log, TSC('tsc-early') is disabled before 'acpi_pm' get
> > initialized, so 'acpi_pm' timer (if exist) had no chance to watchdog
> > the tsc.
> > 
> > > Either way, would it make sense to add CLOCK_SOURCE_MUST_VERIFY to
> > > clocksource_hpet.flags?
> > 
> > Maybe try below patch, which will skip watchdog for 'tsc-early',
> > while giving 'acpi_pm' timer a chance to watchdog 'tsc'.
> > 
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index cafacb2e58cc..9840f0131764 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1131,8 +1131,7 @@ static struct clocksource clocksource_tsc_early = {
> >  	.uncertainty_margin	= 32 * NSEC_PER_MSEC,
> >  	.read			= read_tsc,
> >  	.mask			= CLOCKSOURCE_MASK(64),
> > -	.flags			= CLOCK_SOURCE_IS_CONTINUOUS |
> > -				  CLOCK_SOURCE_MUST_VERIFY,
> > +	.flags			= CLOCK_SOURCE_IS_CONTINUOUS,
> >  	.vdso_clock_mode	= VDSO_CLOCKMODE_TSC,
> >  	.enable			= tsc_cs_enable,
> >  	.resume			= tsc_resume,
> 
> Your mainline patch b50db7095fe0 ("x86/tsc: Disable clocksource watchdog
> for TSC on qualified platorms") mitigates the issue so we are good for
> the immediate future, at least assuming reliable TSC.
> 
> But it also disables checking against HPET, hence my question about
> marking clocksource_hpet.flags with CLOCK_SOURCE_MUST_VERIFY at boot time
> on systems whose CPUs have constant_tsc, nonstop_tsc, and tsc_adjust.

IIUC, this will make TSC to watchdog HPET every 500 ms. We have got
report that the 500ms watchdog timer had big impact on some parallel
workload on big servers, that was another factor for us to seek
stopping the timer.

Is this about the concern of possible TSC frequency calibration
issue, as the 40 ms per second drift between HPET and TSC? With 
b50db7095fe0 backported, we also have another patch to force TSC
calibration for those platforms which get the TSC freq directly
from CPUID or MSR and don't have such info in dmesg:
 "tsc: Refined TSC clocksource calibration: 2693.509 MHz" 

https://lore.kernel.org/lkml/20220509144110.9242-1-feng.tang@intel.com/

We did met tsc calibration issue due to some firmware issue, and
this can help to catch it. You can try it if you think it's relevant.

Thanks,
Feng

> Would that make sense?
> 
> 							Thanx, Paul
> 
> > > I am sending the full console output off-list.  Hey, you asked for it!  ;-)
> > 
> > Thanks for sharing!
> > 
> > Thanks,
> > Feng
> > 
> > > 
> > > 							Thanx, Paul