2022-10-21 06:48:54

by Feng Tang

[permalink] [raw]
Subject: [PATCH v1 2/2] x86/tsc: Extend watchdog check exemption to 4-Sockets platform

There is report again that the tsc clocksource on a 4 sockets x86
Skylake server was wrongly judged as 'unstable' by 'jiffies' watchdog,
and disabled [1].

Commit b50db7095fe0 ("x86/tsc: Disable clocksource watchdog for TSC
on qualified platorms") was introduce to deal with these false
alarms of tsc unstable issues, covering qualified platforms for 2
sockets or smaller ones.

Extend the exemption to 4 sockets to fix the issue.

We also got similar reports on 8 sockets platform from internal test,
but as Peter pointed out, there was tsc sync issues for 8-sockets
platform, and it'd better be handled architecture by architecture,
instead of directly changing the threshold to 8 here.

Rui also proposed another way to disable 'jiffies' as clocksource
watchdog [2], which can also solve this specific problem in an
architecture independent way, with one limitation that there are
also some tsc false alarms which were reported by other hardware
watchdogs like HPET/PMTIMER, while 'jiffies' watchdog is mostly
used in kernel boot phase.

[1]. https://lore.kernel.org/all/[email protected]/
[2]. https://lore.kernel.org/all/[email protected]/

Reported-by: Yu Liao <[email protected]>
Signed-off-by: Feng Tang <[email protected]>
---
arch/x86/kernel/tsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 178448ef00c7..356f06287034 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1400,7 +1400,7 @@ static int __init init_tsc_clocksource(void)
if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
boot_cpu_has(X86_FEATURE_TSC_ADJUST) &&
- logical_packages <= 2)
+ logical_packages <= 4)
clocksource_tsc.flags &= ~CLOCK_SOURCE_MUST_VERIFY;

/*
--
2.34.1


2023-06-02 18:18:42

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] x86/tsc: Extend watchdog check exemption to 4-Sockets platform

On Fri, Oct 21, 2022 at 02:21:31PM +0800, Feng Tang wrote:
> There is report again that the tsc clocksource on a 4 sockets x86
> Skylake server was wrongly judged as 'unstable' by 'jiffies' watchdog,
> and disabled [1].
>
> Commit b50db7095fe0 ("x86/tsc: Disable clocksource watchdog for TSC
> on qualified platorms") was introduce to deal with these false
> alarms of tsc unstable issues, covering qualified platforms for 2
> sockets or smaller ones.
>
> Extend the exemption to 4 sockets to fix the issue.
>
> We also got similar reports on 8 sockets platform from internal test,
> but as Peter pointed out, there was tsc sync issues for 8-sockets
> platform, and it'd better be handled architecture by architecture,
> instead of directly changing the threshold to 8 here.
>
> Rui also proposed another way to disable 'jiffies' as clocksource
> watchdog [2], which can also solve this specific problem in an
> architecture independent way, with one limitation that there are
> also some tsc false alarms which were reported by other hardware
> watchdogs like HPET/PMTIMER, while 'jiffies' watchdog is mostly
> used in kernel boot phase.
>
> [1]. https://lore.kernel.org/all/[email protected]/
> [2]. https://lore.kernel.org/all/[email protected]/
>
> Reported-by: Yu Liao <[email protected]>
> Signed-off-by: Feng Tang <[email protected]>

We have a number of four-socket systems whose TSCs seem to be reliable.
We do see issues where high memory load forces the TSC to be marked
unstable, but that is because those systems are using an older kernel.

If the TSCs do start to misbehave, I will of course let you all know.
But in the meantime:

Reviewed-by: Paul E. McKenney <[email protected]>

The previous patch that changes the definition of "socket" I have no
opinion on. I must let you guys work that out. However, I do note that
this patch can be rebased so as to no longer depend on that patch.

Thanx, Paul

> ---
> arch/x86/kernel/tsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 178448ef00c7..356f06287034 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1400,7 +1400,7 @@ static int __init init_tsc_clocksource(void)
> if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> boot_cpu_has(X86_FEATURE_TSC_ADJUST) &&
> - logical_packages <= 2)
> + logical_packages <= 4)
> clocksource_tsc.flags &= ~CLOCK_SOURCE_MUST_VERIFY;
>
> /*
> --
> 2.34.1
>

2023-06-05 06:40:42

by Feng Tang

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] x86/tsc: Extend watchdog check exemption to 4-Sockets platform

On Fri, Jun 02, 2023 at 11:00:55AM -0700, Paul E. McKenney wrote:
> On Fri, Oct 21, 2022 at 02:21:31PM +0800, Feng Tang wrote:
> > There is report again that the tsc clocksource on a 4 sockets x86
> > Skylake server was wrongly judged as 'unstable' by 'jiffies' watchdog,
> > and disabled [1].
> >
> > Commit b50db7095fe0 ("x86/tsc: Disable clocksource watchdog for TSC
> > on qualified platorms") was introduce to deal with these false
> > alarms of tsc unstable issues, covering qualified platforms for 2
> > sockets or smaller ones.
> >
> > Extend the exemption to 4 sockets to fix the issue.
> >
> > We also got similar reports on 8 sockets platform from internal test,
> > but as Peter pointed out, there was tsc sync issues for 8-sockets
> > platform, and it'd better be handled architecture by architecture,
> > instead of directly changing the threshold to 8 here.
> >
> > Rui also proposed another way to disable 'jiffies' as clocksource
> > watchdog [2], which can also solve this specific problem in an
> > architecture independent way, with one limitation that there are
> > also some tsc false alarms which were reported by other hardware
> > watchdogs like HPET/PMTIMER, while 'jiffies' watchdog is mostly
> > used in kernel boot phase.
> >
> > [1]. https://lore.kernel.org/all/[email protected]/
> > [2]. https://lore.kernel.org/all/[email protected]/
> >
> > Reported-by: Yu Liao <[email protected]>
> > Signed-off-by: Feng Tang <[email protected]>
>
> We have a number of four-socket systems whose TSCs seem to be reliable.
> We do see issues where high memory load forces the TSC to be marked
> unstable, but that is because those systems are using an older kernel.

Thanks for sharing the info.

>
> If the TSCs do start to misbehave, I will of course let you all know.

That will be very helpful! I don't have much access to 4 socket machines.

> But in the meantime:
>
> Reviewed-by: Paul E. McKenney <[email protected]>
>
> The previous patch that changes the definition of "socket" I have no
> opinion on. I must let you guys work that out. However, I do note that
> this patch can be rebased so as to no longer depend on that patch.

During previous discussion, Thomas and Peter mentioned they only
saw real TSC synchronization issue on some old generation 8
socket/package machine.

I can separate this and send out for review. Meanwhile I'll rework
on the 1/2 patch and test more.

Thanks,
Feng

> Thanx, Paul
>
> > ---
> > arch/x86/kernel/tsc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 178448ef00c7..356f06287034 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1400,7 +1400,7 @@ static int __init init_tsc_clocksource(void)
> > if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> > boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> > boot_cpu_has(X86_FEATURE_TSC_ADJUST) &&
> > - logical_packages <= 2)
> > + logical_packages <= 4)
> > clocksource_tsc.flags &= ~CLOCK_SOURCE_MUST_VERIFY;
> >
> > /*
> > --
> > 2.34.1
> >