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

[Eduardo Habkost]

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 */

> +    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".

> +        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.

(If this is an optimization to avoid extra ioctl() calls, you can
cache it inside kvm_has_adjust_clock_stable()).

> +            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.

>  
>          /* 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