[PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

Changes from V4:
Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
Add description of KVMCLOCK_GUEST_PAUSED ioctl to api.txt

Changes from V3:
Include CC's on patch 3
Drop clear flag ioctl and have the watchdog clear the flag when it is reset

Changes from V2:
A new kvm functions defined in kvm_para.h, the only change to pvclock is the
initial flag definition

Changes from V1:
(Thanks Marcelo)
Host code has all been moved to arch/x86/kvm/x86.c
KVM_PAUSE_GUEST was renamed to KVM_GUEST_PAUSED

When a guest kernel is stopped by the host hypervisor it can look like a soft
lockup to the guest kernel.  This false warning can mask later soft lockup
warnings which may be real.  This patch series adds a method for a host
hypervisor to communicate to a guest kernel that it is being stopped.  The
final patch in the series has the watchdog check this flag when it goes to
issue a soft lockup warning and skip the warning if the guest knows it was
stopped.

It was attempted to solve this in Qemu, but the side effects of saving and
restoring the clock and tsc for each vcpu put the wall clock of the guest behind
by the amount of time of the pause.  This forces a guest to have ntp running
in order to keep the wall clock accurate.

Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: arnd@arndb.de
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: levinsasha928@gmail.com
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org

Eric B Munson (5):
  Add flag to indicate that a vm was stopped by the host
  Add functions to check if the host has stopped the vm
  Add ioctl for KVMCLOCK_GUEST_STOPPED
  Add generic stubs for kvm stop check functions
  Add check for suspended vm in softlockup detector

 Documentation/virtual/kvm/api.txt  |   12 ++++++++++++
 arch/x86/include/asm/kvm_host.h    |    2 ++
 arch/x86/include/asm/kvm_para.h    |    1 +
 arch/x86/include/asm/pvclock-abi.h |    1 +
 arch/x86/kernel/kvmclock.c         |   21 +++++++++++++++++++++
 arch/x86/kvm/x86.c                 |   20 ++++++++++++++++++++
 include/asm-generic/kvm_para.h     |   14 ++++++++++++++
 include/linux/kvm.h                |    2 ++
 kernel/watchdog.c                  |   12 ++++++++++++
 9 files changed, 85 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/kvm_para.h

-- 
1.7.5.4

[PATCH 1/5 V5] Add flag to indicate that a vm was stopped by the host

[Eric B Munson]

This flag will be used to check if the vm was stopped by the host when a soft
lockup was detected.  The host will set the flag when it stops the guest.  On
resume, the guest will check this flag if a soft lockup is detected and skip
issuing the warning.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: arnd@arndb.de
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: levinsasha928@gmail.com
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/pvclock-abi.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/pvclock-abi.h b/arch/x86/include/asm/pvclock-abi.h
index 35f2d19..6167fd7 100644
--- a/arch/x86/include/asm/pvclock-abi.h
+++ b/arch/x86/include/asm/pvclock-abi.h
@@ -40,5 +40,6 @@ struct pvclock_wall_clock {
 } __attribute__((__packed__));
 
 #define PVCLOCK_TSC_STABLE_BIT	(1 << 0)
+#define PVCLOCK_GUEST_STOPPED	(1 << 1)
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_PVCLOCK_ABI_H */
-- 
1.7.5.4

[PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Eric B Munson]

When a host stops or suspends a VM it will set a flag to show this.  The
watchdog will use these functions to determine if a softlockup is real, or the
result of a suspended VM.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: arnd@arndb.de
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: levinsasha928@gmail.com
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/kvm_para.h |    1 +
 arch/x86/kernel/kvmclock.c      |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 734c376..e9d63a6 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -95,6 +95,7 @@ struct kvm_vcpu_pv_apf_data {
 extern void kvmclock_init(void);
 extern int kvm_register_clock(char *txt);
 
+bool kvm_check_and_clear_guest_paused(int cpu);
 
 /* This instruction is vmcall.  On non-VT architectures, it will generate a
  * trap that we will then rewrite to the appropriate instruction.
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 44842d7..f0c0599 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -22,6 +22,7 @@
 #include <asm/msr.h>
 #include <asm/apic.h>
 #include <linux/percpu.h>
+#include <linux/hardirq.h>
 
 #include <asm/x86_init.h>
 #include <asm/reboot.h>
@@ -114,6 +115,26 @@ static void kvm_get_preset_lpj(void)
 	preset_lpj = lpj;
 }
 
+bool kvm_check_and_clear_guest_paused(int cpu)
+{
+	bool ret = false;
+	struct pvclock_vcpu_time_info *src;
+
+	/*
+	 * per_cpu() is safe here because this function is only called from
+	 * timer functions where preemption is already disabled.
+	 */
+	WARN_ON(!in_atomic());
+	src = &per_cpu(hv_clock, cpu);
+	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
+		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
+		ret = true;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_check_and_clear_guest_paused);
+
 static struct clocksource kvm_clock = {
 	.name = "kvm-clock",
 	.read = kvm_clock_get_cycles,
-- 
1.7.5.4

[PATCH 3/5 V5] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Eric B Munson]

Now that we have a flag that will tell the guest it was suspended, create an
interface for that communication using a KVM ioctl.

Signed-off-by: Eric B Munson <emunson@mgebm.net>

Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: arnd@arndb.de
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: levinsasha928@gmail.com
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from V4:
 Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
 Add new ioctl description to api.txt

 Documentation/virtual/kvm/api.txt |   12 ++++++++++++
 arch/x86/include/asm/kvm_host.h   |    2 ++
 arch/x86/kvm/x86.c                |   20 ++++++++++++++++++++
 include/linux/kvm.h               |    2 ++
 4 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7945b0b..0f7dd99 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1450,6 +1450,18 @@ is supported; 2 if the processor requires all virtual machines to have
 an RMA, or 1 if the processor can use an RMA but doesn't require it,
 because it supports the Virtual RMA (VRMA) facility.
 
+4.64 KVMCLOCK_GUEST_PAUSED
+
+Capability: basic
+Architechtures: Any that implement pvclocks (currently x86 only)
+Type: vcpu ioctl
+Parameters: None
+Returns: 0 on success, -1 on error
+
+This signals to the host kernel that the specified guest is being paused by
+userspace.  The host will set a flag in the pvclock structure that is checked
+from the soft lockup watchdog.
+
 5. The kvm_run structure
 
 Application code obtains a pointer to the kvm_run structure by
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b4973f4..beb94c6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -672,6 +672,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret);
 u8 kvm_get_guest_memory_type(struct kvm_vcpu *vcpu, gfn_t gfn);
 
+int kvm_set_guest_paused(struct kvm_vcpu *vcpu);
+
 extern bool tdp_enabled;
 
 u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c38efd7..1dab5fd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3295,6 +3295,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 		goto out;
 	}
