From patchwork Fri Dec 2 13:56:09 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eduardo Habkost X-Patchwork-Id: 9458437 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 B6DF260585 for ; Fri, 2 Dec 2016 13:56:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A851028562 for ; Fri, 2 Dec 2016 13:56:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9D15428564; Fri, 2 Dec 2016 13:56:55 +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 64B0D28562 for ; Fri, 2 Dec 2016 13:56:53 +0000 (UTC) Received: from localhost ([::1]:34622 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cCoKY-00068d-T5 for patchwork-qemu-devel@patchwork.kernel.org; Fri, 02 Dec 2016 08:56:50 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46919) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cCoK3-000662-Vk for qemu-devel@nongnu.org; Fri, 02 Dec 2016 08:56:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cCoJy-000650-QR for qemu-devel@nongnu.org; Fri, 02 Dec 2016 08:56:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49608) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cCoJy-00063g-IM for qemu-devel@nongnu.org; Fri, 02 Dec 2016 08:56:14 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 4587BC04D30A; Fri, 2 Dec 2016 13:56:13 +0000 (UTC) Received: from localhost (ovpn-116-204.phx2.redhat.com [10.3.116.204]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uB2DuBtd027537; Fri, 2 Dec 2016 08:56:11 -0500 Date: Fri, 2 Dec 2016 11:56:09 -0200 From: Eduardo Habkost To: Marcelo Tosatti Message-ID: <20161202135609.GC13060@thinpad.lan.raisama.net> References: <20161121105002.291554230@redhat.com> <20161121105052.598267440@redhat.com> <20161128141322.GB14328@thinpad.lan.raisama.net> <20161128164522.GB4028@amt.cnet> <20161128171201.GD14328@thinpad.lan.raisama.net> <20161129105600.GA12426@amt.cnet> <20161129123405.GF14328@thinpad.lan.raisama.net> <20161129235427.GA23961@amt.cnet> <20161130130928.GJ14328@thinpad.lan.raisama.net> <20161201143900.GA17402@amt.cnet> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161201143900.GA17402@amt.cnet> X-Fnord: you can see the fnord User-Agent: Mutt/1.7.1 (2016-10-04) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 02 Dec 2016 13:56:13 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [qemu patch V3 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, Dec 01, 2016 at 12:39:03PM -0200, Marcelo Tosatti wrote: > On Wed, Nov 30, 2016 at 11:09:28AM -0200, Eduardo Habkost wrote: > > On Tue, Nov 29, 2016 at 09:54:29PM -0200, Marcelo Tosatti wrote: > > > On Tue, Nov 29, 2016 at 10:34:05AM -0200, Eduardo Habkost wrote: > > > > 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 > > > > > > > > > > > > > > > > > > --- > > > > > > > > > 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 */ > > > > > > Done. > > > > > > > > + 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". > > > > > > Done. > > > > > > > > + 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. > > > > > > Because if s->clock_valid == false (from migration), > > > kvm_has_adjust_clock_stable() from the local host is irrelevant. > > > > That's true, and that's why updating it on kvm_get_clock() looks > > like a more logical place. > > But the current patch updates > s->clock_is_reliable at the "if (running)" > case of kvmclock_vm_state_change(). > > There, both s->clock_is_reliable and s->clock are > updated in sync: > > if (s->clock_valid == false) { > s->clock_is_reliable = kvm_has_adjust_clock_stable(); > } > > /* We can't rely on the saved clock value, just discard it */ > if (pvclock_via_mem) { > s->clock = pvclock_via_mem; > } > Here s->clock is updated only to be used to set data.clock in the next line (and never used again). I'm talking about every other s->clock assignment in the code. > However, there is no need to call kvm_get_clock() there. I'm talking about the existing kvm_get_clock() calls. > > kvm_get_clock() is called at the "} else {" case. > > Are you suggesting to > > * move s->clock_is_reliable assignment to kvm_get_clock() > * move s->clock assignment to kvm_get_clock() > * call kvm_get_clock() from the "if (running)" because > the assignments are in that function now. No. > > But you don't need KVM_GET_CLOCK to be called there. This is what I'm suggesting: Signed-off-by: Marcelo Tosatti [ehabkost: simplified migration logic] Signed-off-by: Eduardo Habkost --- hw/i386/kvm/clock.c | 95 ++++++++++++++++++++++++++++++++++++++++++-------- include/hw/i386/pc.h | 5 +++ target-i386/kvm.c | 7 ++++ target-i386/kvm_i386.h | 1 + 4 files changed, 94 insertions(+), 14 deletions(-) diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c index 0f75dd3..df07ec7 100644 --- a/hw/i386/kvm/clock.c +++ b/hw/i386/kvm/clock.c @@ -35,6 +35,13 @@ typedef struct KVMClockState { /*< public >*/ uint64_t clock; + /* whether the value in 'clock' is reliable (i.e. was set by a host + * with reliable KVM_GET_CLOCK) */ + bool clock_is_reliable; + /* Enable migration of clock_is_reliable. Some machine-types + * will disable it for compatibility with older QEMU versions. + */ + bool migrate_reliable_flag; bool clock_valid; } KVMClockState; @@ -81,6 +88,20 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s) return nsec + time.system_time; } +static void kvm_get_clock(KVMClockState *s) +{ + 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(); + } + s->clock = data.clock; + s->clock_is_reliable = kvm_has_adjust_clock_stable(); +} + static void kvmclock_vm_state_change(void *opaque, int running, RunState state) { @@ -91,16 +112,18 @@ static void kvmclock_vm_state_change(void *opaque, int running, if (running) { struct kvm_clock_data data = {}; - uint64_t time_at_migration = kvmclock_current_nsec(s); s->clock_valid = false; - /* We can't rely on the migrated clock value, just discard it */ - if (time_at_migration) { - s->clock = time_at_migration; + /* if host that set s->clock did not support reliable KVM_GET_CLOCK, + * read kvmclock value from memory + */ + if (!s->clock_is_reliable) { + data.clock = kvmclock_current_nsec(s); + } else { + data.clock = s->clock; } - data.clock = s->clock; ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); if (ret < 0) { fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret)); @@ -120,22 +143,23 @@ static void kvmclock_vm_state_change(void *opaque, int running, } } } else { - struct kvm_clock_data data; - int ret; + /*FIXME: is this really possible to happen? + * Can kvmclock_vm_state_change(s, 0) be called twice without + * a kvmclock_vm_state_change(s, 1) call in between? + */ if (s->clock_valid) { return; } 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; - + kvm_get_clock(s); + /* FIXME: the comment below doesn't match what is done at + * kvmclock_pre_save(). The comment must be updated, or + * kvmclock_pre_save() must be changed. I don't know what's + * the right thing to do. + */ /* * If the VM is stopped, declare the clock state valid to * avoid re-reading it on next vmsave (which would return @@ -152,22 +176,65 @@ static void kvmclock_realize(DeviceState *dev, Error **errp) 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->migrate_reliable_flag; +} + +static const VMStateDescription kvmclock_reliable_get_clock = { + .name = "kvmclock/clock_is_reliable", + .version_id = 1, + .minimum_version_id = 1, + .needed = kvmclock_src_use_reliable_get_clock, + .fields = (VMStateField[]) { + VMSTATE_BOOL(clock_is_reliable, KVMClockState), + VMSTATE_END_OF_LIST() + } +}; + +static void kvmclock_pre_save(void *opaque) +{ + KVMClockState *s = opaque; + /*TODO: add comment explaining why this is here: + * "re-read clock value here because [...]"" */ + + /*XXX: is it correct to call kvm_get_clock() here + * if s->clock_is_valid is true? (meaning the kvmclock was + * already stopped) + */ + kvm_get_clock(s); +} + 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-migrate-reliable-flag", KVMClockState, + migrate_reliable_flag, 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 = { diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 4b74130..fb80d69 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -372,6 +372,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); #define PC_COMPAT_2_7 \ HW_COMPAT_2_7 \ {\ + .driver = "kvmclock",\ + .property = "x-migrate-reliable-flag",\ + .value = "off",\ + },\ + {\ .driver = TYPE_X86_CPU,\ .property = "l3-cache",\ .value = "off",\ diff --git a/target-i386/kvm.c b/target-i386/kvm.c index f62264a..10a9cd8 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -117,6 +117,13 @@ bool kvm_has_smm(void) 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(); diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h index 7607929..bfce427 100644 --- a/target-i386/kvm_i386.h +++ b/target-i386/kvm_i386.h @@ -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);