From patchwork Fri Dec 2 22:30: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: 9459361 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 2251060515 for ; Fri, 2 Dec 2016 22:31:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 14100285A1 for ; Fri, 2 Dec 2016 22:31:25 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 078EA285AA; Fri, 2 Dec 2016 22:31:25 +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 50003285A3 for ; Fri, 2 Dec 2016 22:31:23 +0000 (UTC) Received: from localhost ([::1]:36814 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cCwMS-0002vQ-03 for patchwork-qemu-devel@patchwork.kernel.org; Fri, 02 Dec 2016 17:31:20 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cCwM7-0002vK-Rc for qemu-devel@nongnu.org; Fri, 02 Dec 2016 17:31:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cCwM4-0007zF-Kk for qemu-devel@nongnu.org; Fri, 02 Dec 2016 17:30:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34140) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cCwM4-0007z6-9F for qemu-devel@nongnu.org; Fri, 02 Dec 2016 17:30:56 -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 2FD0F9D781; Fri, 2 Dec 2016 22:30:54 +0000 (UTC) Received: from amt.cnet (vpn1-4-99.gru2.redhat.com [10.97.4.99]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uB2MUqOP000898; Fri, 2 Dec 2016 17:30:53 -0500 Received: from amt.cnet (localhost [127.0.0.1]) by amt.cnet (Postfix) with ESMTP id 68A611008DD; Fri, 2 Dec 2016 20:30:39 -0200 (BRST) Received: (from marcelo@localhost) by amt.cnet (8.14.7/8.14.7/Submit) id uB2MUZS2001414; Fri, 2 Dec 2016 20:30:35 -0200 Date: Fri, 2 Dec 2016 20:30:35 -0200 From: Marcelo Tosatti To: Eduardo Habkost Message-ID: <20161202223033.GB31213@amt.cnet> References: <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> <20161202135609.GC13060@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161202135609.GC13060@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.28]); Fri, 02 Dec 2016 22:30:54 +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 Fri, Dec 02, 2016 at 11:56:09AM -0200, Eduardo Habkost wrote: > 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(); > +} > + s->clock_is_reliable has to updated at migration, after the first time KVM_SET_CLOCK is called. There is no other place where it should be updated. This is the behaviour desired, according to this table: index: mig->c means use s->clock value from migration mig->m means read pvclock value from memory (both at migration, only the first time kvmclock_change_vmstate is called). runt->c means use KVM_GET_CLOCK/KVM_SET_CLOCK at runtime runt->m means read pvclock value from memory (both at runtime, after the first time kvmclock_change_vmstate is called). SOURCE DESTINATION BEHAVIOUR get_clock_reliable get_clock_reliable mig->c,runt->c 1 get_clock_reliable ~get_clock_reliable mig->c,runt->m 2 ~get_clock_reliable get_clock_reliable mig->m,runt->c 3 ~get_clock_reliable ~get_clock_reliable mig->m,runt->m 4 So looking at your patch below, and thinking of kvm_get_clock() updating s->clock_is_reliable from !running case (that is RUNNING -> ~RUNNING transition) makes no sense at all. That should not happen. s->clock_is_reliable should be updated after the first time ~RUNNING -> RUNNING migration is done, which is my patch does. I think your s->clock,s->clock_is_reliable should be in sync suggestion comes from a misunderstanding of how they are used. s->clock is not read if s->clock_is_reliable is set. Look at my attached patch below (still untested, will test and then resubmit) and check whether it matches the behaviour described in the table above. > 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? > + */ It can't: check the code where notifiers are called. > 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. > + */ This fails for all cases (1,2,3,4) in the table above. > /* > * 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); > -- > 2.7.4 > > > > > > > > > > > > > > > > > If s->clock_valid == true, kvm_has_adjust_clock_stable() is > > > > what decides whether to use KVM_GET_CLOCK or not. > > > > > > > > > (If this is an optimization to avoid extra ioctl() calls, you can > > > > > cache it inside kvm_has_adjust_clock_stable()). > > > > > > > > Its not. > > > > > > > > > > + 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. > > > > > > > > No: the point to change clock_is_reliable is the following: whether > > > > its the first read of the clock (migration), or subsequent > > > > reads (normal machine operation). > > > > > > > > Now kvm_get_clock() just reads the clock, so i don't see it as > > > > the right place to change the variable. > > > > > > > > Agreed? > > > > > > I don't agree. clock_is_reliable is information about the current > > > value in s->clock, I don't see why we need to change s->clock and > > > s->clock_is_reliable at different places in the code. > > > > > > Your code might work too, but I can't tell without reviewing very > > > carefully the state transitions KVMClockState could go through. I > > > don't see the point of making the state transitions in this code > > > even more complex, if we can simply stick to a simple "s->clock > > > and s->clock_is_reliable are always in sync" rule. > > > > It is updating from the same place: > > > > /* migration/savevm/init restore > > * update clock_is_reliable to match local > > * host capabilities. > > */ > > 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; > > } > > I'm talking about the other s->clock assignments. This one > doesn't even seem to be necessary. It is necessary: this is how you read pvclock from memory. > > > > > > > > > > > > > > > > > /* 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 > > > > > > -- > > > Eduardo > > -- > Eduardo kvmclock: reduce kvmclock difference on migration 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 | 97 +++++++++++++++++++++++++++++++++++++++++-------- include/hw/i386/pc.h | 5 ++ target-i386/kvm.c | 7 +++ target-i386/kvm_i386.h | 1 4 files changed, 96 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-12-01 12:43:18.606294830 -0200 @@ -36,6 +36,13 @@ uint64_t clock; bool clock_valid; + + /* whether machine type supports reliable get clock */ + bool mach_use_reliable_get_clock; + + /* whether the 'clock' value was obtained in a host with + * reliable KVM_GET_CLOCK */ + bool clock_is_reliable; } KVMClockState; struct pvclock_vcpu_time_info { @@ -81,6 +88,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 +111,31 @@ 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; + /* + * 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); + } - /* We can't rely on the migrated clock value, just discard it */ - if (time_at_migration) { - s->clock = time_at_migration; + /* migration/savevm/init restore + * update clock_is_reliable to match local + * host capabilities. + */ + 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; + } + + s->clock_valid = false; + data.clock = s->clock; ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); if (ret < 0) { @@ -120,8 +156,6 @@ } } } else { - struct kvm_clock_data data; - int ret; if (s->clock_valid) { return; @@ -129,13 +163,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 +177,66 @@ { KVMClockState *s = KVM_CLOCK(dev); + 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_clock_is_reliable_needed(void *opaque) +{ + KVMClockState *s = opaque; + + return s->mach_use_reliable_get_clock; +} + +static const VMStateDescription kvmclock_reliable_get_clock = { + .name = "kvmclock/clock_is_reliable", + .version_id = 1, + .minimum_version_id = 1, + .needed = kvmclock_clock_is_reliable_needed, + .fields = (VMStateField[]) { + VMSTATE_BOOL(clock_is_reliable, 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);