+	case KVMCLOCK_GUEST_PAUSED: {
+		r = kvm_set_guest_paused(vcpu);
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
@@ -6117,6 +6121,22 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
 }
 EXPORT_SYMBOL_GPL(kvm_task_switch);
 
+/*
+ * kvm_set_guest_paused() indicates to the guest kernel that it has been
+ * stopped by the hypervisor.  This function will be called from the host only.
+ * EINVAL is returned when the host attempts to set the flag for a guest that
+ * does not support pv clocks.
+ */
+int kvm_set_guest_paused(struct kvm_vcpu *vcpu)
+{
+	struct pvclock_vcpu_time_info *src = &vcpu->arch.hv_clock;
+	if (!vcpu->arch.time_page)
+		return -EINVAL;
+	src->flags |= PVCLOCK_GUEST_STOPPED;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_set_guest_paused);
+
 int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index c3892fc..1d1ddef 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -762,6 +762,8 @@ struct kvm_clock_data {
 #define KVM_CREATE_SPAPR_TCE	  _IOW(KVMIO,  0xa8, struct kvm_create_spapr_tce)
 /* Available with KVM_CAP_RMA */
 #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
+/* VM is being stopped by host */
+#define KVMCLOCK_GUEST_PAUSED	  _IO(KVMIO,   0xaa)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
-- 
1.7.5.4

[PATCH 4/5 V5] Add generic stubs for kvm stop check functions

[Eric B Munson]

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: arnd@arndb.de
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: levinsasha928@gmail.com
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/asm-generic/kvm_para.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/kvm_para.h

diff --git a/include/asm-generic/kvm_para.h b/include/asm-generic/kvm_para.h
new file mode 100644
index 0000000..177e1eb
--- /dev/null
+++ b/include/asm-generic/kvm_para.h
@@ -0,0 +1,14 @@
+#ifndef _ASM_GENERIC_KVM_PARA_H
+#define _ASM_GENERIC_KVM_PARA_H
+
+
+/*
+ * This function is used by architectures that support kvm to avoid issuing
+ * false soft lockup messages.
+ */
+static inline bool kvm_check_and_clear_guest_paused(int cpu)
+{
+	return false;
+}
+
+#endif
-- 
1.7.5.4

[PATCH 5/5 V5] Add check for suspended vm in softlockup detector

[Eric B Munson]

A suspended VM can cause spurious soft lockup warnings.  To avoid these, the
watchdog now checks if the kernel knows it was stopped by the host and skips
the warning if so.  When the watchdog is reset successfully, clear the guest
paused flag.

Signed-off-by: Eric B Munson <emunson@mgebm.net>
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: arnd@arndb.de
Cc: ryanh@linux.vnet.ibm.com
Cc: aliguori@us.ibm.com
Cc: mtosatti@redhat.com
Cc: jeremy.fitzhardinge@citrix.com
Cc: levinsasha928@gmail.com
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: kvm@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from V3:
 Clear the PAUSED flag when the watchdog is reset

 kernel/watchdog.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 1d7bca7..7c62919 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -25,6 +25,7 @@
 #include <linux/sysctl.h>
 
 #include <asm/irq_regs.h>
+#include <linux/kvm_para.h>
 #include <linux/perf_event.h>
 
 int watchdog_enabled = 1;
@@ -280,6 +281,9 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 			__this_cpu_write(softlockup_touch_sync, false);
 			sched_clock_tick();
 		}
+
+		/* Clear the guest paused flag on watchdog reset */
+		kvm_check_and_clear_guest_paused(smp_processor_id());
 		__touch_watchdog();
 		return HRTIMER_RESTART;
 	}
@@ -292,6 +296,14 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	 */
 	duration = is_softlockup(touch_ts);
 	if (unlikely(duration)) {
+		/*
+		 * If a virtual machine is stopped by the host it can look to
+		 * the watchdog like a soft lockup, check to see if the host
+		 * stopped the vm before we issue the warning
+		 */
+		if (kvm_check_and_clear_guest_paused(smp_processor_id()))
+			return HRTIMER_RESTART;
+
 		/* only warn once */
 		if (__this_cpu_read(soft_watchdog_warn) == true)
 			return HRTIMER_RESTART;
-- 
1.7.5.4

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Avi Kivity]

On 12/05/2011 10:19 PM, Eric B Munson wrote:
> When a host stops or suspends a VM it will set a flag to show this.  The
> watchdog will use these functions to determine if a softlockup is real, or the
> result of a suspended VM.
>  
> +bool kvm_check_and_clear_guest_paused(int cpu)
> +{
> +	bool ret = false;
> +	struct pvclock_vcpu_time_info *src;
> +
> +	/*
> +	 * per_cpu() is safe here because this function is only called from
> +	 * timer functions where preemption is already disabled.
> +	 */
> +	WARN_ON(!in_atomic());
> +	src = &per_cpu(hv_clock, cpu);

__get_cpu_var(); drop the cpu argument

> +	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
> +		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);

Isn't this racy?  Between reading and writing src->flags, we can exit to
the hypervisor and add/remove new flags.  The write then overrides those
new flags.

> +		ret = true;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(kvm_check_and_clear_guest_paused);
> +
>  static struct clocksource kvm_clock = {
>  	.name = "kvm-clock",
>  	.read = kvm_clock_get_cycles,


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/5 V5] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Avi Kivity]

On 12/05/2011 10:19 PM, Eric B Munson wrote:
> Now that we have a flag that will tell the guest it was suspended, create an
> interface for that communication using a KVM ioctl.
>
> @@ -3295,6 +3295,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  
>  		goto out;
>  	}
> +	case KVMCLOCK_GUEST_PAUSED: {
> +		r = kvm_set_guest_paused(vcpu);
> +		break;
> +	}
>  	default:
>  		r = -EINVAL;
>  	}
>

You could also do this purely from userspace by reading the kvmclock msr
and updating it.  However, it's better to do this in the kernel to avoid
distributing responsibility for kvmclock across too many cooks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 4/5 V5] Add generic stubs for kvm stop check functions

[Avi Kivity]

On 12/05/2011 10:19 PM, Eric B Munson wrote:
>
> diff --git a/include/asm-generic/kvm_para.h b/include/asm-generic/kvm_para.h
> new file mode 100644
> index 0000000..177e1eb
> --- /dev/null
> +++ b/include/asm-generic/kvm_para.h
> @@ -0,0 +1,14 @@
> +#ifndef _ASM_GENERIC_KVM_PARA_H
> +#define _ASM_GENERIC_KVM_PARA_H
> +
> +
> +/*
> + * This function is used by architectures that support kvm to avoid issuing
> + * false soft lockup messages.
> + */
> +static inline bool kvm_check_and_clear_guest_paused(int cpu)
> +{
> +	return false;
> +}
> +
> +#endif

