[RFC PATCH] x86/kvm: do not touch watchdogs in pvclock

[RFC PATCH] x86/kvm: do not touch watchdogs in pvclock

[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

Re: [RFC PATCH] x86/kvm: do not touch watchdogs in pvclock

[Sergey Senozhatsky]

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.