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

[Paolo Bonzini]

On 17/11/2016 15:11, Eduardo Habkost wrote:
> On Thu, Nov 17, 2016 at 11:24:13AM -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    |  108 
>> ++++++++++++++++++++++++++++++++++++++++++-------
>>  include/hw/i386/pc.h   |    5 ++
>>  target-i386/kvm.c      |    7 +++
>>  target-i386/kvm_i386.h |    1 
>>  4 files changed, 107 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)
>>
>> Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
>> ===================================================================
>> --- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c  2016-11-14 
>> 10:40:39.748116312 -0200
>> +++ qemu-mig-advance-clock/hw/i386/kvm/clock.c       2016-11-14 
>> 13:38:29.299955042 -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,37 @@
>>  
>>      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()) {
>> +                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->mach_use_reliable_get_clock &&
>> +                  s->src_use_reliable_get_clock)) {
>> +                pvclock_via_mem = kvmclock_current_nsec(s);
>> +            }
> 
> The s->mach_use_reliable_get_clock check seems redundant.
> src_use_reliable_get_clock is set only if
> mach_use_reliable_get_clock is true.
> 
>> +        }
>>  
>> -        /* 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 +161,6 @@
>>              }
>>          }
>>      } else {
>> -        struct kvm_clock_data data;
>> -        int ret;
>>  
>>          if (s->clock_valid) {
>>              return;
>> @@ -129,13 +168,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
>> @@ -152,22 +185,69 @@
>>      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>>  }
>>  
>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
>> +{
>> +    KVMClockState *s = opaque;
>> +
>> +    /*
>> +     * 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;
>> +    }
> 
> It feels fragile to change device state inside the .needed
> function. Better to initialize src_use_reliable_get_clock on
> kvmclock_realize()?
> 
> What exactly ensures src_use_reliable_get_clock is correctly
> initialized on the migration destination as well?
> 
>> +
>> +    return s->mach_use_reliable_get_clock;
> 
> If if kvm_has_adjust_clock_stable() is false, isn't it simpler to
> simply skip the section?

This is what I asked for. :)

However, I was proposing a different way to initialize
src_use_reliable_get_clock.  I still have to understand exactly how
Marcelo's algorithm works because (based on the kvmclock code) it's more
trick than it seems.

Paolo