* [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock
@ 2021-07-16 5:34 Sergey Senozhatsky
2021-07-17 0:24 ` Sergey Senozhatsky
0 siblings, 1 reply; 2+ messages in thread
From: Sergey Senozhatsky @ 2021-07-16 5:34 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
Cc: Suleiman Souhlal, Paul E . McKenney, kvm, linux-kernel,
Sergey Senozhatsky
When host suspends a VM it may signal the guest that its
being suspended via KVM_KVMCLOCK_CTRL ioctl, so that once
resumed guest VCPUs can discover PVCLOCK_GUEST_STOPPED bit
and touch watchdogs to update stale timeouts.
The way it's implemented is that every kvm_clock_read() calls
pvclock_clocksource_read(), which tests PVCLOCK_GUEST_STOPPED
and invokes pvclock_touch_watchdogs() when needed. This scheme
appears not to be always working as intended. For instance,
when lockdep is enabled we have the following:
<IRQ>
apic_timer_interrupt()
smp_apic_timer_interrupt()
hrtimer_interrupt()
_raw_spin_lock_irqsave()
lock_acquire()
__lock_acquire()
sched_clock_cpu()
sched_clock()
kvm_sched_clock_read()
kvm_clock_read()
pvclock_clocksource_read()
pvclock_touch_watchdogs()
Since this is VM and VCPU resume path, jiffies still maybe
be outdated here, which is often the case on my device.
pvclock_clocksource_read() clears PVCLOCK_GUEST_STOPPED,
touches watchdogs, but it uses stale jiffies: 4294740764
(for example).
Now comes in the sched tick IRQ, which invokes RCU watchdog:
<IRQ>
apic_timer_interrupt()
smp_apic_timer_interrupt()
hrtimer_interrupt()
__hrtimer_run_queues()
tick_sched_timer()
tick_sched_handle()
update_process_times()
rcu_sched_clock_irq()
At this point, however, jiffies are already updated and include
the VM suspension time (approx 80 seconds in this case): 4294819216,
but PVCLOCK_GUEST_STOPPED is already cleared and we used outdated
jiffies, so RCU watchdog concludes it's a stall.
There are probably more scenarios under which resuming VCPUs can
invoke kvm_clock_read() too early.
Both lockup and RCU watchdogs call kvm_check_and_clear_guest_paused()
from hard IRQ contexts, so we probably can remove one from
pvclock_clocksource_read() and avoid preliminary PVCLOCK_GUEST_STOPPED
handling from some random paths.
That is, since kvm_check_and_clear_guest_paused() is for watchdogs
then only watchdogs should use it.
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
arch/x86/kernel/kvmclock.c | 2 +-
arch/x86/kernel/pvclock.c | 5 -----
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index ad273e5861c1..af90b889e923 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -150,8 +150,8 @@ bool kvm_check_and_clear_guest_paused(void)
return ret;
if ((src->pvti.flags & PVCLOCK_GUEST_STOPPED) != 0) {
- src->pvti.flags &= ~PVCLOCK_GUEST_STOPPED;
pvclock_touch_watchdogs();
+ src->pvti.flags &= ~PVCLOCK_GUEST_STOPPED;
ret = true;
}
return ret;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index eda37df016f0..b176e083e543 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -77,11 +77,6 @@ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
flags = src->flags;
} while (pvclock_read_retry(src, version));
- if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
- src->flags &= ~PVCLOCK_GUEST_STOPPED;
- pvclock_touch_watchdogs();
- }
-
if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) &&
(flags & PVCLOCK_TSC_STABLE_BIT))
return ret;
--
2.32.0.402.g57bb445576-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock
2021-07-16 5:34 [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock Sergey Senozhatsky
@ 2021-07-17 0:24 ` Sergey Senozhatsky
0 siblings, 0 replies; 2+ messages in thread
From: Sergey Senozhatsky @ 2021-07-17 0:24 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Suleiman Souhlal, Paul E . McKenney,
kvm, linux-kernel
On (21/07/16 14:34), Sergey Senozhatsky wrote:
>
> <IRQ>
> apic_timer_interrupt()
> smp_apic_timer_interrupt()
> hrtimer_interrupt()
> _raw_spin_lock_irqsave()
> lock_acquire()
> __lock_acquire()
> sched_clock_cpu()
> sched_clock()
> kvm_sched_clock_read()
> kvm_clock_read()
> pvclock_clocksource_read()
> pvclock_touch_watchdogs()
>
> Since this is VM and VCPU resume path, jiffies still maybe
> be outdated here, which is often the case on my device.
> pvclock_clocksource_read() clears PVCLOCK_GUEST_STOPPED,
> touches watchdogs, but it uses stale jiffies: 4294740764
> (for example).
Hmm, on the other hand, there is probably nothing that guarantees
that the first watchdog hard IRQ we execute on a resuming VCPU is
going to see updated jiffies, it still can use stale jiffies.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-07-17 0:24 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-16 5:34 [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock Sergey Senozhatsky
2021-07-17 0:24 ` Sergey Senozhatsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.