[Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Marcelo Tosatti]

Regression fix tested with fio and ping-pong migration.

[Qemu-devel] [patch 1/3] kvmclock: Ensure time in migration never goes backward

[Marcelo Tosatti]

[-- Attachment #1: kvmclock-ensure-time-in-mig.patch --]
[-- Type: text/plain, Size: 3014 bytes --]

From: Alexander Graf <agraf@suse.de>

When we migrate we ask the kernel about its current belief on what the guest
time would be. However, I've seen cases where the kvmclock guest structure
indicates a time more recent than the kvm returned time.

To make sure we never go backwards, calculate what the guest would have seen as time at the point of migration and use that value instead of the kernel returned one when it's more recent.  
This bases the view of the kvmclock after migration on the 
same foundation in host as well as guest.

Signed-off-by: Alexander Graf <agraf@suse.de>
Cc: qemu-stable@nongnu.org
Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

---
 hw/i386/kvm/clock.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Index: qemu/hw/i386/kvm/clock.c
===================================================================
--- qemu.orig/hw/i386/kvm/clock.c	2014-08-15 15:43:30.440191065 -0300
+++ qemu/hw/i386/kvm/clock.c	2014-08-15 15:56:31.754875109 -0300
@@ -14,6 +14,7 @@
  */
 
 #include "qemu-common.h"
+#include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "hw/sysbus.h"
@@ -34,6 +35,47 @@
     bool clock_valid;
 } KVMClockState;
 
+struct pvclock_vcpu_time_info {
+    uint32_t   version;
+    uint32_t   pad0;
+    uint64_t   tsc_timestamp;
+    uint64_t   system_time;
+    uint32_t   tsc_to_system_mul;
+    int8_t     tsc_shift;
+    uint8_t    flags;
+    uint8_t    pad[2];
+} __attribute__((__packed__)); /* 32 bytes */
+
+static uint64_t kvmclock_current_nsec(KVMClockState *s)
+{
+    CPUState *cpu = first_cpu;
+    CPUX86State *env = cpu->env_ptr;
+    hwaddr kvmclock_struct_pa = env->system_time_msr & ~1ULL;
+    uint64_t migration_tsc = env->tsc;
+    struct pvclock_vcpu_time_info time;
+    uint64_t delta;
+    uint64_t nsec_lo;
+    uint64_t nsec_hi;
+    uint64_t nsec;
+
+    if (!(env->system_time_msr & 1ULL)) {
+        /* KVM clock not active */
+        return 0;
+    }
+
+    cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
+
+    delta = migration_tsc - time.tsc_timestamp;
+    if (time.tsc_shift < 0) {
+        delta >>= -time.tsc_shift;
+    } else {
+        delta <<= time.tsc_shift;
+    }
+
+    mulu64(&nsec_lo, &nsec_hi, delta, time.tsc_to_system_mul);
+    nsec = (nsec_lo >> 32) | (nsec_hi << 32);
+    return nsec + time.system_time;
+}
 
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
@@ -45,9 +87,15 @@
 
     if (running) {
         struct kvm_clock_data data;
+        uint64_t time_at_migration = kvmclock_current_nsec(s);
 
         s->clock_valid = false;
 
+	/* We can't rely on the migrated clock value, just discard it */
+	if (time_at_migration) {
+	        s->clock = time_at_migration;
+	}
+
         data.clock = s->clock;
         data.flags = 0;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);

Re: [Qemu-devel] [patch 1/3] kvmclock: Ensure time in migration never goes backward

[Marcelo Tosatti]

Ping?

On Fri, Sep 05, 2014 at 10:52:45AM -0300, Marcelo Tosatti wrote:
> From: Alexander Graf <agraf@suse.de>
> 
> When we migrate we ask the kernel about its current belief on what the guest
> time would be. However, I've seen cases where the kvmclock guest structure
> indicates a time more recent than the kvm returned time.
> 
> To make sure we never go backwards, calculate what the guest would have seen as time at the point of migration and use that value instead of the kernel returned one when it's more recent.  
> This bases the view of the kvmclock after migration on the 
> same foundation in host as well as guest.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [patch 1/3] kvmclock: Ensure time in migration never goes backward

