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 <mtosatti@xxxxxxxxxx>
>>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