Please fold into patch 2.

What about arch/{ia64,s390,powerpc}/include/asm/kvm_para.h?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/5 V5] Add check for suspended vm in softlockup detector

[Avi Kivity]

On 12/05/2011 10:19 PM, Eric B Munson wrote:
> A suspended VM can cause spurious soft lockup warnings.  To avoid these, the
> watchdog now checks if the kernel knows it was stopped by the host and skips
> the warning if so.  When the watchdog is reset successfully, clear the guest
> paused flag.
>
>  int watchdog_enabled = 1;
> @@ -280,6 +281,9 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
>  			__this_cpu_write(softlockup_touch_sync, false);
>  			sched_clock_tick();
>  		}
> +
> +		/* Clear the guest paused flag on watchdog reset */
> +		kvm_check_and_clear_guest_paused(smp_processor_id());
>  		__touch_watchdog();
>  		return HRTIMER_RESTART;
>  	}

Won't build if !CONFIG_KVMCLOCK


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/05/2011 10:18 PM, Eric B Munson wrote:
> Changes from V4:
> Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
> Add description of KVMCLOCK_GUEST_PAUSED ioctl to api.txt
>
> Changes from V3:
> Include CC's on patch 3
> Drop clear flag ioctl and have the watchdog clear the flag when it is reset
>
> Changes from V2:
> A new kvm functions defined in kvm_para.h, the only change to pvclock is the
> initial flag definition
>
> Changes from V1:
> (Thanks Marcelo)
> Host code has all been moved to arch/x86/kvm/x86.c
> KVM_PAUSE_GUEST was renamed to KVM_GUEST_PAUSED
>
> When a guest kernel is stopped by the host hypervisor it can look like a soft
> lockup to the guest kernel.  This false warning can mask later soft lockup
> warnings which may be real.  This patch series adds a method for a host
> hypervisor to communicate to a guest kernel that it is being stopped.  The
> final patch in the series has the watchdog check this flag when it goes to
> issue a soft lockup warning and skip the warning if the guest knows it was
> stopped.
>
> It was attempted to solve this in Qemu, but the side effects of saving and
> restoring the clock and tsc for each vcpu put the wall clock of the guest behind
> by the amount of time of the pause.  This forces a guest to have ntp running
> in order to keep the wall clock accurate.

Having this controlled from userspace means it doesn't work for SIGSTOP
or for long scheduling delays.  What about doing this automatically
based on preempt notifiers?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Amit Shah]

On (Mon) 05 Dec 2011 [15:18:59], Eric B Munson wrote:
> When a guest kernel is stopped by the host hypervisor it can look like a soft
> lockup to the guest kernel.  This false warning can mask later soft lockup
> warnings which may be real.  This patch series adds a method for a host
> hypervisor to communicate to a guest kernel that it is being stopped.  The
> final patch in the series has the watchdog check this flag when it goes to
> issue a soft lockup warning and skip the warning if the guest knows it was
> stopped.

Guest S4 would need similar treatment, and I think the code in the two
approaches can be shared.  Just something to consider.

		Amit

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

On Wed, 07 Dec 2011, Avi Kivity wrote:

> On 12/05/2011 10:18 PM, Eric B Munson wrote:
> > Changes from V4:
> > Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
> > Add description of KVMCLOCK_GUEST_PAUSED ioctl to api.txt
> >
> > Changes from V3:
> > Include CC's on patch 3
> > Drop clear flag ioctl and have the watchdog clear the flag when it is reset
> >
> > Changes from V2:
> > A new kvm functions defined in kvm_para.h, the only change to pvclock is the
> > initial flag definition
> >
> > Changes from V1:
> > (Thanks Marcelo)
> > Host code has all been moved to arch/x86/kvm/x86.c
> > KVM_PAUSE_GUEST was renamed to KVM_GUEST_PAUSED
> >
> > When a guest kernel is stopped by the host hypervisor it can look like a soft
> > lockup to the guest kernel.  This false warning can mask later soft lockup
> > warnings which may be real.  This patch series adds a method for a host
> > hypervisor to communicate to a guest kernel that it is being stopped.  The
> > final patch in the series has the watchdog check this flag when it goes to
> > issue a soft lockup warning and skip the warning if the guest knows it was
> > stopped.
> >
> > It was attempted to solve this in Qemu, but the side effects of saving and
> > restoring the clock and tsc for each vcpu put the wall clock of the guest behind
> > by the amount of time of the pause.  This forces a guest to have ntp running
> > in order to keep the wall clock accurate.
> 
> Having this controlled from userspace means it doesn't work for SIGSTOP
> or for long scheduling delays.  What about doing this automatically
> based on preempt notifiers?
> 
> 
> -- 
> error compiling committee.c: too many arguments to function
> 
My concern for preempt notifiers is masking real soft lockup warnings.  If the
flag is set every time the vm is preempted, it becomes more likely that we will
mask real warnings.  The ioctl was choosen because it sets the flag only when
the guest is being paused deliberately.

AFAIK, SIGSTOP is not a supported way to stop a qemu vm so a soft lockup
warning would be working as designed there.  If that isn't the case, or if it
ever changes, we could always install a signal handler for SIGCONT that set the
flag before resuming the vm.

Scheduling delays are also beyond the scope of this problem and I see the soft
lockup warning as appropriate in that case.

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Wed, 07 Dec 2011, Avi Kivity wrote:

> On 12/05/2011 10:19 PM, Eric B Munson wrote:
> > When a host stops or suspends a VM it will set a flag to show this.  The
> > watchdog will use these functions to determine if a softlockup is real, or the
> > result of a suspended VM.
> >  
> > +bool kvm_check_and_clear_guest_paused(int cpu)
> > +{
> > +	bool ret = false;
> > +	struct pvclock_vcpu_time_info *src;
> > +
> > +	/*
> > +	 * per_cpu() is safe here because this function is only called from
> > +	 * timer functions where preemption is already disabled.
> > +	 */
> > +	WARN_ON(!in_atomic());
> > +	src = &per_cpu(hv_clock, cpu);
> 
> __get_cpu_var(); drop the cpu argument
> 

Will change for V6.

> > +	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
> > +		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
> 
> Isn't this racy?  Between reading and writing src->flags, we can exit to
> the hypervisor and add/remove new flags.  The write then overrides those
> new flags.
> 

If I understand (please correct me if this is wrong) because this is only
called from the watchdog, which disables preemption, we should be protected
from something else writing to these flags.

> > +		ret = true;
> > +	}
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_check_and_clear_guest_paused);
> > +
> >  static struct clocksource kvm_clock = {
> >  	.name = "kvm-clock",
> >  	.read = kvm_clock_get_cycles,
> 
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/5 V5] Add ioctl for KVMCLOCK_GUEST_STOPPED

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