[Alexander Graf]

Who are you pinging?


Alex

On 09.09.14 17:12, Marcelo Tosatti wrote:
> Ping?
> 
> On Fri, Sep 05, 2014 at 10:52:45AM -0300, Marcelo Tosatti wrote:
>> From: Alexander Graf <agraf@suse.de>
>>
>> When we migrate we ask the kernel about its current belief on what the guest
>> time would be. However, I've seen cases where the kvmclock guest structure
>> indicates a time more recent than the kvm returned time.
>>
>> To make sure we never go backwards, calculate what the guest would have seen as time at the point of migration and use that value instead of the kernel returned one when it's more recent.  
>> This bases the view of the kvmclock after migration on the 
>> same foundation in host as well as guest.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Cc: qemu-stable@nongnu.org
>> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> 

Re: [Qemu-devel] [patch 1/3] kvmclock: Ensure time in migration never goes backward

[Marcelo Tosatti]

On Tue, Sep 09, 2014 at 11:16:45PM +0200, Alexander Graf wrote:
> Who are you pinging?

Paolo.

[Qemu-devel] [patch 2/3] Introduce cpu_clean_all_dirty

[Marcelo Tosatti]

[-- Attachment #1: introduce-cpu-clean-all-dirty --]
[-- Type: text/plain, Size: 2588 bytes --]

Introduce cpu_clean_all_dirty, to force subsequent cpu_synchronize_all_states
to read in-kernel register state.

Cc: qemu-stable@nongnu.org
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
 cpus.c                |    9 +++++++++
 include/sysemu/cpus.h |    1 +
 include/sysemu/kvm.h  |    8 ++++++++
 kvm-all.c             |    5 +++++
 4 files changed, 23 insertions(+)

Index: qemu/cpus.c
===================================================================
--- qemu.orig/cpus.c	2014-09-04 15:38:03.304783188 -0300
+++ qemu/cpus.c	2014-09-04 15:38:31.643709731 -0300
@@ -589,6 +589,15 @@
     }
 }
 
+void cpu_clean_all_dirty(void)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        cpu_clean_state(cpu);
+    }
+}
+
 static int do_vm_stop(RunState state)
 {
     int ret = 0;
Index: qemu/include/sysemu/cpus.h
===================================================================
--- qemu.orig/include/sysemu/cpus.h	2014-08-22 18:04:23.129789186 -0300
+++ qemu/include/sysemu/cpus.h	2014-09-04 15:38:31.710709558 -0300
@@ -10,6 +10,7 @@
 void cpu_synchronize_all_states(void);
 void cpu_synchronize_all_post_reset(void);
 void cpu_synchronize_all_post_init(void);
+void cpu_clean_all_dirty(void);
 
 void qtest_clock_warp(int64_t dest);
 
Index: qemu/include/sysemu/kvm.h
===================================================================
--- qemu.orig/include/sysemu/kvm.h	2014-08-22 18:04:23.129789186 -0300
+++ qemu/include/sysemu/kvm.h	2014-09-04 15:38:31.722709526 -0300
@@ -348,6 +348,7 @@
 void kvm_cpu_synchronize_state(CPUState *cpu);
 void kvm_cpu_synchronize_post_reset(CPUState *cpu);
 void kvm_cpu_synchronize_post_init(CPUState *cpu);
+void kvm_cpu_clean_state(CPUState *cpu);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -372,6 +373,13 @@
     }
 }
 
+static inline void cpu_clean_state(CPUState *cpu)
+{
+    if (kvm_enabled()) {
+        kvm_cpu_clean_state(cpu);
+    }
+}
+
 int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg);
 int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg);
 void kvm_irqchip_release_virq(KVMState *s, int virq);
Index: qemu/kvm-all.c
===================================================================
--- qemu.orig/kvm-all.c	2014-09-04 15:38:03.338783100 -0300
+++ qemu/kvm-all.c	2014-09-04 15:38:31.722709526 -0300
@@ -1695,6 +1695,11 @@
     run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, cpu);
 }
 
