qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock differenc


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration
Date: Fri, 2 Dec 2016 11:56:09 -0200
User-agent: Mutt/1.7.1 (2016-10-04)

On Thu, Dec 01, 2016 at 12:39:03PM -0200, Marcelo Tosatti wrote:
> On Wed, Nov 30, 2016 at 11:09:28AM -0200, Eduardo Habkost wrote:
> > On Tue, Nov 29, 2016 at 09:54:29PM -0200, Marcelo Tosatti wrote:
> > > On Tue, Nov 29, 2016 at 10:34:05AM -0200, Eduardo Habkost wrote:
> > > > On Tue, Nov 29, 2016 at 08:56:00AM -0200, Marcelo Tosatti wrote:
> > > > > On Mon, Nov 28, 2016 at 03:12:01PM -0200, Eduardo Habkost wrote:
> > > > > > On Mon, Nov 28, 2016 at 02:45:24PM -0200, Marcelo Tosatti wrote:
> > > > > > > On Mon, Nov 28, 2016 at 12:13:22PM -0200, Eduardo Habkost wrote:
> > > > > > > > Sorry for not noticing the following issues on the previous
> > > > > > > > reviews. I was only paying attention to the vmstate and machine
> > > > > > > > code:
> > > > > > > > 
> > > > > > > > On Mon, Nov 21, 2016 at 08:50:04AM -0200, Marcelo Tosatti wrote:
> > > > > > > > > Check for KVM_CAP_ADJUST_CLOCK capability 
> > > > > > > > > KVM_CLOCK_TSC_STABLE, which
> > > > > > > > > indicates that KVM_GET_CLOCK returns a value as seen by the 
> > > > > > > > > guest at
> > > > > > > > > that moment.
> > > > > > > > > 
> > > > > > > > > For new machine types, use this value rather than reading 
> > > > > > > > > from guest memory.
> > > > > > > > > 
> > > > > > > > > This reduces kvmclock difference on migration from 5s to 0.1s
> > > > > > > > > (when max_downtime == 5s).
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Marcelo Tosatti <address@hidden>
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >  hw/i386/kvm/clock.c    |  107 
> > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > > > > >  include/hw/i386/pc.h   |    5 ++
> > > > > > > > >  target-i386/kvm.c      |    7 +++
> > > > > > > > >  target-i386/kvm_i386.h |    1 
> > > > > > > > >  4 files changed, 106 insertions(+), 14 deletions(-)
> > > > > > > > > 
> > > > > > > > > v2: 
> > > > > > > > > - improve variable names (Juan)
> > > > > > > > > - consolidate code on kvm_get_clock function (Paolo)
> > > > > > > > > - return mach_use_reliable_get_clock from needed function 
> > > > > > > > > (Paolo)
> > > > > > > > > v3: 
> > > > > > > > > - simplify check for src_use_reliable_get_clock (Eduardo)
> > > > > > > > > - move src_use_reliable_get_clock initialization to realize 
> > > > > > > > > (Eduardo)
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
> > > > > > > > > ===================================================================
> > > > > > > > > --- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c   
> > > > > > > > > 2016-11-17 15:07:11.220632761 -0200
> > > > > > > > > +++ qemu-mig-advance-clock/hw/i386/kvm/clock.c        
> > > > > > > > > 2016-11-17 15:11:51.372048640 -0200
> > > > > > > > > @@ -36,6 +36,12 @@
> > > > > > > > >  
> > > > > > > > >      uint64_t clock;
> > > > > > > > >      bool clock_valid;
> > > > > > > > > +
> > > > > > > > > +    /* whether machine supports reliable KVM_GET_CLOCK */
> > > > > > > > > +    bool mach_use_reliable_get_clock;
> > > > > > > > > +
> > > > > > > > > +    /* whether source host supported reliable KVM_GET_CLOCK 
> > > > > > > > > */
> > > > > > > > > +    bool src_use_reliable_get_clock;
> > > > > > > > >  } KVMClockState;
> > > > > > > > >  
> > > > > > > > >  struct pvclock_vcpu_time_info {
> > > > > > > > > @@ -81,6 +87,19 @@
> > > > > > > > >      return nsec + time.system_time;
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > +static uint64_t kvm_get_clock(void)
> > > > > > > > > +{
> > > > > > > > > +    struct kvm_clock_data data;
> > > > > > > > > +    int ret;
> > > > > > > > > +
> > > > > > > > > +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> > > > > > > > > +    if (ret < 0) {
> > > > > > > > > +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", 
> > > > > > > > > strerror(ret));
> > > > > > > > > +                abort();
> > > > > > > > > +    }
> > > > > > > > > +    return data.clock;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static void kvmclock_vm_state_change(void *opaque, int 
> > > > > > > > > running,
> > > > > > > > >                                       RunState state)
> > > > > > > > >  {
> > > > > > > > > @@ -91,15 +110,36 @@
> > > > > > > > 
> > > > > > > > Can you please use "diff -p" on your patches?
> > > > > > > > 
> > > > > > > > >  
> > > > > > > > >      if (running) {
> > > > > > > > >          struct kvm_clock_data data = {};
> > > > > > > > > -        uint64_t time_at_migration = 
> > > > > > > > > kvmclock_current_nsec(s);
> > > > > > > > > +        uint64_t pvclock_via_mem = 0;
> > > > > > > > >  
> > > > > > > > > -        s->clock_valid = false;
> > > > > > > > > +        /* local (running VM) restore */
> > > > > > > > > +        if (s->clock_valid) {
> > > > > > > > > +            /*
> > > > > > > > > +             * if host does not support reliable 
> > > > > > > > > KVM_GET_CLOCK,
> > > > > > > > > +             * read kvmclock value from memory
> > > > > > > > > +             */
> > > > > > > > > +            if (!kvm_has_adjust_clock_stable()) {
> > > > > > > > 
> > > > > > > > Why not use src_use_reliable_get_clock here, too? (Maybe rename
> > > > > > > > it to simply clock_is_reliable?)
> > > > > > > 
> > > > > > > Because you end up mixing two mental ideas (one: whether the 
> > > > > > > source
> > > > > > > has KVM_GET_CLOCK, second: whether the destination has 
> > > > > > > KVM_GET_CLOCK 
> > > > > > > into one variable). I find it more confusing than not.
> > > > > > > Maybe its just my limited brain but i find it
> > > > > > > confusing.
> > > > > > 
> > > > > > I find it simpler and easier to reason about.
> > > > > 
> > > > > I disagree, but i don't mind. Is this what you refer to:
> > > > > 
> > > > > Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
> > > > > ===================================================================
> > > > > --- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c   2016-11-17 
> > > > > 15:11:51.372048640 -0200
> > > > > +++ qemu-mig-advance-clock/hw/i386/kvm/clock.c        2016-11-29 
> > > > > 08:53:20.609817026 -0200
> > > > > @@ -37,11 +37,11 @@
> > > > >      uint64_t clock;
> > > > >      bool clock_valid;
> > > > >  
> > > > > -    /* whether machine supports reliable KVM_GET_CLOCK */
> > > > > +    /* whether machine type supports reliable get clock */
> > > > >      bool mach_use_reliable_get_clock;
> > > > >  
> > > > > -    /* whether source host supported reliable KVM_GET_CLOCK */
> > > > > -    bool src_use_reliable_get_clock;
> > > > > +    /* whether clock is reliable */
> > > > 
> > > > I would make the comment more explicit about its meaning:
> > > >   /* whether the 'clock' value was obtained in a host with
> > > >    * reliable KVM_GET_CLOCK */
> > > 
> > > Done.
> > > 
> > > > > +    bool clock_is_reliable;
> > > >
> > > >  } KVMClockState;
> > > > >  
> > > > >  struct pvclock_vcpu_time_info {
> > > > > @@ -112,25 +112,17 @@
> > > > >          struct kvm_clock_data data = {};
> > > > >          uint64_t pvclock_via_mem = 0;
> > > > >  
> > > > > -        /* local (running VM) restore */
> > > > > -        if (s->clock_valid) {
> > > > > -            /*
> > > > > -             * if host does not support reliable KVM_GET_CLOCK,
> > > > > -             * read kvmclock value from memory
> > > > > -             */
> > > > > -            if (!kvm_has_adjust_clock_stable()) {
> > > > > -                pvclock_via_mem = kvmclock_current_nsec(s);
> > > > > -            }
> > > > > -        /* migration/savevm/init restore */
> > > > > -        } else {
> > > > > -            /*
> > > > > -             * use s->clock in case machine uses reliable
> > > > > -             * get clock and source host supported
> > > > > -             * reliable get clock
> > > > > -             */
> > > > > -            if (!s->src_use_reliable_get_clock) {
> > > > > -                pvclock_via_mem = kvmclock_current_nsec(s);
> > > > > -            }
> > > > > +
> > > > 
> > > > A comment here would be welcome. Something like: "If the host
> > > > where s->clock was read did not support reliable KVM_GET_CLOCK,
> > > > read kvmclock value from memory".
> > > 
> > > Done.
> > > 
> > > > > +        if (!s->clock_is_reliable) {
> > > > > +            pvclock_via_mem = kvmclock_current_nsec(s);
> > > > > +        }
> > > > > +
> > > > > +        /* migration/savevm/init restore
> > > > > +         * update clock_is_reliable to match local
> > > > > +         * host capabilities.
> > > > > +         */
> > > > > +        if (s->clock_valid == false) {
> > > > 
> > > > Why the s->clock_valid check? It seems unnecessary.
> > > 
> > > Because if s->clock_valid == false (from migration),
> > > kvm_has_adjust_clock_stable() from the local host is irrelevant.
> > 
> > That's true, and that's why updating it on kvm_get_clock() looks
> > like a more logical place.
> 
> But the current patch updates
> s->clock_is_reliable at the "if (running)"
> case of kvmclock_vm_state_change().
> 
> There, both s->clock_is_reliable and s->clock are
> updated in sync:
> 
>         if (s->clock_valid == false) {
>             s->clock_is_reliable = kvm_has_adjust_clock_stable();
>         }
> 
>         /* We can't rely on the saved clock value, just discard it */
>         if (pvclock_via_mem) {
>             s->clock = pvclock_via_mem;
>         }
> 

Here s->clock is updated only to be used to set data.clock in the
next line (and never used again). I'm talking about every other
s->clock assignment in the code.

> However, there is no need to call kvm_get_clock() there.

I'm talking about the existing kvm_get_clock() calls.

> 
> kvm_get_clock() is called at the "} else {" case.
> 
> Are you suggesting to
> 
> * move s->clock_is_reliable assignment to kvm_get_clock()
> * move s->clock assignment to kvm_get_clock()
> * call kvm_get_clock() from the "if (running)" because
> the assignments are in that function now.

No.

> 
> But you don't need KVM_GET_CLOCK to be called there.

This is what I'm suggesting:

Signed-off-by: Marcelo Tosatti <address@hidden>
[ehabkost: simplified migration logic]
Signed-off-by: Eduardo Habkost <address@hidden>
---
 hw/i386/kvm/clock.c    | 95 ++++++++++++++++++++++++++++++++++++++++++--------
 include/hw/i386/pc.h   |  5 +++
 target-i386/kvm.c      |  7 ++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 94 insertions(+), 14 deletions(-)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 0f75dd3..df07ec7 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -35,6 +35,13 @@ typedef struct KVMClockState {
     /*< public >*/
 
     uint64_t clock;
+    /* whether the value in 'clock' is reliable (i.e. was set by a host
+     * with reliable KVM_GET_CLOCK) */
+    bool clock_is_reliable;
+    /* Enable migration of clock_is_reliable. Some machine-types
+     * will disable it for compatibility with older QEMU versions.
+     */
+    bool migrate_reliable_flag;
     bool clock_valid;
 } KVMClockState;
 
@@ -81,6 +88,20 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
     return nsec + time.system_time;
 }
 
+static void kvm_get_clock(KVMClockState *s)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+        abort();
+    }
+    s->clock = data.clock;
+    s->clock_is_reliable = kvm_has_adjust_clock_stable();
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {
@@ -91,16 +112,18 @@ static void kvmclock_vm_state_change(void *opaque, int 
running,
 
     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;
+        /* if host that set s->clock did not support reliable KVM_GET_CLOCK,
+         * read kvmclock value from memory
+         */
+        if (!s->clock_is_reliable) {
+            data.clock = kvmclock_current_nsec(s);
+        } else {
+            data.clock = s->clock;
         }
 
-        data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret));
@@ -120,22 +143,23 @@ static void kvmclock_vm_state_change(void *opaque, int 
running,
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
+        /*FIXME: is this really possible to happen?
+         * Can kvmclock_vm_state_change(s, 0) be called twice without
+         * a kvmclock_vm_state_change(s, 1) call in between?
+         */
         if (s->clock_valid) {
             return;
         }
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        kvm_get_clock(s);
+        /* FIXME: the comment below doesn't match what is done at
+         * kvmclock_pre_save(). The comment must be updated, or
+         * kvmclock_pre_save() must be changed. I don't know what's
+         * the right thing to do.
+         */
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return
@@ -152,22 +176,65 @@ static void kvmclock_realize(DeviceState *dev, Error 
**errp)
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->migrate_reliable_flag;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/clock_is_reliable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    /*TODO: add comment explaining why this is here:
+     * "re-read clock value here because [...]"" */
+
+    /*XXX: is it correct to call kvm_get_clock() here
+     * if s->clock_is_valid is true? (meaning the kvmclock was
+     * already stopped)
+     */
+    kvm_get_clock(s);
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_reliable_get_clock,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("x-migrate-reliable-flag", KVMClockState,
+                      migrate_reliable_flag, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4b74130..fb80d69 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -372,6 +372,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "x-migrate-reliable-flag",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f62264a..10a9cd8 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -117,6 +117,13 @@ bool kvm_has_smm(void)
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 7607929..bfce427 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -17,6 +17,7 @@
 
 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);
-- 
2.7.4


> 
> 
> > 
> > > 
> > > If s->clock_valid == true, kvm_has_adjust_clock_stable() is
> > > what decides whether to use KVM_GET_CLOCK or not.
> > > 
> > > > (If this is an optimization to avoid extra ioctl() calls, you can
> > > > cache it inside kvm_has_adjust_clock_stable()).
> > > 
> > > Its not.
> > > 
> > > > > +            s->clock_is_reliable = kvm_has_adjust_clock_stable();
> > > > >          }
> > > > 
> > > > This should go to kvm_get_clock(), so s->clock and
> > > > s->clock_is_reliable are guaranteed to be always in sync.
> > > 
> > > No: the point to change clock_is_reliable is the following: whether 
> > > its the first read of the clock (migration), or subsequent 
> > > reads (normal machine operation).
> > > 
> > > Now kvm_get_clock() just reads the clock, so i don't see it as
> > > the right place to change the variable.
> > > 
> > > Agreed?
> > 
> > I don't agree. clock_is_reliable is information about the current
> > value in s->clock, I don't see why we need to change s->clock and
> > s->clock_is_reliable at different places in the code.
> > 
> > Your code might work too, but I can't tell without reviewing very
> > carefully the state transitions KVMClockState could go through. I
> > don't see the point of making the state transitions in this code
> > even more complex, if we can simply stick to a simple "s->clock
> > and s->clock_is_reliable are always in sync" rule.
> 
> It is updating from the same place:
> 
>         /* migration/savevm/init restore
>          * update clock_is_reliable to match local
>          * host capabilities.
>          */
>         if (s->clock_valid == false) {
>             s->clock_is_reliable = kvm_has_adjust_clock_stable();
>         }
> 
>         /* We can't rely on the saved clock value, just discard it */
>         if (pvclock_via_mem) {
>             s->clock = pvclock_via_mem;
>         }

I'm talking about the other s->clock assignments. This one
doesn't even seem to be necessary.

> 
> 
> > 
> > > 
> > > > >          /* We can't rely on the saved clock value, just discard it */
> > > > > @@ -181,20 +173,14 @@
> > > > >  {
> > > > >      KVMClockState *s = KVM_CLOCK(dev);
> > > > >  
> > > > > -    /*
> > > > > -     * On machine types that support reliable KVM_GET_CLOCK,
> > > > > -     * if host kernel does provide reliable KVM_GET_CLOCK,
> > > > > -     * set src_use_reliable_get_clock=true so that destination
> > > > > -     * avoids reading kvmclock from memory.
> > > > > -     */
> > > > > -    if (s->mach_use_reliable_get_clock && 
> > > > > kvm_has_adjust_clock_stable()) {
> > > > > -        s->src_use_reliable_get_clock = true;
> > > > > +    if (kvm_has_adjust_clock_stable()) {
> > > > > +        s->clock_is_reliable = true;
> > > > >      }
> > > > >  
> > > > >      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> > > > >  }
> > > > >  
> > > > > -static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> > > > > +static bool kvmclock_clock_is_reliable_needed(void *opaque)
> > > > >  {
> > > > >      KVMClockState *s = opaque;
> > > > >  
> > > > > @@ -202,12 +188,12 @@
> > > > >  }
> > > > >  
> > > > >  static const VMStateDescription kvmclock_reliable_get_clock = {
> > > > > -    .name = "kvmclock/src_use_reliable_get_clock",
> > > > > +    .name = "kvmclock/clock_is_reliable",
> > > > >      .version_id = 1,
> > > > >      .minimum_version_id = 1,
> > > > > -    .needed = kvmclock_src_use_reliable_get_clock,
> > > > > +    .needed = kvmclock_clock_is_reliable_needed,
> > > > >      .fields = (VMStateField[]) {
> > > > > -        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
> > > > > +        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
> > > > >          VMSTATE_END_OF_LIST()
> > > > >      }
> > > > >  };
> > > > 
> > > > -- 
> > > > Eduardo
> > 
> > -- 
> > Eduardo

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]