Message-ID: <9b7337b7-756a-2066-aaf6-ca0e57e5b4de@linaro.org>
Date:   Thu, 26 Jan 2023 11:57:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v2 clocksource 6/7] clocksource: Verify HPET and PMTMR
 when TSC unverified
Content-Language: en-US
To:     "Paul E. McKenney" <paulmck@kernel.org>, tglx@linutronix.de
Cc:     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, feng.tang@intel.com, zhengjun.xing@intel.com,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Waiman Long <longman@redhat.com>,
        x86@kernel.org
References: <20230125002708.GA1471122@paulmck-ThinkPad-P17-Gen-1>
 <20230125002730.1471349-6-paulmck@kernel.org>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20230125002730.1471349-6-paulmck@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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 |
<http://www.linaro.org/linaro-blog/> Blog