On Wed, 07 Dec 2011, Avi Kivity wrote:

> On 12/05/2011 10:19 PM, Eric B Munson wrote:
> > Now that we have a flag that will tell the guest it was suspended, create an
> > interface for that communication using a KVM ioctl.
> >
> > @@ -3295,6 +3295,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >  
> >  		goto out;
> >  	}
> > +	case KVMCLOCK_GUEST_PAUSED: {
> > +		r = kvm_set_guest_paused(vcpu);
> > +		break;
> > +	}
> >  	default:
> >  		r = -EINVAL;
> >  	}
> >
> 
> You could also do this purely from userspace by reading the kvmclock msr
> and updating it.  However, it's better to do this in the kernel to avoid
> distributing responsibility for kvmclock across too many cooks.
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

As stated in the cover letter, per Marcelo's suggestion I already tried this
method and it resulted in the guest wall clock skewing by the amount of time
the guest was suspended.  This places a new burden on the guest to be running
ntpd or equivslent to keep the wall clock correct.

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5 V5] Add generic stubs for kvm stop check functions

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On Wed, 07 Dec 2011, Avi Kivity wrote:

> On 12/05/2011 10:19 PM, Eric B Munson wrote:
> >
> > diff --git a/include/asm-generic/kvm_para.h b/include/asm-generic/kvm_para.h
> > new file mode 100644
> > index 0000000..177e1eb
> > --- /dev/null
> > +++ b/include/asm-generic/kvm_para.h
> > @@ -0,0 +1,14 @@
> > +#ifndef _ASM_GENERIC_KVM_PARA_H
> > +#define _ASM_GENERIC_KVM_PARA_H
> > +
> > +
> > +/*
> > + * This function is used by architectures that support kvm to avoid issuing
> > + * false soft lockup messages.
> > + */
> > +static inline bool kvm_check_and_clear_guest_paused(int cpu)
> > +{
> > +	return false;
> > +}
> > +
> > +#endif
> 
> Please fold into patch 2.

Will do for V6.

> 
> What about arch/{ia64,s390,powerpc}/include/asm/kvm_para.h?
> 

I will add the above to those as well.

> -- 
> error compiling committee.c: too many arguments to function
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Dor Laor]

On 12/07/2011 04:41 PM, Avi Kivity wrote:
> On 12/05/2011 10:18 PM, Eric B Munson wrote:
>> Changes from V4:
>> Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
>> Add description of KVMCLOCK_GUEST_PAUSED ioctl to api.txt
>>
>> Changes from V3:
>> Include CC's on patch 3
>> Drop clear flag ioctl and have the watchdog clear the flag when it is reset
>>
>> Changes from V2:
>> A new kvm functions defined in kvm_para.h, the only change to pvclock is the
>> initial flag definition
>>
>> Changes from V1:
>> (Thanks Marcelo)
>> Host code has all been moved to arch/x86/kvm/x86.c
>> KVM_PAUSE_GUEST was renamed to KVM_GUEST_PAUSED
>>
>> When a guest kernel is stopped by the host hypervisor it can look like a soft
>> lockup to the guest kernel.  This false warning can mask later soft lockup
>> warnings which may be real.  This patch series adds a method for a host
>> hypervisor to communicate to a guest kernel that it is being stopped.  The
>> final patch in the series has the watchdog check this flag when it goes to
>> issue a soft lockup warning and skip the warning if the guest knows it was
>> stopped.
>>
>> It was attempted to solve this in Qemu, but the side effects of saving and
>> restoring the clock and tsc for each vcpu put the wall clock of the guest behind
>> by the amount of time of the pause.  This forces a guest to have ntp running
>> in order to keep the wall clock accurate.

Guests need to run NTP regardless, not only the virtualization layer add 
some skew, the physical world is not that perfect.
btw: traditional NTP client won't sync the time automatically if the 
diff is > 0.5%.

>
> Having this controlled from userspace means it doesn't work for SIGSTOP
> or for long scheduling delays.  What about doing this automatically
> based on preempt notifiers?
>
>

Isn't it solved by steal time?

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Marcelo Tosatti]

On Thu, Dec 08, 2011 at 10:23:10AM -0500, Eric B Munson wrote:
> On Wed, 07 Dec 2011, Avi Kivity wrote:
> 
> > On 12/05/2011 10:19 PM, Eric B Munson wrote:
> > > When a host stops or suspends a VM it will set a flag to show this.  The
> > > watchdog will use these functions to determine if a softlockup is real, or the
> > > result of a suspended VM.
> > >  
> > > +bool kvm_check_and_clear_guest_paused(int cpu)
> > > +{
> > > +	bool ret = false;
> > > +	struct pvclock_vcpu_time_info *src;
> > > +
> > > +	/*
> > > +	 * per_cpu() is safe here because this function is only called from
> > > +	 * timer functions where preemption is already disabled.
> > > +	 */
> > > +	WARN_ON(!in_atomic());
> > > +	src = &per_cpu(hv_clock, cpu);
> > 
> > __get_cpu_var(); drop the cpu argument
> > 
> 
> Will change for V6.
> 
> > > +	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
> > > +		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
> > 
> > Isn't this racy?  Between reading and writing src->flags, we can exit to
> > the hypervisor and add/remove new flags.  The write then overrides those
> > new flags.
> > 
> 
> If I understand (please correct me if this is wrong) because this is only
> called from the watchdog, which disables preemption, we should be protected
> from something else writing to these flags.

The host can write, but in that case race is harmless.

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti]

On Wed, Dec 07, 2011 at 04:41:17PM +0200, Avi Kivity wrote:
> On 12/05/2011 10:18 PM, Eric B Munson wrote:
> > Changes from V4:
> > Rename KVM_GUEST_PAUSED to KVMCLOCK_GUEST_PAUSED
> > Add description of KVMCLOCK_GUEST_PAUSED ioctl to api.txt
> >
> > Changes from V3:
> > Include CC's on patch 3
> > Drop clear flag ioctl and have the watchdog clear the flag when it is reset
> >
> > Changes from V2:
> > A new kvm functions defined in kvm_para.h, the only change to pvclock is the
> > initial flag definition
> >
> > Changes from V1:
> > (Thanks Marcelo)
> > Host code has all been moved to arch/x86/kvm/x86.c
> > KVM_PAUSE_GUEST was renamed to KVM_GUEST_PAUSED
> >
> > When a guest kernel is stopped by the host hypervisor it can look like a soft
> > lockup to the guest kernel.  This false warning can mask later soft lockup
> > warnings which may be real.  This patch series adds a method for a host
> > hypervisor to communicate to a guest kernel that it is being stopped.  The
> > final patch in the series has the watchdog check this flag when it goes to
> > issue a soft lockup warning and skip the warning if the guest knows it was
> > stopped.
> >
> > It was attempted to solve this in Qemu, but the side effects of saving and
> > restoring the clock and tsc for each vcpu put the wall clock of the guest behind
> > by the amount of time of the pause.  This forces a guest to have ntp running
> > in order to keep the wall clock accurate.
> 
> Having this controlled from userspace means it doesn't work for SIGSTOP
> or for long scheduling delays.  What about doing this automatically
> based on preempt notifiers?

