[PATCH] watchdog: revert cleanup handling of false positives

[PATCH] watchdog: revert cleanup handling of false positives

[Sergey Senozhatsky]

This reverts commit 9bf3bc949f8aeefeacea4b1198db833b722a8e27.

I can reproduce the case when resumed VCPU starts to execute
is_softlockup() with PVCLOCK_GUEST_STOPPED set on this VCPU:

	watchdog_timer_fn()
	{
		...

		kvm_check_and_clear_guest_paused();

		...

		duration = is_softlockup(touch_ts, period_ts);
		if (unlikely(duration)) {
			....
		}
	}

Which means that guest VCPU has been suspended between
kvm_check_and_clear_guest_paused() and is_softlockup(),
and jiffies (clock) thus shifted forward.

VCPU can be preempted anywhere, so we want to do
kvm_check_and_clear_guest_paused() at the very last
moment: when we are in the soft-lockup branch but
then we figure out that it's false positive and we
need to bail out.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 kernel/watchdog.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 7c397907d0e9..8cf0678378d2 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -376,14 +376,7 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	/* .. and repeat */
 	hrtimer_forward_now(hrtimer, ns_to_ktime(sample_period));
 
-	/*
-	 * If a virtual machine is stopped by the host it can look to
-	 * the watchdog like a soft lockup. Check to see if the host
-	 * stopped the vm before we process the timestamps.
-	 */
-	kvm_check_and_clear_guest_paused();
-
-	/* Reset the interval when touched by known problematic code. */
+	/* Reset the interval when touched externally by a known slow code. */
 	if (period_ts == SOFTLOCKUP_DELAY_REPORT) {
 		if (unlikely(__this_cpu_read(softlockup_touch_sync))) {
 			/*
@@ -394,7 +387,10 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 			sched_clock_tick();
 		}
 
+		/* Clear the guest paused flag on watchdog reset */
+		kvm_check_and_clear_guest_paused();
 		update_report_ts();
+
 		return HRTIMER_RESTART;
 	}
 
@@ -406,6 +402,14 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	 */
 	duration = is_softlockup(touch_ts, period_ts);
 	if (unlikely(duration)) {
+		/*
+		 * If a virtual machine is stopped by the host it can look to
+		 * the watchdog like a soft lockup, check to see if the host
+		 * stopped the vm before we issue the warning
+		 */
+		if (kvm_check_and_clear_guest_paused())
+			return HRTIMER_RESTART;
+
 		/*
 		 * Prevent multiple soft-lockup reports if one cpu is already
 		 * engaged in dumping all cpu back traces.
-- 
2.31.1.751.gd2f1c929bd-goog

Re: [PATCH] watchdog: revert cleanup handling of false positives

[Petr Mladek]

On Mon 2021-05-17 23:06:12, Sergey Senozhatsky wrote:
> This reverts commit 9bf3bc949f8aeefeacea4b1198db833b722a8e27.
> 
> I can reproduce the case when resumed VCPU starts to execute
> is_softlockup() with PVCLOCK_GUEST_STOPPED set on this VCPU:
> 
> 	watchdog_timer_fn()
> 	{
> 		...
> 
> 		kvm_check_and_clear_guest_paused();
> 
> 		...
> 
> 		duration = is_softlockup(touch_ts, period_ts);
> 		if (unlikely(duration)) {
> 			....
> 		}
> 	}
> 
> Which means that guest VCPU has been suspended between
> kvm_check_and_clear_guest_paused() and is_softlockup(),
> and jiffies (clock) thus shifted forward.

Are jiffies really updated here?
watchdog_timer_fn() should be called with interrupts disabled.

kvm_check_and_clear_guest_paused() calls
touch_softlockup_watchdog_sync(). It sets softlockup_touch_sync
when jiffies have to be updated explicitely.

Well, I am not 100% sure.

Anyway, the code does not guarantee in which order and how
many times are touch_ts and current jiffies read. And touch_ts
might be updated also from NMI.

I have a patch that mfixes the ordering and makes sure that
the same value is used in all checks. But I still need to double
check some things and write proper commit message.

I would prefer to fix it properly. The original code was
not good either.

Best Regards,
Petr

[PATCH] watchdog: Reliable handling of timestamps

[Petr Mladek]

The commit 9bf3bc949f8aeefeacea4b ("watchdog: cleanup handling of false
positives") tried to handle a virtual host stopped by the host a more
straightforward and cleaner way.

But it introduced a risk of false softlockup reports. The virtual host
might be stopped at any time, for example between
kvm_check_and_clear_guest_paused() and is_softlockup().
As a result, is_softlockup() might read the updated jiffies
are detects softlockup.

A solution might be to put back kvm_check_and_clear_guest_paused()
after is_softlockup() and detect it. But it would put back
the cycle that complicates the logic.

In fact, the handling of all the timestamps is not reliable.
The code does not guarantee when and how many times the timestamps
are read. For example, "period_ts" might be touched anytime also
from NMI and re-read in is_softlockup(). It works just by chance.

Fix all the problems by making the code even more explicit.

1. Make sure that "now" and "period_ts" timestamps are read only
   once. They might be changed at anytime by NMI or when the virtual
   guest is stopped by the host. Note that "now" timestamp does
   this implicitly because "jiffies" is marked volatile.

2. "now" time must be read first. The state of "period_ts" will decide
   whether it will be used or the period will get restarted.

3. kvm_check_and_clear_guest_paused() must be called before reading
   "period_ts". It touches the variable when the guest was
   stopped.

As a result, "now" timestamp is used only when the watchdog was
not touched and the guest not stopped in the meantime. "period_ts"
is restarted in all other situations.

Fixes: 9bf3bc949f8aeefeacea4b ("watchdog: cleanup handling of false positives")
Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/watchdog.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 7c397907d0e9..92d3bcc5a5e0 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -302,10 +302,10 @@ void touch_softlockup_watchdog_sync(void)
 	__this_cpu_write(watchdog_report_ts, SOFTLOCKUP_DELAY_REPORT);
 }
 
-static int is_softlockup(unsigned long touch_ts, unsigned long period_ts)
+static int is_softlockup(unsigned long touch_ts,
+			 unsigned long period_ts,
+			 unsigned long now)
 {
-	unsigned long now = get_timestamp();
-
 	if ((watchdog_enabled & SOFT_WATCHDOG_ENABLED) && watchdog_thresh){
 		/* Warn about unreasonable delays. */
 		if (time_after(now, period_ts + get_softlockup_thresh()))
@@ -353,8 +353,7 @@ static int softlockup_fn(void *data)
 /* watchdog kicker functions */
 static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 {
-	unsigned long touch_ts = __this_cpu_read(watchdog_touch_ts);
-	unsigned long period_ts = __this_cpu_read(watchdog_report_ts);
+	unsigned long touch_ts, period_ts, now;
 	struct pt_regs *regs = get_irq_regs();
 	int duration;
 	int softlockup_all_cpu_backtrace = sysctl_softlockup_all_cpu_backtrace;
@@ -376,12 +375,23 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	/* .. and repeat */
 	hrtimer_forward_now(hrtimer, ns_to_ktime(sample_period));
 
+	/*
+	 * Read the current timestamp first. It might become invalid anytime
+	 * when a virtual machine is stopped by the host or when the watchog
+	 * is touched from NMI.
+	 */
+	now = get_timestamp();
 	/*
 	 * If a virtual machine is stopped by the host it can look to
-	 * the watchdog like a soft lockup. Check to see if the host
-	 * stopped the vm before we process the timestamps.
+	 * the watchdog like a soft lockup. This function touches the watchdog.
 	 */
 	kvm_check_and_clear_guest_paused();
+	/*
+	 * The stored timestamp is comparable with @now only when not touched.
+	 * It might get touched anytime from NMI. Make sure that is_softlockup()
+	 * uses the same (valid) value.
+	 */
+	period_ts = READ_ONCE(*this_cpu_ptr(&watchdog_report_ts));
 
 	/* Reset the interval when touched by known problematic code. */
 	if (period_ts == SOFTLOCKUP_DELAY_REPORT) {
@@ -398,13 +408,9 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 		return HRTIMER_RESTART;
 	}
 
-	/* check for a softlockup
-	 * This is done by making sure a high priority task is
-	 * being scheduled.  The task touches the watchdog to
-	 * indicate it is getting cpu time.  If it hasn't then
-	 * this is a good indication some task is hogging the cpu
-	 */
-	duration = is_softlockup(touch_ts, period_ts);
+	/* Check for a softlockup. */
+	touch_ts = __this_cpu_read(watchdog_touch_ts);
+	duration = is_softlockup(touch_ts, period_ts, now);
 	if (unlikely(duration)) {
 		/*
 		 * Prevent multiple soft-lockup reports if one cpu is already
-- 
2.26.2

Re: [PATCH] watchdog: Reliable handling of timestamps

[Petr Mladek]

On Wed 2021-05-19 13:43:34, Petr Mladek wrote:
> The commit 9bf3bc949f8aeefeacea4b ("watchdog: cleanup handling of false
> positives") tried to handle a virtual host stopped by the host a more
> straightforward and cleaner way.
> 
> But it introduced a risk of false softlockup reports. The virtual host
> might be stopped at any time, for example between
> kvm_check_and_clear_guest_paused() and is_softlockup().
> As a result, is_softlockup() might read the updated jiffies
> are detects softlockup.
> 
> Fix all the problems by making the code even more explicit.

This is my preferred solution. It makes it clear when the various
values are read and various situations handled.

In the original code, kvm_check_and_clear_guest_paused() was handled
partially in the given and partially in the next watchdog_timer_fn()
invocation.

It would be great to push this patch or at least Sergey's revert
for 5.13.

Best Regards,
Petr

Re: [PATCH] watchdog: Reliable handling of timestamps

[Sergey Senozhatsky]

On (21/05/19 14:01), Petr Mladek wrote:
> This is my preferred solution. It makes it clear when the various
> values are read and various situations handled.

Looks good to me.

Re: [PATCH] watchdog: Reliable handling of timestamps

[Sergey Senozhatsky]

On (21/05/19 13:43), Petr Mladek wrote:
> 
> The commit 9bf3bc949f8aeefeacea4b ("watchdog: cleanup handling of false
> positives") tried to handle a virtual host stopped by the host a more
> straightforward and cleaner way.
> 
> But it introduced a risk of false softlockup reports. The virtual host
> might be stopped at any time, for example between
> kvm_check_and_clear_guest_paused() and is_softlockup().
> As a result, is_softlockup() might read the updated jiffies
> are detects softlockup.
> 
> A solution might be to put back kvm_check_and_clear_guest_paused()
> after is_softlockup() and detect it. But it would put back
> the cycle that complicates the logic.
> 
> In fact, the handling of all the timestamps is not reliable.
> The code does not guarantee when and how many times the timestamps
> are read. For example, "period_ts" might be touched anytime also
> from NMI and re-read in is_softlockup(). It works just by chance.
> 
> Fix all the problems by making the code even more explicit.
> 
> 1. Make sure that "now" and "period_ts" timestamps are read only
>    once. They might be changed at anytime by NMI or when the virtual
>    guest is stopped by the host. Note that "now" timestamp does
>    this implicitly because "jiffies" is marked volatile.
> 
> 2. "now" time must be read first. The state of "period_ts" will decide
>    whether it will be used or the period will get restarted.
> 
> 3. kvm_check_and_clear_guest_paused() must be called before reading
>    "period_ts". It touches the variable when the guest was
>    stopped.
> 
> As a result, "now" timestamp is used only when the watchdog was
> not touched and the guest not stopped in the meantime. "period_ts"
> is restarted in all other situations.
> 
> Fixes: 9bf3bc949f8aeefeacea4b ("watchdog: cleanup handling of false positives")
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH] watchdog: revert cleanup handling of false positives

[Sergey Senozhatsky]

On (21/05/18 17:36), Petr Mladek wrote:
> On Mon 2021-05-17 23:06:12, Sergey Senozhatsky wrote:
> > This reverts commit 9bf3bc949f8aeefeacea4b1198db833b722a8e27.
> > 
> > I can reproduce the case when resumed VCPU starts to execute
> > is_softlockup() with PVCLOCK_GUEST_STOPPED set on this VCPU:
> > 
> > 	watchdog_timer_fn()
> > 	{
> > 		...
> > 
> > 		kvm_check_and_clear_guest_paused();
> > 
> > 		...
> > 
> > 		duration = is_softlockup(touch_ts, period_ts);
> > 		if (unlikely(duration)) {
> > 			....
> > 		}
> > 	}
> > 
> > Which means that guest VCPU has been suspended between
> > kvm_check_and_clear_guest_paused() and is_softlockup(),
> > and jiffies (clock) thus shifted forward.
> 
> Are jiffies really updated here?

I guess so. Why not?

VCPUs are not brought up simultaneously, it's up to the host that
schedules them. So, for instance, when we resume VCPU-3 and it
discovers this_cpu PVCLOCK_GUEST_STOPPED the VCPU-0 can already
be resumed, up and running, adding ticks to jiffies.

Am I missing the point of your question?