Re: [PATCH v3 1/3] kvm: Introduce kvm_total_suspend_ns().

[Suleiman Souhlal]

On Wed, Jan 8, 2025 at 12:27 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> KVM: for the scope.
>
> On Tue, Jan 07, 2025, Suleiman Souhlal wrote:
> > It returns the cumulative nanoseconds that the host has been suspended.
>
> Please write changelogs that are standalone.  "It returns ..." is completely
> nonsensical without the context of the shortlog.
>
> > It is intended to be used for reporting host suspend time to the guest.
> >
> > Change-Id: I8f644c9fbdb2c48d2c99dd9efaa5c85a83a14c2a
>
> Drop gerrit's metadata before posting.
>
> > Signed-off-by: Suleiman Souhlal <suleiman@xxxxxxxxxx>
> > ---
> >  include/linux/kvm_host.h |  2 ++
> >  virt/kvm/kvm_main.c      | 26 ++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 401439bb21e3e6..cf926168b30820 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -2553,4 +2553,6 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
> >                                   struct kvm_pre_fault_memory *range);
> >  #endif
> >
> > +u64 kvm_total_suspend_ns(void);
> > +
> >  #endif
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index de2c11dae23163..d5ae237df76d0d 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -889,13 +889,39 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
> >
> >  #endif /* CONFIG_KVM_GENERIC_MMU_NOTIFIER */
> >
> > +static u64 last_suspend;
> > +static u64 total_suspend_ns;
> > +
> > +u64 kvm_total_suspend_ns(void)
> > +{
> > +     return total_suspend_ns;
> > +}
> > +
> > +
>
> Extra whitespace.
>
> >  #ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
> > +static int kvm_pm_notifier(struct kvm *kvm, unsigned long state)
> > +{
> > +     switch (state) {
> > +     case PM_HIBERNATION_PREPARE:
> > +     case PM_SUSPEND_PREPARE:
> > +             last_suspend = ktime_get_boottime_ns();
> > +     case PM_POST_HIBERNATION:
> > +     case PM_POST_SUSPEND:
> > +             total_suspend_ns += ktime_get_boottime_ns() - last_suspend;
>
> This is broken.  As should be quite clear from the function parameters, this is
> a per-VM notifier.  While clobbering "last_suspend" is relatively benign,
> accumulating into "total_suspend_ns" for every VM will cause the "total" suspend
> time to be wildly inaccurate if there are multiple VMs.

Good catch. Thanks for spotting that.

>
> > +     }
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> >  static int kvm_pm_notifier_call(struct notifier_block *bl,
> >                               unsigned long state,
> >                               void *unused)
> >  {
> >       struct kvm *kvm = container_of(bl, struct kvm, pm_notifier);
> >
> > +     if (kvm_pm_notifier(kvm, state) != NOTIFY_DONE)
> > +             return NOTIFY_BAD;
>
> This is ridiculous on multiple fronts.  There's zero reason for kvm_pm_notifier()
> to return a value, it always succeeds.  I also see zero reason to put this code
> in common KVM.  It's 100% an x86-only concept, and x86 is the only architecture
> that implements kvm_arch_pm_notifier().  So just shove the logic into x86's
> implementation.

The feedback I thought I got in v1 was that this could be useful for
other architectures too.
But given that the current series only implements it for x86, I guess
it's fair for it to be out of common KVM for now.
I will move it back into x86's implementation.

Thanks for the very quick reviews.
-- Suleiman