Long scheduling delays should be considered hangups from the guest
perspective.

About SIGSTOP, that is a corner case. Unsure if its even properly supported 
by QEMU.

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti]

On Sun, Dec 11, 2011 at 02:40:58PM +0200, Dor Laor wrote:
> >>When a guest kernel is stopped by the host hypervisor it can look like a soft
> >>lockup to the guest kernel.  This false warning can mask later soft lockup
> >>warnings which may be real.  This patch series adds a method for a host
> >>hypervisor to communicate to a guest kernel that it is being stopped.  The
> >>final patch in the series has the watchdog check this flag when it goes to
> >>issue a soft lockup warning and skip the warning if the guest knows it was
> >>stopped.
> >>
> >>It was attempted to solve this in Qemu, but the side effects of saving and
> >>restoring the clock and tsc for each vcpu put the wall clock of the guest behind
> >>by the amount of time of the pause.  This forces a guest to have ntp running
> >>in order to keep the wall clock accurate.
> 
> Guests need to run NTP regardless, not only the virtualization layer
> add some skew, the physical world is not that perfect.
> btw: traditional NTP client won't sync the time automatically if the
> diff is > 0.5%.
> 
> >
> >Having this controlled from userspace means it doesn't work for SIGSTOP
> >or for long scheduling delays.  What about doing this automatically
> >based on preempt notifiers?
> >
> >
> 
> Isn't it solved by steal time?

No.

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Avi Kivity]

On 12/14/2011 02:11 PM, Marcelo Tosatti wrote:
> On Thu, Dec 08, 2011 at 10:23:10AM -0500, Eric B Munson wrote:
> > On Wed, 07 Dec 2011, Avi Kivity wrote:
> > 
> > > On 12/05/2011 10:19 PM, Eric B Munson wrote:
> > > > When a host stops or suspends a VM it will set a flag to show this.  The
> > > > watchdog will use these functions to determine if a softlockup is real, or the
> > > > result of a suspended VM.
> > > >  
> > > > +bool kvm_check_and_clear_guest_paused(int cpu)
> > > > +{
> > > > +	bool ret = false;
> > > > +	struct pvclock_vcpu_time_info *src;
> > > > +
> > > > +	/*
> > > > +	 * per_cpu() is safe here because this function is only called from
> > > > +	 * timer functions where preemption is already disabled.
> > > > +	 */
> > > > +	WARN_ON(!in_atomic());
> > > > +	src = &per_cpu(hv_clock, cpu);
> > > 
> > > __get_cpu_var(); drop the cpu argument
> > > 
> > 
> > Will change for V6.
> > 
> > > > +	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
> > > > +		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
> > > 
> > > Isn't this racy?  Between reading and writing src->flags, we can exit to
> > > the hypervisor and add/remove new flags.  The write then overrides those
> > > new flags.
> > > 
> > 
> > If I understand (please correct me if this is wrong) because this is only
> > called from the watchdog, which disables preemption, we should be protected
> > from something else writing to these flags.
>
> The host can write, but in that case race is harmless.

Why is it harmless?  You don't know what's in those other flags.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/14/2011 02:16 PM, Marcelo Tosatti wrote:
> > Having this controlled from userspace means it doesn't work for SIGSTOP
> > or for long scheduling delays.  What about doing this automatically
> > based on preempt notifiers?
>
> Long scheduling delays should be considered hangups from the guest
> perspective.

Why?  To the guest it looks like slow hardware, but it will interpret it
as a softlockup.

> About SIGSTOP, that is a corner case. Unsure if its even properly supported 
> by QEMU.

It works from my experience.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Anthony Liguori]

On 12/14/2011 08:39 AM, Avi Kivity wrote:
> On 12/14/2011 02:16 PM, Marcelo Tosatti wrote:
>>> Having this controlled from userspace means it doesn't work for SIGSTOP
>>> or for long scheduling delays.  What about doing this automatically
>>> based on preempt notifiers?
>>
>> Long scheduling delays should be considered hangups from the guest
>> perspective.
>
> Why?  To the guest it looks like slow hardware, but it will interpret it
> as a softlockup.
>
>> About SIGSTOP, that is a corner case. Unsure if its even properly supported
>> by QEMU.
>
> It works from my experience.

We don't adjust vm_clock's offset on SIGSTOP/SIGCONT so while it may appear to 
work okay, QEMU isn't exhibiting the behavior it is supposed to.

Regards,

Anthony Liguori

>

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Wed, 14 Dec 2011, Avi Kivity wrote:

> On 12/14/2011 02:16 PM, Marcelo Tosatti wrote:
> > > Having this controlled from userspace means it doesn't work for SIGSTOP
> > > or for long scheduling delays.  What about doing this automatically
> > > based on preempt notifiers?
> >
> > Long scheduling delays should be considered hangups from the guest
> > perspective.
> 
> Why?  To the guest it looks like slow hardware, but it will interpret it
> as a softlockup.
> 
> > About SIGSTOP, that is a corner case. Unsure if its even properly supported 
> > by QEMU.
> 
> It works from my experience.
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

And if the soft lockup warning is a concern here, we can have a SIGCONT handler
set the flag the same way qemu stop/cont does.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/14/2011 04:49 PM, Anthony Liguori wrote:
>>
>>> About SIGSTOP, that is a corner case. Unsure if its even properly
>>> supported
>>> by QEMU.
>>
>> It works from my experience.
>
>
> We don't adjust vm_clock's offset on SIGSTOP/SIGCONT so while it may
> appear to work okay, QEMU isn't exhibiting the behavior it is supposed
> to.

Should we trap and handle SIGCONT?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/08/2011 05:19 PM, Eric B Munson wrote:
> > error compiling committee.c: too many arguments to function
> > 
> My concern for preempt notifiers is masking real soft lockup warnings.  If the
> flag is set every time the vm is preempted, it becomes more likely that we will
> mask real warnings.  The ioctl was choosen because it sets the flag only when
> the guest is being paused deliberately.
>
> AFAIK, SIGSTOP is not a supported way to stop a qemu vm so a soft lockup

This is a kvm patch, not a qemu patch.  In general we try to address
general requirements, not just those that are specific to one userspace,
even if it is near to our hearts.

> warning would be working as designed there.  If that isn't the case, or if it
> ever changes, we could always install a signal handler for SIGCONT that set the
> flag before resuming the vm.
>
> Scheduling delays are also beyond the scope of this problem and I see the soft
> lockup warning as appropriate in that case.

