Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration

[Eduardo Habkost]

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.

> 
> > You can change kvm_get_clock() to kvm_get_clock(KVMClockState*),
> > and make it update both s->clock and s->clock_is_reliable. With
> > this, s->clock and s->clock_is_reliable will be always in sync,
> > and have a single code path for both local restore and migration:
> 
> There should not be a single path for migration/runtime cases: 
> on migration, we care whether the source used reliable KVM_GET_CLOCK.
> 
> On runtime, we care whether the destination uses reliable KVM_GET_CLOCK.

The conditions may be different, but both can be translated to a
"should we trust the value in s->clock" flag, to simplify the
code (just like Paolo's suggestion on his reply to v1 today).

> 
> >     /*
> >      * If host that set s->clock did not support reliable
> >      * KVM_GET_CLOCK, read kvmclock value from memory
> >      */
> >     if (!s->clock_is_reliable) {
> >         pvclock_via_mem = kvmclock_current_nsec(s);
> >     }
> > 
> > Updating the table from my previous message, the result would be:
> > 
> > --------+------------+---------------+-----------+---------+
> >         | kvm_has_adj_clock_stable() | clock_is_reliable   |
> > machine | src        | dst           | src       | dst (*) |
> > --------+------------+---------------+-----------+---------+
> > pc-2.8  | false      | false         | false     | false   |
> > pc-2.8  | false      | true          | false     | false   |
> > pc-2.8  | true       | false         | true      | true    |
> > pc-2.8  | true       | true          | true      | true    |
> > --------+------------+---------------+-----------+---------+
> > pc-2.7  | false      | (any)         | false     | false   |
> > pc-2.7  | true       | (any)         | true (**) | false   |
> > --------+------------+---------------+-----------+---------+
> > 
> > (*) More precisely: in the first kvmclock_vm_state_change() call
> >     just after migration. It will get updated the next time
> >     s->clock is set.
> > 
> > (**) On purpose, so the code I suggest above would work for local
> >      restore, too.
> > 
> > 
> > > +                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);
> > > +            }
> > > +        }
> > >  
> > > -        /* We can't rely on the migrated clock value, just discard it */
> > > -        if (time_at_migration) {
> > > -            s->clock = time_at_migration;
> > > +        /* We can't rely on the saved clock value, just discard it */
> > > +        if (pvclock_via_mem) {
> > > +            s->clock = pvclock_via_mem;
> > >          }
> > >  
> > > +        s->clock_valid = false;
> > > +
> > >          data.clock = s->clock;
> > >          ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> > >          if (ret < 0) {
> > > @@ -120,8 +160,6 @@
> > >              }
> > >          }
> > >      } else {
> > > -        struct kvm_clock_data data;
> > > -        int ret;
> > >  
> > >          if (s->clock_valid) {
> > >              return;
> > > @@ -129,13 +167,7 @@
> > >  
> > >          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;
> > > -
> > > +        s->clock = kvm_get_clock();
> > >          /*
> > >           * If the VM is stopped, declare the clock state valid to
> > >           * avoid re-reading it on next vmsave (which would return
> > > @@ -149,25 +181,72 @@
> > >  {
> > >      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;
> > > +    }
> > > +
> > >      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->mach_use_reliable_get_clock;
> > > +}
> > > +
> > > +static const VMStateDescription kvmclock_reliable_get_clock = {
> > > +    .name = "kvmclock/src_use_reliable_get_clock",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = kvmclock_src_use_reliable_get_clock,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +static void kvmclock_pre_save(void *opaque)
> > > +{
> > > +    KVMClockState *s = opaque;
> > > +
> > > +    s->clock = kvm_get_clock();
> > 
> > I can't tell if s->clock_valid here is a bug, or a feature. In
> > either case, we seem to be contradicting the comment at
> > kvmclock_vm_state_change():
> > 
> >     /*
> >      * If the VM is stopped, declare the clock state valid to
> >      * avoid re-reading it on next vmsave (which would return
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >      * a different value). Will be reset when the VM is continued.
> >      */
> >     s->clock_valid = true;
> 
> Sorry, what is the problem??

The comment says clock_valid will avoid re-reading the clock, but
kvmclock_pre_save() ignores clock_valid and will re-read it
unconditionally. Either the comment is incorrect, or
kvmclock_pre_save() is incorrect (I don't know which is the
case).

-- 
Eduardo