+void kvm_cpu_clean_state(CPUState *cpu)
+{
+    cpu->kvm_vcpu_dirty = false;
+}
+
 int kvm_cpu_exec(CPUState *cpu)
 {
     struct kvm_run *run = cpu->kvm_run;

[Qemu-devel] [patch 3/3] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Marcelo Tosatti]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: kvmclock-ensure-proper-env-tsc.patch --]
[-- Type: text/plain, Size: 1276 bytes --]

Ensure proper env->tsc value for kvmclock_current_nsec calculation.

Reported-and-analyzed-by: Marcin Gibuła <m.gibula@beyond.pl>
Cc: qemu-stable@nongnu.org
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
 hw/i386/kvm/clock.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: qemu/hw/i386/kvm/clock.c
===================================================================
--- qemu.orig/hw/i386/kvm/clock.c	2014-09-05 10:07:04.116665549 -0300
+++ qemu/hw/i386/kvm/clock.c	2014-09-05 10:31:06.894106473 -0300
@@ -17,6 +17,7 @@
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
 #include "hw/sysbus.h"
 #include "hw/kvm/clock.h"
 
@@ -65,6 +66,7 @@
 
     cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
 
+    assert(time.tsc_timestamp <= migration_tsc);
     delta = migration_tsc - time.tsc_timestamp;
     if (time.tsc_shift < 0) {
         delta >>= -time.tsc_shift;
@@ -123,6 +125,9 @@
         if (s->clock_valid) {
             return;
         }
+
+        cpu_synchronize_all_states();
+        cpu_clean_all_dirty();
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Andrey Korolyov]

On Fri, Sep 5, 2014 at 5:52 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> Regression fix tested with fio and ping-pong migration.
>
>

Thanks, actually this series also introduces regression with average
live migration time - 42s avg vs 7s avg for the reference VM on
ping-pong test. Can you please confirm this?

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Marcelo Tosatti]

On Fri, Sep 05, 2014 at 10:16:17PM +0400, Andrey Korolyov wrote:
> On Fri, Sep 5, 2014 at 5:52 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > Regression fix tested with fio and ping-pong migration.
> >
> >
> 
> Thanks, actually this series also introduces regression with average
> live migration time - 42s avg vs 7s avg for the reference VM on
> ping-pong test. Can you please confirm this?

Can't confirm - migration time is similar for me.

What workload are you using?

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Paolo Bonzini]

Il 05/09/2014 20:26, Marcelo Tosatti ha scritto:
>> > Thanks, actually this series also introduces regression with average
>> > live migration time - 42s avg vs 7s avg for the reference VM on
>> > ping-pong test. Can you please confirm this?
> Can't confirm - migration time is similar for me.

It's also fairly unlikely.

Paolo

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Andrey Korolyov]

On Fri, Sep 5, 2014 at 10:26 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Fri, Sep 05, 2014 at 10:16:17PM +0400, Andrey Korolyov wrote:
>> On Fri, Sep 5, 2014 at 5:52 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>> > Regression fix tested with fio and ping-pong migration.
>> >
>> >
>>
>> Thanks, actually this series also introduces regression with average
>> live migration time - 42s avg vs 7s avg for the reference VM on
>> ping-pong test. Can you please confirm this?
>
> Can't confirm - migration time is similar for me.
>
> What workload are you using?
>

Just an 'empty' VM on reference test and fio tests at the
patch-specific test suite. Actual presence of workload brings
difference for about 2s, effectively unmeasurable on a small series of
40+s migration times.

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Andrey Korolyov]