I don't think you should see a guest softlockup if the host is
overloaded.  Nor should you see it due to a long live migration pause,
or STOP/CONT.  You should see a guest softlockup if it is spinning due
to a guest bug, and not for any other reason.

I think we need a mix of the ioctl (for STOP/CONT and voluntary pauses,
like live migration or qemu stop/cont) and preempt notifiers (together
with a check for TASK_RUNNING/TASK_UNINTERRUPTIBLE, for scheduling or
swap delays)

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

On Wed, 14 Dec 2011, Avi Kivity wrote:

> On 12/14/2011 02:11 PM, Marcelo Tosatti wrote:
> > On Thu, Dec 08, 2011 at 10:23:10AM -0500, Eric B Munson wrote:
> > > On Wed, 07 Dec 2011, Avi Kivity wrote:
> > > 
> > > > On 12/05/2011 10:19 PM, Eric B Munson wrote:
> > > > > When a host stops or suspends a VM it will set a flag to show this.  The
> > > > > watchdog will use these functions to determine if a softlockup is real, or the
> > > > > result of a suspended VM.
> > > > >  
> > > > > +bool kvm_check_and_clear_guest_paused(int cpu)
> > > > > +{
> > > > > +	bool ret = false;
> > > > > +	struct pvclock_vcpu_time_info *src;
> > > > > +
> > > > > +	/*
> > > > > +	 * per_cpu() is safe here because this function is only called from
> > > > > +	 * timer functions where preemption is already disabled.
> > > > > +	 */
> > > > > +	WARN_ON(!in_atomic());
> > > > > +	src = &per_cpu(hv_clock, cpu);
> > > > 
> > > > __get_cpu_var(); drop the cpu argument
> > > > 
> > > 
> > > Will change for V6.
> > > 
> > > > > +	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
> > > > > +		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
> > > > 
> > > > Isn't this racy?  Between reading and writing src->flags, we can exit to
> > > > the hypervisor and add/remove new flags.  The write then overrides those
> > > > new flags.
> > > > 
> > > 
> > > If I understand (please correct me if this is wrong) because this is only
> > > called from the watchdog, which disables preemption, we should be protected
> > > from something else writing to these flags.
> >
> > The host can write, but in that case race is harmless.
> 
> Why is it harmless?  You don't know what's in those other flags.
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

Currently there is only one other flag in this byte (PVCLOCK_TSC_STABLE_BIT)
and it isset once in kvmclock_init().  It is highly unlikely that the vm will
be stopped during this init and have the flag clobbered.  After the tsc stable
bit is written in the init, the field is read only outside of the guest paused
code.

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Avi Kivity]

On 12/14/2011 07:11 PM, Eric B Munson wrote:
> > > > 
> > > > If I understand (please correct me if this is wrong) because this is only
> > > > called from the watchdog, which disables preemption, we should be protected
> > > > from something else writing to these flags.
> > >
> > > The host can write, but in that case race is harmless.
> > 
> > Why is it harmless?  You don't know what's in those other flags.
> > 
> > -- 
> > error compiling committee.c: too many arguments to function
> > 
>
> Currently there is only one other flag in this byte (PVCLOCK_TSC_STABLE_BIT)
> and it isset once in kvmclock_init().  It is highly unlikely that the vm will
> be stopped during this init and have the flag clobbered.  After the tsc stable
> bit is written in the init, the field is read only outside of the guest paused
> code.

Let's not lay minefields for us later to step into.  Making this a
cpu-local atomic (irq safe but not smp safe) is simple and easy,
somehting like __this_cpu_and().

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

On Wed, 14 Dec 2011, Avi Kivity wrote:

> On 12/14/2011 07:11 PM, Eric B Munson wrote:
> > > > > 
> > > > > If I understand (please correct me if this is wrong) because this is only
> > > > > called from the watchdog, which disables preemption, we should be protected
> > > > > from something else writing to these flags.
> > > >
> > > > The host can write, but in that case race is harmless.
> > > 
> > > Why is it harmless?  You don't know what's in those other flags.
> > > 
> > > -- 
> > > error compiling committee.c: too many arguments to function
> > > 
> >
> > Currently there is only one other flag in this byte (PVCLOCK_TSC_STABLE_BIT)
> > and it isset once in kvmclock_init().  It is highly unlikely that the vm will
> > be stopped during this init and have the flag clobbered.  After the tsc stable
> > bit is written in the init, the field is read only outside of the guest paused
> > code.
> 
> Let's not lay minefields for us later to step into.  Making this a
> cpu-local atomic (irq safe but not smp safe) is simple and easy,
> somehting like __this_cpu_and().
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

Fair enough, I will add this to V7.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

On Wed, 14 Dec 2011, Avi Kivity wrote:

> On 12/08/2011 05:19 PM, Eric B Munson wrote:
> > > error compiling committee.c: too many arguments to function
> > > 
> > My concern for preempt notifiers is masking real soft lockup warnings.  If the
> > flag is set every time the vm is preempted, it becomes more likely that we will
> > mask real warnings.  The ioctl was choosen because it sets the flag only when
> > the guest is being paused deliberately.
> >
> > AFAIK, SIGSTOP is not a supported way to stop a qemu vm so a soft lockup
> 
> This is a kvm patch, not a qemu patch.  In general we try to address
> general requirements, not just those that are specific to one userspace,
> even if it is near to our hearts.
> 
> > warning would be working as designed there.  If that isn't the case, or if it
> > ever changes, we could always install a signal handler for SIGCONT that set the
> > flag before resuming the vm.
> >
> > Scheduling delays are also beyond the scope of this problem and I see the soft
> > lockup warning as appropriate in that case.
> 
> I don't think you should see a guest softlockup if the host is
> overloaded.  Nor should you see it due to a long live migration pause,
> or STOP/CONT.  You should see a guest softlockup if it is spinning due
> to a guest bug, and not for any other reason.
> 
> I think we need a mix of the ioctl (for STOP/CONT and voluntary pauses,
> like live migration or qemu stop/cont) and preempt notifiers (together
> with a check for TASK_RUNNING/TASK_UNINTERRUPTIBLE, for scheduling or
> swap delays)
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

Do you want the preemption notifier work in before you will consider merging
this set, or can that be a follow on?

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti]

On Wed, Dec 14, 2011 at 04:39:56PM +0200, Avi Kivity wrote:
> On 12/14/2011 02:16 PM, Marcelo Tosatti wrote:
> > > Having this controlled from userspace means it doesn't work for SIGSTOP
> > > or for long scheduling delays.  What about doing this automatically
> > > based on preempt notifiers?
> >
> > Long scheduling delays should be considered hangups from the guest
> > perspective.
> 
> Why?  To the guest it looks like slow hardware, but it will interpret it
> as a softlockup.

