From: Thomas Gleixner <tglx@linutronix.de>
To: "Paul E. McKenney" <paulmck@kernel.org>
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@fb.com, neeraju@codeaurora.org,
ak@linux.intel.com, "Paul E. McKenney" <paulmck@kernel.org>,
Chris Mason <clm@fb.com>
Subject: Re: [PATCH v8 clocksource 2/5] clocksource: Retry clock read if long delays detected
Date: Sat, 17 Apr 2021 14:24:23 +0200 [thread overview]
Message-ID: <87y2dhrte0.ffs@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20210414043602.2812981-2-paulmck@kernel.org>
On Tue, Apr 13 2021 at 21:35, Paul E. McKenney wrote:
> #define WATCHDOG_INTERVAL (HZ >> 1)
> #define WATCHDOG_THRESHOLD (NSEC_PER_SEC >> 4)
> +#define WATCHDOG_MAX_SKEW (NSEC_PER_SEC >> 6)
That's ~15ms which is a tad large I'd say...
> static void clocksource_watchdog_work(struct work_struct *work)
> {
> @@ -213,9 +214,10 @@ static void clocksource_watchdog_inject_delay(void)
> static void clocksource_watchdog(struct timer_list *unused)
> {
> struct clocksource *cs;
> - u64 csnow, wdnow, cslast, wdlast, delta;
> - int64_t wd_nsec, cs_nsec;
> + u64 csnow, wdnow, wdagain, cslast, wdlast, delta;
> + int64_t wd_nsec, wdagain_delta, wderr_nsec = 0, cs_nsec;
> int next_cpu, reset_pending;
> + int nretries;
>
> spin_lock(&watchdog_lock);
> if (!watchdog_running)
> @@ -224,6 +226,7 @@ static void clocksource_watchdog(struct timer_list *unused)
> reset_pending = atomic_read(&watchdog_reset_pending);
>
> list_for_each_entry(cs, &watchdog_list, wd_list) {
> + nretries = 0;
>
> /* Clocksource already marked unstable? */
> if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
> @@ -232,11 +235,24 @@ static void clocksource_watchdog(struct timer_list *unused)
> continue;
> }
>
> +retry:
> local_irq_disable();
> - csnow = cs->read(cs);
> - clocksource_watchdog_inject_delay();
> wdnow = watchdog->read(watchdog);
> + clocksource_watchdog_inject_delay();
> + csnow = cs->read(cs);
> + wdagain = watchdog->read(watchdog);
> local_irq_enable();
> + delta = clocksource_delta(wdagain, wdnow, watchdog->mask);
> + wdagain_delta = clocksource_cyc2ns(delta, watchdog->mult, watchdog->shift);
> + if (wdagain_delta > WATCHDOG_MAX_SKEW) {
> + wderr_nsec = wdagain_delta;
> + if (nretries++ < max_read_retries)
> + goto retry;
> + }
> + if (nretries) {
> + pr_warn("timekeeping watchdog on CPU%d: %s read-back delay of %lldns, attempt %d\n",
> + smp_processor_id(), watchdog->name, wderr_nsec, nretries);
> + }
>
> /* Clocksource initialized ? */
> if (!(cs->flags & CLOCK_SOURCE_WATCHDOG) ||
This can nicely be split out into a read function which avoids brain
overload when reading. Something like the uncompiled below.
I so wish we could just delete all of this horror instead of making it
more horrible.
Thanks,
tglx
---
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -124,6 +124,12 @@ static void __clocksource_change_rating(
#define WATCHDOG_INTERVAL (HZ >> 1)
#define WATCHDOG_THRESHOLD (NSEC_PER_SEC >> 4)
+/*
+ * The maximum delay between two consecutive readouts of the watchdog
+ * clocksource to detect SMI,NMI,vCPU preemption.
+ */
+#define WATCHDOG_MAX_DELAY (100 * NSEC_PER_USEC)
+
static void clocksource_watchdog_work(struct work_struct *work)
{
/*
@@ -184,12 +190,37 @@ void clocksource_mark_unstable(struct cl
spin_unlock_irqrestore(&watchdog_lock, flags);
}
+static bool cs_watchdog_read(struct clocksource *cs, u64 *csnow, u64 *wdnow)
+{
+ unsigned int nretries;
+ u64 wd_end, wd_delta;
+ int64_t wd_delay;
+
+ for (nretries = 0; nretries < max_read_retries; nretries++) {
+ local_irq_disable();
+ *wdnow = watchdog->read(watchdog);
+ clocksource_watchdog_inject_delay();
+ *csnow = cs->read(cs);
+ wd_end = watchdog->read(watchdog);
+ local_irq_enable();
+
+ wd_delta = clocksource_delta(wd_end, *wdnow, watchdog->mask);
+ wd_delay = clocksource_cyc2ns(wd_delta, watchdog->mult, watchdog->shift);
+ if (wd_delay < WATCHDOG_MAX_DELAY)
+ return true;
+ }
+
+ pr_warn("timekeeping watchdog on CPU%d: %s read-back delay of %lldns, %d attempts\n",
+ smp_processor_id(), watchdog->name, wd_delay, nretries);
+ return false;
+}
+
static void clocksource_watchdog(struct timer_list *unused)
{
- struct clocksource *cs;
u64 csnow, wdnow, cslast, wdlast, delta;
- int64_t wd_nsec, cs_nsec;
int next_cpu, reset_pending;
+ int64_t wd_nsec, cs_nsec;
+ struct clocksource *cs;
spin_lock(&watchdog_lock);
if (!watchdog_running)
@@ -206,10 +237,14 @@ static void clocksource_watchdog(struct
continue;
}
- local_irq_disable();
- csnow = cs->read(cs);
- wdnow = watchdog->read(watchdog);
- local_irq_enable();
+ if (!cs_watchdog_read(cs, &csnow, &wdnow)) {
+ /*
+ * No point to continue if the watchdog readout is
+ * unreliable.
+ */
+ __clocksource_unstable(cs);
+ continue;
+ }
/* Clocksource initialized ? */
if (!(cs->flags & CLOCK_SOURCE_WATCHDOG) ||
next prev parent reply other threads:[~2021-04-17 12:27 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-14 4:34 [PATCH v8 clocksource] Do not mark clocks unstable due to delays for v5.13 Paul E. McKenney
2021-04-14 4:35 ` [PATCH v8 clocksource 1/5] clocksource: Provide module parameters to inject delays in watchdog Paul E. McKenney
2021-04-16 20:10 ` Thomas Gleixner
2021-04-16 22:38 ` Paul E. McKenney
2021-04-14 4:35 ` [PATCH v8 clocksource 2/5] clocksource: Retry clock read if long delays detected Paul E. McKenney
2021-04-16 20:45 ` Thomas Gleixner
2021-04-17 0:25 ` Paul E. McKenney
2021-04-17 12:24 ` Thomas Gleixner [this message]
2021-04-17 22:54 ` Paul E. McKenney
2021-04-17 23:15 ` Thomas Gleixner
2021-04-17 23:40 ` Paul E. McKenney
2021-04-14 4:36 ` [PATCH v8 clocksource 3/5] clocksource: Check per-CPU clock synchronization when marked unstable Paul E. McKenney
2021-04-17 12:28 ` Thomas Gleixner
2021-04-17 23:42 ` Paul E. McKenney
2021-04-17 12:47 ` Thomas Gleixner
2021-04-17 23:51 ` Paul E. McKenney
2021-04-18 16:20 ` Paul E. McKenney
2021-04-14 4:36 ` [PATCH v8 clocksource 4/5] clocksource: Provide a module parameter to fuzz per-CPU clock checking Paul E. McKenney
2021-04-14 4:36 ` [PATCH v8 clocksource 5/5] clocksource: Limit number of CPUs checked for clock synchronization 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 \
--in-reply-to=87y2dhrte0.ffs@nanos.tec.linutronix.de \
--to=tglx@linutronix.de \
--cc=Mark.Rutland@arm.com \
--cc=ak@linux.intel.com \
--cc=clm@fb.com \
--cc=corbet@lwn.net \
--cc=john.stultz@linaro.org \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=neeraju@codeaurora.org \
--cc=paulmck@kernel.org \
--cc=sboyd@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).