From patchwork Tue Nov 29 10:56:00 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marcelo Tosatti X-Patchwork-Id: 9451477 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 18C5F60756 for ; Tue, 29 Nov 2016 10:57:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 142A227DA4 for ; Tue, 29 Nov 2016 10:57:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 08FE328173; Tue, 29 Nov 2016 10:57:16 +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=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5B8E227DA4 for ; Tue, 29 Nov 2016 10:57:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756263AbcK2K5K (ORCPT ); Tue, 29 Nov 2016 05:57:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35482 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755462AbcK2K5F (ORCPT ); Tue, 29 Nov 2016 05:57: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 096124E356; Tue, 29 Nov 2016 10:57:05 +0000 (UTC) Received: from amt.cnet (vpn1-4-137.gru2.redhat.com [10.97.4.137]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uATAv3SI006246; Tue, 29 Nov 2016 05:57:04 -0500 Received: from amt.cnet (localhost [127.0.0.1]) by amt.cnet (Postfix) with ESMTP id 94EEF1008B3; Tue, 29 Nov 2016 08:56:03 -0200 (BRST) Received: (from marcelo@localhost) by amt.cnet (8.14.7/8.14.7/Submit) id uATAu0Zt012735; Tue, 29 Nov 2016 08:56:00 -0200 Date: Tue, 29 Nov 2016 08:56:00 -0200 From: Marcelo Tosatti To: Eduardo Habkost Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Dr David Alan Gilbert , Paolo Bonzini , Juan Quintela , Radim Krcmar Subject: Re: [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration Message-ID: <20161129105600.GA12426@amt.cnet> 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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161128171201.GD14328@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.38]); Tue, 29 Nov 2016 10:57:05 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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: --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 */ + 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); - } + + 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) { + s->clock_is_reliable = kvm_has_adjust_clock_stable(); } /* 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() } };