From patchwork Thu Nov 17 17:15:35 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marcelo Tosatti X-Patchwork-Id: 9434669 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3D33A60469 for ; Thu, 17 Nov 2016 17:16:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 29CBF2960D for ; Thu, 17 Nov 2016 17:16:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1D76829671; Thu, 17 Nov 2016 17:16:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 28B3C2960D for ; Thu, 17 Nov 2016 17:16:39 +0000 (UTC) Received: from localhost ([::1]:60481 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7QIf-0000OA-TS for patchwork-qemu-devel@patchwork.kernel.org; Thu, 17 Nov 2016 12:16:37 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58610) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7QID-0000Mx-2E for qemu-devel@nongnu.org; Thu, 17 Nov 2016 12:16:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7QI9-0000z8-9R for qemu-devel@nongnu.org; Thu, 17 Nov 2016 12:16:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40838) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c7QI9-0000yW-25 for qemu-devel@nongnu.org; Thu, 17 Nov 2016 12:16:05 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3450D96B7; Thu, 17 Nov 2016 17:16:04 +0000 (UTC) Received: from amt.cnet (vpn1-4-42.gru2.redhat.com [10.97.4.42]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAHHG2JL020834; Thu, 17 Nov 2016 12:16:03 -0500 Received: from amt.cnet (localhost [127.0.0.1]) by amt.cnet (Postfix) with ESMTP id CE8B11008F2; Thu, 17 Nov 2016 15:15:38 -0200 (BRST) Received: (from marcelo@localhost) by amt.cnet (8.14.7/8.14.7/Submit) id uAHHFZLv032044; Thu, 17 Nov 2016 15:15:35 -0200 Date: Thu, 17 Nov 2016 15:15:35 -0200 From: Marcelo Tosatti To: Eduardo Habkost Message-ID: <20161117171532.GB27675@amt.cnet> References: <20161117132411.125108658@redhat.com> <20161117132432.367332082@redhat.com> <20161117141126.GW5057@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161117141126.GW5057@thinpad.lan.raisama.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 17 Nov 2016 17:16:04 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [qemu patch V2 2/2] kvmclock: reduce kvmclock difference on migration X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kvm@vger.kernel.org, Juan Quintela , Radim Krcmar , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Paolo Bonzini Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Nov 17, 2016 at 12:11:26PM -0200, 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 > > > > --- > > 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. Done. > > + } > > > > - /* 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? Its initialized to false (because its part of device state). > > > + > > + 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? > > It looks like mach_use_reliable_get_clock and > src_use_reliable_get_clock could become a single field: > > * use_reliable_get_clock ("x-use-reliable-get-clock" property) > set to true by default (set on DEFINE_PROP_BOOL parameter) > * "x-use-reliable-get-clock" set to false by default on older > machine-types > * use_reliable_get_clock forced to false on kvmclock_realize() if > !kvm_has_adjust_clock_stable() > * kvmclock_reliable_get_clock.needed return s->use_reliable_get_clock No because use_reliable_get_clock can be false because the destination host does not support kvm_has_adjust_clock_stable(), but the source host supports it (so we want to use the value in the first incoming migration). So the variable indicates whether the source host supported migration, not whether the destination supports it. > > > +} > > + > > +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(); > > +} > > + > > 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("mach_use_reliable_get_clock", KVMClockState, > > + mach_use_reliable_get_clock, true), > > QOM property coding style is to use "-" instead of "_". Also, > please prefix it with "x-" to indicate it is not part of QEMU > stable external interface. > > > + 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 = { > > Index: qemu-mig-advance-clock/include/hw/i386/pc.h > > =================================================================== > > --- qemu-mig-advance-clock.orig/include/hw/i386/pc.h 2016-11-14 10:40:39.748116312 -0200 > > +++ qemu-mig-advance-clock/include/hw/i386/pc.h 2016-11-14 10:40:48.059128649 -0200 > > @@ -370,6 +370,11 @@ > > #define PC_COMPAT_2_7 \ > > HW_COMPAT_2_7 \ > > {\ > > + .driver = "kvmclock",\ > > + .property = "mach_use_reliable_get_clock",\ > > + .value = "off",\ > > + },\ > > + {\ > > .driver = TYPE_X86_CPU,\ > > .property = "l3-cache",\ > > .value = "off",\ > > Index: qemu-mig-advance-clock/target-i386/kvm.c > > =================================================================== > > --- qemu-mig-advance-clock.orig/target-i386/kvm.c 2016-11-14 10:40:39.750116314 -0200 > > +++ qemu-mig-advance-clock/target-i386/kvm.c 2016-11-14 10:40:48.061128652 -0200 > > @@ -117,6 +117,13 @@ > > 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(); > > Index: qemu-mig-advance-clock/target-i386/kvm_i386.h > > =================================================================== > > --- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h 2016-11-14 10:40:39.751116316 -0200 > > +++ qemu-mig-advance-clock/target-i386/kvm_i386.h 2016-11-14 10:40:48.061128652 -0200 > > @@ -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); > > > > > > -- > Eduardo How about the following: 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 @@ 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->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(); +} + 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-mach-use-reliable-get-clock", KVMClockState, + mach_use_reliable_get_clock, 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 = { Index: qemu-mig-advance-clock/include/hw/i386/pc.h =================================================================== --- qemu-mig-advance-clock.orig/include/hw/i386/pc.h 2016-11-17 15:07:11.220632761 -0200 +++ qemu-mig-advance-clock/include/hw/i386/pc.h 2016-11-17 15:08:58.096791416 -0200 @@ -370,6 +370,11 @@ #define PC_COMPAT_2_7 \ HW_COMPAT_2_7 \ {\ + .driver = "kvmclock",\ + .property = "x-mach-use-reliable-get-clock",\ + .value = "off",\ + },\ + {\ .driver = TYPE_X86_CPU,\ .property = "l3-cache",\ .value = "off",\ Index: qemu-mig-advance-clock/target-i386/kvm.c =================================================================== --- qemu-mig-advance-clock.orig/target-i386/kvm.c 2016-11-17 15:07:11.221632762 -0200 +++ qemu-mig-advance-clock/target-i386/kvm.c 2016-11-17 15:07:15.867639659 -0200 @@ -117,6 +117,13 @@ 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(); Index: qemu-mig-advance-clock/target-i386/kvm_i386.h =================================================================== --- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h 2016-11-17 15:07:11.222632764 -0200 +++ qemu-mig-advance-clock/target-i386/kvm_i386.h 2016-11-17 15:07:15.867639659 -0200 @@ -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);