| Message ID | 20250107042202.2554063-2-suleiman@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: x86: Include host suspended time in steal time. | expand |
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@google.com> > --- > 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. > + } > + > + 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. > + > return kvm_arch_pm_notifier(kvm, state); > } > > -- > 2.47.1.613.gc27f4b7a9f-goog >
On Wed, Jan 8, 2025 at 12:27 AM Sean Christopherson <seanjc@google.com> 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@google.com> > > --- > > 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
Hi Suleiman, kernel test robot noticed the following build warnings: [auto build test WARNING on kvm/queue] [also build test WARNING on kvm/next linus/master v6.13-rc6 next-20250107] [cannot apply to kvm/linux-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Suleiman-Souhlal/kvm-Introduce-kvm_total_suspend_ns/20250107-122819 base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue patch link: https://lore.kernel.org/r/20250107042202.2554063-2-suleiman%40google.com patch subject: [PATCH v3 1/3] kvm: Introduce kvm_total_suspend_ns(). config: s390-randconfig-001-20250108 (https://download.01.org/0day-ci/archive/20250108/202501081504.AE4wDCL9-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250108/202501081504.AE4wDCL9-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501081504.AE4wDCL9-lkp@intel.com/ All warnings (new ones prefixed by >>): >> arch/s390/kvm/../../../virt/kvm/kvm_main.c:897:12: warning: 'last_suspend' defined but not used [-Wunused-variable] 897 | static u64 last_suspend; | ^~~~~~~~~~~~ vim +/last_suspend +897 arch/s390/kvm/../../../virt/kvm/kvm_main.c 896 > 897 static u64 last_suspend; 898 static u64 total_suspend_ns; 899
Hi Suleiman, kernel test robot noticed the following build warnings: [auto build test WARNING on kvm/queue] [also build test WARNING on kvm/next linus/master v6.13-rc6 next-20250107] [cannot apply to kvm/linux-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Suleiman-Souhlal/kvm-Introduce-kvm_total_suspend_ns/20250107-122819 base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue patch link: https://lore.kernel.org/r/20250107042202.2554063-2-suleiman%40google.com patch subject: [PATCH v3 1/3] kvm: Introduce kvm_total_suspend_ns(). config: loongarch-randconfig-002-20250108 (https://download.01.org/0day-ci/archive/20250108/202501081616.nFwj9jxx-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250108/202501081616.nFwj9jxx-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501081616.nFwj9jxx-lkp@intel.com/ All warnings (new ones prefixed by >>): >> arch/loongarch/kvm/../../../virt/kvm/kvm_main.c:897:12: warning: 'last_suspend' defined but not used [-Wunused-variable] 897 | static u64 last_suspend; | ^~~~~~~~~~~~ vim +/last_suspend +897 arch/loongarch/kvm/../../../virt/kvm/kvm_main.c 896 > 897 static u64 last_suspend; 898 static u64 total_suspend_ns; 899
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; +} + + #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; + } + + 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; + return kvm_arch_pm_notifier(kvm, state); }
It returns the cumulative nanoseconds that the host has been suspended. It is intended to be used for reporting host suspend time to the guest. Change-Id: I8f644c9fbdb2c48d2c99dd9efaa5c85a83a14c2a Signed-off-by: Suleiman Souhlal <suleiman@google.com> --- include/linux/kvm_host.h | 2 ++ virt/kvm/kvm_main.c | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+)