On Mon, Apr 15, 2024, Marcelo Tosatti wrote:
> On Mon, Apr 08, 2024 at 10:16:24AM -0700, Sean Christopherson wrote:
> > On Fri, Apr 05, 2024, Paul E. McKenney wrote:
> > > Beyond a certain point, we have no choice. How long should RCU let
> > > a CPU run with preemption disabled before complaining? We choose 21
> > > seconds in mainline and some distros choose 60 seconds. Android chooses
> > > 20 milliseconds for synchronize_rcu_expedited() grace periods.
> >
> > Issuing a warning based on an arbitrary time limit is wildly different than using
> > an arbitrary time window to make functional decisions. My objection to the "assume
> > the CPU will enter a quiescent state if it exited a KVM guest in the last second"
> > is that there are plenty of scenarios where that assumption falls apart, i.e. where
> > _that_ physical CPU will not re-enter the guest.
> >
> > Off the top of my head:
> >
> > - If the vCPU is migrated to a different physical CPU (pCPU), the *old* pCPU
> > will get false positives, and the *new* pCPU will get false negatives (though
> > the false negatives aren't all that problematic since the pCPU will enter a
> > quiescent state on the next VM-Enter.
> >
> > - If the vCPU halts, in which case KVM will schedule out the vCPU/task, i.e.
> > won't re-enter the guest. And so the pCPU will get false positives until the
> > vCPU gets a wake event or the 1 second window expires.
> >
> > - If the VM terminates, the pCPU will get false positives until the 1 second
> > window expires.
> >
> > The false positives are solvable problems, by hooking vcpu_put() to reset
> > kvm_last_guest_exit. And to help with the false negatives when a vCPU task is
> > scheduled in on a different pCPU, KVM would hook vcpu_load().
>
> Hi Sean,
>
> So this should deal with it? (untested, don't apply...).
Not entirely. As I belatedly noted, hooking vcpu_put() doesn't handle the case
where the vCPU is preempted, i.e. kvm_sched_out() would also need to zero out
kvm_last_guest_exit to avoid a false positive. Going through the scheduler will
note the CPU is quiescent for the current grace period, but after that RCU will
still see a non-zero kvm_last_guest_exit even though the vCPU task isn't actively
running.
And snapshotting the VM-Exit time will get false negatives when the vCPU is about
to run, but for whatever reason has kvm_last_guest_exit=0, e.g. if a vCPU was
preempted and/or migrated to a different pCPU.
I don't understand the motivation for keeping the kvm_last_guest_exit logic. My
understanding is that RCU already has a timeout to avoid stalling RCU. I don't
see what is gained by effectively duplicating that timeout for KVM. Why not have
KVM provide a "this task is in KVM_RUN" flag, and then let the existing timeout
handle the (hopefully rare) case where KVM doesn't "immediately" re-enter the guest?
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 48f31dcd318a..be90d83d631a 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -477,6 +477,16 @@ static __always_inline void guest_state_enter_irqoff(void)
> lockdep_hardirqs_on(CALLER_ADDR0);
> }
>
> +DECLARE_PER_CPU(unsigned long, kvm_last_guest_exit);
> +
> +/*
> + * Returns time (jiffies) for the last guest exit in current cpu
> + */
> +static inline unsigned long guest_exit_last_time(void)
> +{
> + return this_cpu_read(kvm_last_guest_exit);
> +}
> +
> /*
> * Exit guest context and exit an RCU extended quiescent state.
> *
> @@ -488,6 +498,9 @@ static __always_inline void guest_state_enter_irqoff(void)
> static __always_inline void guest_context_exit_irqoff(void)
> {
> context_tracking_guest_exit();
> +
> + /* Keeps track of last guest exit */
> + this_cpu_write(kvm_last_guest_exit, jiffies);
> }
>
> /*
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index fb49c2a60200..231d0e4d2cf1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -110,6 +110,9 @@ static struct kmem_cache *kvm_vcpu_cache;
> static __read_mostly struct preempt_ops kvm_preempt_ops;
> static DEFINE_PER_CPU(struct kvm_vcpu *, kvm_running_vcpu);
>
> +DEFINE_PER_CPU(unsigned long, kvm_last_guest_exit);
> +EXPORT_SYMBOL_GPL(kvm_last_guest_exit);
> +
> struct dentry *kvm_debugfs_dir;
> EXPORT_SYMBOL_GPL(kvm_debugfs_dir);
>
> @@ -210,6 +213,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
> int cpu = get_cpu();
>
> __this_cpu_write(kvm_running_vcpu, vcpu);
> + __this_cpu_write(kvm_last_guest_exit, 0);
> preempt_notifier_register(&vcpu->preempt_notifier);
> kvm_arch_vcpu_load(vcpu, cpu);
> put_cpu();
> @@ -222,6 +226,7 @@ void vcpu_put(struct kvm_vcpu *vcpu)
> kvm_arch_vcpu_put(vcpu);
> preempt_notifier_unregister(&vcpu->preempt_notifier);
> __this_cpu_write(kvm_running_vcpu, NULL);
> + __this_cpu_write(kvm_last_guest_exit, 0);
> preempt_enable();
> }
> EXPORT_SYMBOL_GPL(vcpu_put);
>