Slow enough that progress of the watchdog thread is unable to keep up
with timer interrupt processing. This is considered a hang and
should be reported.

> > About SIGSTOP, that is a corner case. Unsure if its even properly supported 
> > by QEMU.
> 
> It works from my experience.

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/14/2011 07:58 PM, Eric B Munson wrote:
> > I don't think you should see a guest softlockup if the host is
> > overloaded.  Nor should you see it due to a long live migration pause,
> > or STOP/CONT.  You should see a guest softlockup if it is spinning due
> > to a guest bug, and not for any other reason.
> > 
> > I think we need a mix of the ioctl (for STOP/CONT and voluntary pauses,
> > like live migration or qemu stop/cont) and preempt notifiers (together
> > with a check for TASK_RUNNING/TASK_UNINTERRUPTIBLE, for scheduling or
> > swap delays)
> > 
>
> Do you want the preemption notifier work in before you will consider merging
> this set, or can that be a follow on?

Yes please.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/14/2011 08:21 PM, Marcelo Tosatti wrote:
> On Wed, Dec 14, 2011 at 04:39:56PM +0200, Avi Kivity wrote:
> > On 12/14/2011 02:16 PM, Marcelo Tosatti wrote:
> > > > Having this controlled from userspace means it doesn't work for SIGSTOP
> > > > or for long scheduling delays.  What about doing this automatically
> > > > based on preempt notifiers?
> > >
> > > Long scheduling delays should be considered hangups from the guest
> > > perspective.
> > 
> > Why?  To the guest it looks like slow hardware, but it will interpret it
> > as a softlockup.
>
> Slow enough that progress of the watchdog thread is unable to keep up
> with timer interrupt processing. This is considered a hang and
> should be reported.

It's not a guest hang though!

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/08/2011 01:34 PM, Amit Shah wrote:
> On (Mon) 05 Dec 2011 [15:18:59], Eric B Munson wrote:
> > When a guest kernel is stopped by the host hypervisor it can look like a soft
> > lockup to the guest kernel.  This false warning can mask later soft lockup
> > warnings which may be real.  This patch series adds a method for a host
> > hypervisor to communicate to a guest kernel that it is being stopped.  The
> > final patch in the series has the watchdog check this flag when it goes to
> > issue a soft lockup warning and skip the warning if the guest knows it was
> > stopped.
>
> Guest S4 would need similar treatment, and I think the code in the two
> approaches can be shared.  Just something to consider.
>

Why does S4 need any treatment?  The guest is aware that it's sleeping,
unlike the other cases treated here.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Eric B Munson]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Thu, 15 Dec 2011, Avi Kivity wrote:

> On 12/14/2011 07:58 PM, Eric B Munson wrote:
> > > I don't think you should see a guest softlockup if the host is
> > > overloaded.  Nor should you see it due to a long live migration pause,
> > > or STOP/CONT.  You should see a guest softlockup if it is spinning due
> > > to a guest bug, and not for any other reason.
> > > 
> > > I think we need a mix of the ioctl (for STOP/CONT and voluntary pauses,
> > > like live migration or qemu stop/cont) and preempt notifiers (together
> > > with a check for TASK_RUNNING/TASK_UNINTERRUPTIBLE, for scheduling or
> > > swap delays)
> > > 
> >
> > Do you want the preemption notifier work in before you will consider merging
> > this set, or can that be a follow on?
> 
> Yes please.
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

I am working on V7 to incorporate the __this_cpu_and suggestion, would you
consider that for inclusion and we can continue discussing the need for the
preemption notification work?  I think that having a guest complain when the
host is so loaded that the guest watchdog threads can't make any progress is
desirable behavior, it may be the only notification that an admin gets that a
particular host is over loaded.

Eric

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti]

On Thu, Dec 15, 2011 at 12:21:16PM +0200, Avi Kivity wrote:
> On 12/14/2011 08:21 PM, Marcelo Tosatti wrote:
> > On Wed, Dec 14, 2011 at 04:39:56PM +0200, Avi Kivity wrote:
> > > On 12/14/2011 02:16 PM, Marcelo Tosatti wrote:
> > > > > Having this controlled from userspace means it doesn't work for SIGSTOP
> > > > > or for long scheduling delays.  What about doing this automatically
> > > > > based on preempt notifiers?
> > > >
> > > > Long scheduling delays should be considered hangups from the guest
> > > > perspective.
> > > 
> > > Why?  To the guest it looks like slow hardware, but it will interpret it
> > > as a softlockup.
> >
> > Slow enough that progress of the watchdog thread is unable to keep up
> > with timer interrupt processing. This is considered a hang and
> > should be reported.
> 
> It's not a guest hang though!

No, but your host system is in such a load state that for the sake of
system usability you better print out a warning message.

I don't see the advantage of preempt notifiers over the simple, paravirt
solution proposed? Note kvmclock is already paravirt.

What do you want to be done in preempt notifiers? Measure what to
consider setting this flag?

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Amit Shah]

On (Thu) 15 Dec 2011 [13:55:15], Avi Kivity wrote:
> On 12/08/2011 01:34 PM, Amit Shah wrote:
> > On (Mon) 05 Dec 2011 [15:18:59], Eric B Munson wrote:
> > > When a guest kernel is stopped by the host hypervisor it can look like a soft
> > > lockup to the guest kernel.  This false warning can mask later soft lockup
> > > warnings which may be real.  This patch series adds a method for a host
> > > hypervisor to communicate to a guest kernel that it is being stopped.  The
> > > final patch in the series has the watchdog check this flag when it goes to
> > > issue a soft lockup warning and skip the warning if the guest knows it was
> > > stopped.
> >
> > Guest S4 would need similar treatment, and I think the code in the two
> > approaches can be shared.  Just something to consider.
> >
> 
> Why does S4 need any treatment?  The guest is aware that it's sleeping,
> unlike the other cases treated here.

Er, right.

S4 needs some treatment, though, as resume after s4 doesn't work with
kvmclock enabled.  I didn't realise this series was only handling the
soft lockup case.

		Amit

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/16/2011 11:31 AM, Marcelo Tosatti wrote:
> > >
> > > Slow enough that progress of the watchdog thread is unable to keep up
> > > with timer interrupt processing. This is considered a hang and
> > > should be reported.
> > 
> > It's not a guest hang though!
>
> No, but your host system is in such a load state that for the sake of
> system usability you better print out a warning message.

What's the point in printing it in the guest?  The guest can't observe
host conditions.

> I don't see the advantage of preempt notifiers over the simple, paravirt
> solution proposed? Note kvmclock is already paravirt.

Right.

> What do you want to be done in preempt notifiers? Measure what to
> consider setting this flag?

