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

Daniel Lezcano
2023-01-26T10:57:55Z

Hi Thomas,

are you ok with this patch ? Shall I pick it ?

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@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: Waiman Long <longman@redhat.com>
> Cc: <x86@kernel.org>
> Tested-by: Feng Tang <feng.tang@intel.com>
> ---
> 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 |