Re: [PATCH v2 clocksource 6/7] clocksource: Verify HPET and PMTMR when TSC unverified

[Paul E. McKenney]

On Thu, Jan 26, 2023 at 11:57:41AM +0100, Daniel Lezcano wrote:
> 
> Hi Thomas,
> 
> are you ok with this patch ? Shall I pick it ?

Seeing no response, I sent a pull request.

If it would be helpful for me to make the pilgrimmage to (say) Intel
Jones Farm, I can easily make that trip.

							Thanx, Paul

> Thanks
> 
>   -- Daniel
> 
> 
> On 25/01/2023 01:27, Paul E. McKenney wrote:
> > 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 production-level 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
> > sometimes can 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@xxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxxxx>
> > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > Cc: Waiman Long <longman@xxxxxxxxxx>
> > Cc: <x86@xxxxxxxxxx>
> > Tested-by: Feng Tang <feng.tang@xxxxxxxxx>
> > ---
> >   arch/x86/include/asm/time.h   | 1 +
> >   arch/x86/kernel/hpet.c        | 2 ++
> >   arch/x86/kernel/tsc.c         | 5 +++++
> >   drivers/clocksource/acpi_pm.c | 6 ++++--
> >   4 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/time.h b/arch/x86/include/asm/time.h
> > index 8ac563abb567b..a53961c64a567 100644
> > --- a/arch/x86/include/asm/time.h
> > +++ b/arch/x86/include/asm/time.h
> > @@ -8,6 +8,7 @@
> >   extern void hpet_time_init(void);
> >   extern void time_init(void);
> >   extern bool pit_timer_init(void);
> > +extern bool tsc_clocksource_watchdog_disabled(void);
> >   extern struct clock_event_device *global_clock_event;
> > diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> > index 71f336425e58a..c8eb1ac5125ab 100644
> > --- a/arch/x86/kernel/hpet.c
> > +++ b/arch/x86/kernel/hpet.c
> > @@ -1091,6 +1091,8 @@ int __init hpet_enable(void)
> >   	if (!hpet_counting())
> >   		goto out_nohpet;
> > +	if (tsc_clocksource_watchdog_disabled())
> > +		clocksource_hpet.flags |= CLOCK_SOURCE_MUST_VERIFY;
> >   	clocksource_register_hz(&clocksource_hpet, (u32)hpet_freq);
> >   	if (id & HPET_ID_LEGSUP) {
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index a78e73da4a74b..af3782fb6200c 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1186,6 +1186,11 @@ static void __init tsc_disable_clocksource_watchdog(void)
> >   	clocksource_tsc.flags &= ~CLOCK_SOURCE_MUST_VERIFY;
> >   }
> > +bool tsc_clocksource_watchdog_disabled(void)
> > +{
> > +	return !(clocksource_tsc.flags & CLOCK_SOURCE_MUST_VERIFY);
> > +}
> > +
> >   static void __init check_system_tsc_reliable(void)
> >   {
> >   #if defined(CONFIG_MGEODEGX1) || defined(CONFIG_MGEODE_LX) || defined(CONFIG_X86_GENERIC)
> > diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
> > index 279ddff81ab49..82338773602ca 100644
> > --- a/drivers/clocksource/acpi_pm.c
> > +++ b/drivers/clocksource/acpi_pm.c
> > @@ -23,6 +23,7 @@
> >   #include <linux/pci.h>
> >   #include <linux/delay.h>
> >   #include <asm/io.h>
> > +#include <asm/time.h>
> >   /*
> >    * The I/O port the PMTMR resides at.
> > @@ -210,8 +211,9 @@ static int __init init_acpi_pm_clocksource(void)
> >   		return -ENODEV;
> >   	}
> > -	return clocksource_register_hz(&clocksource_acpi_pm,
> > -						PMTMR_TICKS_PER_SEC);
> > +	if (tsc_clocksource_watchdog_disabled())
> > +		clocksource_acpi_pm.flags |= CLOCK_SOURCE_MUST_VERIFY;
> > +	return clocksource_register_hz(&clocksource_acpi_pm, PMTMR_TICKS_PER_SEC);
> >   }
> >   /* We use fs_initcall because we want the PCI fixups to have run
> 
> -- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>