From: Thomas Gleixner <email@example.com> To: firstname.lastname@example.org, Feng Tang <email@example.com> Cc: kernel test robot <firstname.lastname@example.org>, 0day robot <email@example.com>, John Stultz <firstname.lastname@example.org>, Stephen Boyd <email@example.com>, Jonathan Corbet <firstname.lastname@example.org>, Mark Rutland <Mark.Rutland@arm.com>, Marc Zyngier <email@example.com>, Andi Kleen <firstname.lastname@example.org>, Xing Zhengjun <email@example.com>, LKML <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, Paolo Bonzini <firstname.lastname@example.org> Subject: Re: [clocksource] 8c30ace35d: WARNING:at_kernel/time/clocksource.c:#clocksource_watchdog Date: Wed, 28 Apr 2021 15:34:52 +0200 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> Paul, On Tue, Apr 27 2021 at 23:09, Thomas Gleixner wrote: > On Tue, Apr 27 2021 at 10:50, Paul E. McKenney wrote: >> OK, it turns out that there are systems for which boot times in excess >> of one minute are expected behavior. They are a bit rare, though. >> So what I will do is keep the 60-second default, add a boot parameter, >> and also add a comment by the warning pointing out the boot parameter. > > Oh, no. This starts to become yet another duct tape horror show. > > I'm not at all against a more robust and resilent watchdog mechanism, > but having a dozen knobs to tune and heuristics which are doomed to fail > is not a solution at all. That said, let's take a step back and look at the trainwreck from a different POV. Let's start with the easy part: Virtualization While I'm still convinced that virt creates more problems than it solves, in this particular case, the virt solution is actually halfways trivial. 1) If the host does not trust the TSC then it clearly wants the guest to use KVM clock and not TSC. KVM clock is still utilizing TSC, but it's a host controlled and assisted mechanism. 2) If the host has clear indications that TSC can be trusted, then it can tell the guest that TSC is trustworthy. A synthesized CPU feature bit is the obvious solution for this. That solves several virt problems at once: - The watchdog issue - The fact that TSC synchronization checks between two vCPUs cannot ever work reliably. Now for the bare metal case. We have to distinguish the following scenarios: 1) CPU does not advertise X86_FEATURE_CONSTANT_TSC That's a lost case and only relevant for really old hardware. 2) CPU does advertise X86_FEATURE_CONSTANT_TSC, but does not have X86_FEATURE_NONSTOP_TSC Mostly a lost case as well unless you disable the C-states in which TSC stops working. 3) CPU does advertise X86_FEATURE_CONSTANT_TSC and X86_FEATURE_NONSTOP_TSC That's the point where usable starts, which is around 2007/2008 4) CPU has X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC and TSC_ADJUST That's anything Intel starting from Haswell - not sure about the ATOM parts though. Non-Intel CPUs lack this completely. 5) CPU has X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC and TSC_ADJUST and the not yet existing feature TSC_LOCKDOWN We're asking for this for at least 15 years now, but we also had to wait 10+ years to get a halfways usable TSC, so why am I complaining? So we really care about #3 and #4. #4 is the easy case because we can check MSR_TSC_ADJUST to figure out whether something has written to MSR_TSC or MSR_TSC_ADJUST and undo the damage in a sane way. That's currently only done when a CPU goes idle, but there are options to do that differently (timer, TIF flag ...) This allows to disable the clocksource watchdog completely and based on Fengs work we are going to do so sooner than later. The problematic case is #3 because that affects any Non-Intel CPUs and the pre Haswell Intel ones. The approach we have taken so far is the clocksource watchdog in the form in which it exists today with the known limitations: a) Broken watchdog clocksource. Not that I care much, because then all bets are off. This includes the jiffies based watchdog which is the worst bandaid which we have, though that can be made 'work' by some definition of work. b) Long delays which prevent the watchdog from running which in the worst case let the watchdog wrap around. For ACPI_PMTIMER that's ~4.69 seconds and for HPET ~300 seconds. Anything which keeps the watchdog timer from running for that long is broken and trying to fix that at the watchdog level is putting the cart before the horse. Ignore virt here. See above. c) vCPUs scheduled out between the watchdog and the TSC read Cannot be mitigated reliably under all circumstances. See the virt section above. d) Long lasting NMI/SMI's between watchdog and TSC read Can be mitigated with reread/retry as you demonstrated. e) A too large threshold, which if reduced causes other problems as you figured out already. Unfortunately there is no other way than using the watchdog mechanism, simply because we need hardware assistance for detection and a reliable way to undo the damage, which is what we have with TSC_ADJUST. Of course the best case would be a mechanism to prevent writes to TSC/TSC_ADJUST completely after boot. Now let's look at the cases which cause TSC problems in the real world: 1) Boot time (or resume time) TSC inconsistency That's the major problem today, but that's not a watchdog issue. On TSC_ADJUST equipped machines we fix the wreckage up, on others we give up. For the latter case we don't need a watchdog :) 2) Runtime wreckage caused by BIOS/SMM That used to be a wide spread problem 10 years ago and today it's only a sporadic issue, but it's not eliminated completely which is why we have this discussion at all. As we have no reliable indicator whether a BIOS can be trusted and history taught us that it can't be trusted, we need to have this trainwreck until hardware people actually come to senses and fix the root cause once and forever. So let's look at the issues a - e above: a) Is not fixable though the hillarious refined-jiffies case can be handled to some degree. But I really don't care much about it because it's a lost case. b) Not interesting at all. If that ever happens, then sure the TSC will be marked unstable for the wrong reasons, but that's the least of the problems in that case. And that includes virt because virt should not use the watchdog ever. c) Not relevant because virt has to solve the problems which virt causes at the virt level and not here. d) The reread/retry mechanism is sensible. Though all the command line knobs for delay injection are really horrible. If at all this can be avoided by having a special watchdog clocksource module which registers a 'preferred' watchdog clocksource and hides all the magic outside of the actual watchdog code. Neither I'm sure whether this IPI collect data muck is adding much value, but shrug. e) As I said elsewhere already this is an issue which has two components if we want to handle the refined-jiffies case: clocksource frequency is not accurate: early-TSC watchdog is not accurate: refined-jiffies So the threshold wants to be: max(cs->uncertainty_margin, wd->uncertainty_margin) So the right thing to do here is to set a small and reasonable default margin ($N us) in the clock source registration code if cs->uncertainty_margin == 0 and add the larger margins to early-TSC and refined-jiffies. The only other option to handle this mess is to declare the watchdog experiment as failed, rip it out completely and emit a fat warning on boot when a non-trustable TSC is detected: HARDWARE-BUG: Untrusted TSC detected. For further information see: https://www.kernel.org/doc/html/latest/admin-guide/hw-trainwrecks/tsc.html I can live with that and maybe we should have done that 15 years ago instead of trying to work around it at the symptom level. In case we agree on that option, I'm volunteering to write the documentation. Thanks, tglx
next prev parent reply other threads:[~2021-04-28 13:34 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-25 22:45 [PATCH v10 clocksource 0/7] Do not mark clocks unstable due to delays for v5.13 Paul E. McKenney 2021-04-25 22:47 ` [PATCH v10 clocksource 1/7] clocksource: Provide module parameters to inject delays in watchdog Paul E. McKenney 2021-04-26 4:07 ` Andi Kleen 2021-04-26 7:13 ` Thomas Gleixner 2021-04-26 15:28 ` Paul E. McKenney 2021-04-26 16:00 ` Andi Kleen 2021-04-26 16:14 ` Paul E. McKenney 2021-04-26 17:56 ` Andi Kleen 2021-04-26 18:24 ` Paul E. McKenney 2021-04-28 4:49 ` Luming Yu 2021-04-28 13:57 ` Paul E. McKenney 2021-04-28 14:24 ` Luming Yu 2021-04-28 14:37 ` Thomas Gleixner 2021-04-25 22:47 ` [PATCH v10 clocksource 2/7] clocksource: Retry clock read if long delays detected Paul E. McKenney 2021-04-27 1:44 ` Feng Tang 2021-04-25 22:47 ` [PATCH v10 clocksource 3/7] clocksource: Check per-CPU clock synchronization when marked unstable Paul E. McKenney 2021-04-26 4:12 ` Andi Kleen 2021-04-26 7:16 ` Thomas Gleixner 2021-04-25 22:47 ` [PATCH v10 clocksource 4/7] clocksource: Provide a module parameter to fuzz per-CPU clock checking Paul E. McKenney 2021-04-25 22:47 ` [PATCH v10 clocksource 5/7] clocksource: Limit number of CPUs checked for clock synchronization Paul E. McKenney 2021-04-25 22:47 ` [PATCH v10 clocksource 6/7] clocksource: Forgive tsc_early pre-calibration drift Paul E. McKenney 2021-04-26 15:01 ` Feng Tang 2021-04-26 15:25 ` Paul E. McKenney 2021-04-26 15:36 ` Feng Tang 2021-04-26 18:26 ` Paul E. McKenney 2021-04-27 1:13 ` Feng Tang 2021-04-27 3:46 ` Paul E. McKenney 2021-04-27 4:16 ` Feng Tang 2021-04-26 15:28 ` Thomas Gleixner 2021-04-27 21:03 ` Thomas Gleixner 2021-04-27 7:27 ` [clocksource] 8c30ace35d: WARNING:at_kernel/time/clocksource.c:#clocksource_watchdog kernel test robot 2021-04-27 8:45 ` Feng Tang 2021-04-27 13:37 ` Paul E. McKenney 2021-04-27 17:50 ` Paul E. McKenney 2021-04-27 21:09 ` Thomas Gleixner 2021-04-28 1:48 ` Paul E. McKenney 2021-04-28 10:14 ` Thomas Gleixner 2021-04-28 18:31 ` Paul E. McKenney 2021-04-28 13:34 ` Thomas Gleixner [this message] 2021-04-28 15:39 ` Peter Zijlstra 2021-04-28 17:00 ` Thomas Gleixner 2021-04-29 7:38 ` Feng Tang 2021-04-28 18:31 ` Paul E. McKenney 2021-04-29 8:27 ` Thomas Gleixner 2021-04-29 14:26 ` Paul E. McKenney 2021-04-29 17:30 ` Thomas Gleixner 2021-04-29 23:04 ` Andi Kleen 2021-04-30 0:24 ` Paul E. McKenney 2021-04-30 0:59 ` Paul E. McKenney 2021-04-30 5:08 ` Paul E. McKenney 2021-04-25 22:47 ` [PATCH v9 clocksource 6/6] clocksource: Reduce WATCHDOG_THRESHOLD Paul E. McKenney 2021-04-25 22:47 ` [PATCH v10 clocksource 7/7] " Paul E. McKenney
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --cc=Mark.Rutland@arm.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [clocksource] 8c30ace35d: WARNING:at_kernel/time/clocksource.c:#clocksource_watchdog' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).