Preemption while TASK_RUNNING or TASK_UNINTERRUPTIBLE.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/19/2011 02:52 PM, Amit Shah wrote:
> On (Thu) 15 Dec 2011 [13:55:15], Avi Kivity wrote:
> > On 12/08/2011 01:34 PM, Amit Shah wrote:
> > > On (Mon) 05 Dec 2011 [15:18:59], Eric B Munson wrote:
> > > > When a guest kernel is stopped by the host hypervisor it can look like a soft
> > > > lockup to the guest kernel.  This false warning can mask later soft lockup
> > > > warnings which may be real.  This patch series adds a method for a host
> > > > hypervisor to communicate to a guest kernel that it is being stopped.  The
> > > > final patch in the series has the watchdog check this flag when it goes to
> > > > issue a soft lockup warning and skip the warning if the guest knows it was
> > > > stopped.
> > >
> > > Guest S4 would need similar treatment, and I think the code in the two
> > > approaches can be shared.  Just something to consider.
> > >
> > 
> > Why does S4 need any treatment?  The guest is aware that it's sleeping,
> > unlike the other cases treated here.
>
> Er, right.
>
> S4 needs some treatment, though, as resume after s4 doesn't work with
> kvmclock enabled.  I didn't realise this series was only handling the
> soft lockup case.
>

What's the issue there?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/15/2011 08:53 PM, Eric B Munson wrote:
> I am working on V7 to incorporate the __this_cpu_and suggestion, would you
> consider that for inclusion and we can continue discussing the need for the
> preemption notification work?  

Okay.

> I think that having a guest complain when the
> host is so loaded that the guest watchdog threads can't make any progress is
> desirable behavior, it may be the only notification that an admin gets that a
> particular host is over loaded.

It's the wrong admin, consider a cloud scenario where the host admin and
guest admin are different people.

Notification on host overload (to initiate migration, say) is a good
idea, but it would be a different mechanism.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti]

On Mon, Dec 19, 2011 at 02:59:10PM +0200, Avi Kivity wrote:
> On 12/16/2011 11:31 AM, Marcelo Tosatti wrote:
> > > >
> > > > Slow enough that progress of the watchdog thread is unable to keep up
> > > > with timer interrupt processing. This is considered a hang and
> > > > should be reported.
> > > 
> > > It's not a guest hang though!
> >
> > No, but your host system is in such a load state that for the sake of
> > system usability you better print out a warning message.
> 
> What's the point in printing it in the guest?  The guest can't observe
> host conditions.
>
> > I don't see the advantage of preempt notifiers over the simple, paravirt
> > solution proposed? Note kvmclock is already paravirt.
> 
> Right.
> 
> > What do you want to be done in preempt notifiers? Measure what to
> > consider setting this flag?
> 
> Preemption while TASK_RUNNING or TASK_UNINTERRUPTIBLE.

Maybe it is good (not sure), need to look into schedstats and think of
cases that would break legitimate guest hangs. And it probably also
affects the position of clearing the flag on the guest side as its 
currently done in Eric's patchset.

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti]

On Mon, Dec 19, 2011 at 02:11:08PM -0200, Marcelo Tosatti wrote:
> On Mon, Dec 19, 2011 at 02:59:10PM +0200, Avi Kivity wrote:
> > On 12/16/2011 11:31 AM, Marcelo Tosatti wrote:
> > > > >
> > > > > Slow enough that progress of the watchdog thread is unable to keep up
> > > > > with timer interrupt processing. This is considered a hang and
> > > > > should be reported.
> > > > 
> > > > It's not a guest hang though!
> > >
> > > No, but your host system is in such a load state that for the sake of
> > > system usability you better print out a warning message.
> > 
> > What's the point in printing it in the guest?  The guest can't observe
> > host conditions.
> >
> > > I don't see the advantage of preempt notifiers over the simple, paravirt
> > > solution proposed? Note kvmclock is already paravirt.
> > 
> > Right.
> > 
> > > What do you want to be done in preempt notifiers? Measure what to
> > > consider setting this flag?
> > 
> > Preemption while TASK_RUNNING or TASK_UNINTERRUPTIBLE.
> 
> Maybe it is good (not sure), need to look into schedstats and think of
> cases that would break legitimate guest hangs. And it probably also
> affects the position of clearing the flag on the guest side as its 
> currently done in Eric's patchset.

Is the task going to be TASK_UNINTERRUPTIBLE with SIGSTOP? Don't think
so. 

Note "Preemption while TASK_RUNNING or TASK_UNINTERRUPTIBLE" is not
functional for guest-paused-via-QEMU-monitor case.

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Amit Shah]

On (Mon) 19 Dec 2011 [14:59:36], Avi Kivity wrote:
> On 12/19/2011 02:52 PM, Amit Shah wrote:

(snip)

> > S4 needs some treatment, though, as resume after s4 doesn't work with
> > kvmclock enabled.  I didn't realise this series was only handling the
> > soft lockup case.
> >
> 
> What's the issue there?

Not sure yet; after resume, the VM freezes while bringing the 2nd vcpu
up.  Maybe it's just sitting there spinning, maybe the delta needs
adjusting.  Haven't poked at it yet.

		Amit

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Dor Laor]

On 12/19/2011 07:59 PM, Amit Shah wrote:
> On (Mon) 19 Dec 2011 [14:59:36], Avi Kivity wrote:
>> On 12/19/2011 02:52 PM, Amit Shah wrote:
>
> (snip)
>
>>> S4 needs some treatment, though, as resume after s4 doesn't work with
>>> kvmclock enabled.  I didn't realise this series was only handling the
>>> soft lockup case.
>>>
>>
>> What's the issue there?
>
> Not sure yet; after resume, the VM freezes while bringing the 2nd vcpu
> up.  Maybe it's just sitting there spinning, maybe the delta needs
> adjusting.  Haven't poked at it yet.

Bug 694801 - Guest fail to resume from S4 if guest using kvmclock

Re: [PATCH 0/5 V5] Avoid soft lockup message when KVM is stopped by host

[Avi Kivity]

On 12/19/2011 07:50 PM, Marcelo Tosatti wrote:
> > 
> > Maybe it is good (not sure), need to look into schedstats and think of
> > cases that would break legitimate guest hangs. And it probably also
> > affects the position of clearing the flag on the guest side as its 
> > currently done in Eric's patchset.
>
> Is the task going to be TASK_UNINTERRUPTIBLE with SIGSTOP? Don't think
> so. 

No.

> Note "Preemption while TASK_RUNNING or TASK_UNINTERRUPTIBLE" is not
> functional for guest-paused-via-QEMU-monitor case.

Right, it's a different case.  Note SIGCONT can be trapped, so it's
similar to monitor cont.

-- 
error compiling committee.c: too many arguments to function