On Fri, Sep 5, 2014 at 10:35 PM, Andrey Korolyov <andrey@xdel.ru> wrote:
> On Fri, Sep 5, 2014 at 10:26 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>> On Fri, Sep 05, 2014 at 10:16:17PM +0400, Andrey Korolyov wrote:
>>> On Fri, Sep 5, 2014 at 5:52 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>>> > Regression fix tested with fio and ping-pong migration.
>>> >
>>> >
>>>
>>> Thanks, actually this series also introduces regression with average
>>> live migration time - 42s avg vs 7s avg for the reference VM on
>>> ping-pong test. Can you please confirm this?
>>
>> Can't confirm - migration time is similar for me.
>>
>> What workload are you using?
>>
>
> Just an 'empty' VM on reference test and fio tests at the
> patch-specific test suite. Actual presence of workload brings
> difference for about 2s, effectively unmeasurable on a small series of
> 40+s migration times.

Heh... hit independent bug due to different pre-test conditions, the
patch itself is ok.

Bug follows:
- boot up a VM, make sure to feed it heavy long task, for example
kernel compilation,
- neither migrate once in the middle of process (with fallback to
non-live migration which is available in libvirt, else it may take
forever) or just wait up to the end,
- live migration time degrades from sub-10s value to larger values
forever, not regarding if workload is still presented or not.

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Paolo Bonzini]

Il 05/09/2014 21:35, Andrey Korolyov ha scritto:
> - boot up a VM, make sure to feed it heavy long task, for example
> kernel compilation,
> - neither migrate once in the middle of process (with fallback to
> non-live migration which is available in libvirt, else it may take
> forever) or just wait up to the end,
> - live migration time degrades from sub-10s value to larger values
> forever, not regarding if workload is still presented or not.

Are you sure you aren't simply doing migration with a low bandwidth
limit (the default is 32 MB/s), and moving over a gigabyte worth of page
cache data to the new machine?

Paolo

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Andrey Korolyov]

On Sat, Sep 6, 2014 at 2:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 05/09/2014 21:35, Andrey Korolyov ha scritto:
>> - boot up a VM, make sure to feed it heavy long task, for example
>> kernel compilation,
>> - neither migrate once in the middle of process (with fallback to
>> non-live migration which is available in libvirt, else it may take
>> forever) or just wait up to the end,
>> - live migration time degrades from sub-10s value to larger values
>> forever, not regarding if workload is still presented or not.
>
> Are you sure you aren't simply doing migration with a low bandwidth
> limit (the default is 32 MB/s), and moving over a gigabyte worth of page
> cache data to the new machine?
>
> Paolo

As far as I can see setspeed does not change anything over stdev
variation for gigabit, but rss size does. Will check if this part is
actually broken, because we are overriding default speed value every
time and if it started being constant, it may explain such a
difference.

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Andrey Korolyov]

On Sat, Sep 6, 2014 at 3:05 AM, Andrey Korolyov <andrey@xdel.ru> wrote:
> On Sat, Sep 6, 2014 at 2:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 05/09/2014 21:35, Andrey Korolyov ha scritto:
>>> - boot up a VM, make sure to feed it heavy long task, for example
>>> kernel compilation,
>>> - neither migrate once in the middle of process (with fallback to
>>> non-live migration which is available in libvirt, else it may take
>>> forever) or just wait up to the end,
>>> - live migration time degrades from sub-10s value to larger values
>>> forever, not regarding if workload is still presented or not.
>>
>> Are you sure you aren't simply doing migration with a low bandwidth
>> limit (the default is 32 MB/s), and moving over a gigabyte worth of page
>> cache data to the new machine?
>>
>> Paolo
>
> As far as I can see setspeed does not change anything over stdev
> variation for gigabit, but rss size does. Will check if this part is
> actually broken, because we are overriding default speed value every
> time and if it started being constant, it may explain such a
> difference.

The mess was caused by libvirt which sets migration bandwidth to 8
EB/s, so process took shorter times when I expect from it to utilize
some percent of server interconnection bandwidth.

Re: [Qemu-devel] [patch 0/3] kvmclock: Ensure time in migration never goes backward (v3)

[Paolo Bonzini]

Il 05/09/2014 15:52, Marcelo Tosatti ha scritto:
> Regression fix tested with fio and ping-pong migration.

I applied the patches, but in a different order (2-3-1) to keep them
bisectable.

Can you send a follow-up patch, adding a comment before the call to
cpu_clean_all_dirty that explains what's going on?

Paolo