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

From: Petr Mladek <pmladek@suse.com>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] watchdog: revert cleanup handling of false positives
Date: Tue, 18 May 2021 17:36:45 +0200	[thread overview]
Message-ID: <YKPfDQoN5hToB9nk@alley> (raw)
In-Reply-To: <20210517140612.222750-1-senozhatsky@chromium.org>

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