From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration Date: Mon, 28 Nov 2016 12:28:55 -0200 Message-ID: <20161128142855.GC14328@thinpad.lan.raisama.net> References: <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> <20161114145054.GA28663@amt.cnet> <67bffd95-2e4e-7273-c154-a3fdfe622387@redhat.com> <20161114154015.GA30048@amt.cnet> <20161114171318.GA6336@amt.cnet> <14044cda-054d-94eb-8d91-7ad3a1e0869e@redhat.com> <20161114181518.GA14076@amt.cnet> <20161117121637.GA13404@amt.cnet> <1023a283-77a7-45e5-8877-6264e08d0658@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44616 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932761AbcK1O26 (ORCPT ); Mon, 28 Nov 2016 09:28:58 -0500 Content-Disposition: inline In-Reply-To: <1023a283-77a7-45e5-8877-6264e08d0658@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 28, 2016 at 02:47:18PM +0100, Paolo Bonzini wrote: > > > On 17/11/2016 13:16, Marcelo Tosatti wrote: > > What QEMU wants is to use KVM_GET_CLOCK at pre_save independently > > of whether masterclock is enabled or not... it just depends > > on KVM_GET_CLOCK being correct for the masterclock case > > (108b249c453dd7132599ab6dc7e435a7036c193f). > > > > So a "reliable KVM_GET_CLOCK" (that does not timebackward > > when masterclock is enabled) is much simpler to userspace > > than "whether masterclock is enabled or not". > > > > If you have a reason why that should not be the case, > > let me know. > > I still cannot understand this case. > > If the source has masterclock _disabled_, shouldn't it read kvmclock > from memory? But that's not what your patch does. To be clear, what > I mean is: > > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c > index 653b0b4..6f6e2dc 100644 > --- a/hw/i386/kvm/clock.c > +++ b/hw/i386/kvm/clock.c > @@ -97,6 +97,7 @@ static uint64_t kvm_get_clock(void) > fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret)); > abort(); > } > + s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE; I still don't understand the reasoning behind kvm_has_adjust_clock_stable() vs (flags & KVM_CLOCK_TSC_STABLE), but on either case, updating src_use_reliable_get_clock inside kvm_get_clock() looks like the right thing to do. > return data.clock; > } > > @@ -110,34 +111,19 @@ static void kvmclock_vm_state_change(void *opaque, int running, > > if (running) { > 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 last KVM_GET_CLOCK did not retrieve a reliable value, > + * we can't rely on the saved clock value. Just discard it and > + * read kvmclock value from memory > + */ > + if (!s->src_use_reliable_get_clock) { > + uint64_t pvclock_via_mem = kvmclock_current_nsec(s); > + if (pvclock_via_mem) { > + s->clock = pvclock_via_mem; > } > } This is equivalent to the logic I suggested on my reply to v3. Much simpler. > > - /* 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; > @@ -181,16 +168,6 @@ static void kvmclock_realize(DeviceState *dev, Error **errp) > { > 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); > } > -- Eduardo From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cBMvW-00014K-MJ for qemu-devel@nongnu.org; Mon, 28 Nov 2016 09:29:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cBMvT-0007wv-IL for qemu-devel@nongnu.org; Mon, 28 Nov 2016 09:29:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42722) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cBMvT-0007wc-95 for qemu-devel@nongnu.org; Mon, 28 Nov 2016 09:28:59 -0500 Date: Mon, 28 Nov 2016 12:28:55 -0200 From: Eduardo Habkost Message-ID: <20161128142855.GC14328@thinpad.lan.raisama.net> References: <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> <20161114145054.GA28663@amt.cnet> <67bffd95-2e4e-7273-c154-a3fdfe622387@redhat.com> <20161114154015.GA30048@amt.cnet> <20161114171318.GA6336@amt.cnet> <14044cda-054d-94eb-8d91-7ad3a1e0869e@redhat.com> <20161114181518.GA14076@amt.cnet> <20161117121637.GA13404@amt.cnet> <1023a283-77a7-45e5-8877-6264e08d0658@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1023a283-77a7-45e5-8877-6264e08d0658@redhat.com> Subject: Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Marcelo Tosatti , kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar On Mon, Nov 28, 2016 at 02:47:18PM +0100, Paolo Bonzini wrote: > > > On 17/11/2016 13:16, Marcelo Tosatti wrote: > > What QEMU wants is to use KVM_GET_CLOCK at pre_save independently > > of whether masterclock is enabled or not... it just depends > > on KVM_GET_CLOCK being correct for the masterclock case > > (108b249c453dd7132599ab6dc7e435a7036c193f). > > > > So a "reliable KVM_GET_CLOCK" (that does not timebackward > > when masterclock is enabled) is much simpler to userspace > > than "whether masterclock is enabled or not". > > > > If you have a reason why that should not be the case, > > let me know. > > I still cannot understand this case. > > If the source has masterclock _disabled_, shouldn't it read kvmclock > from memory? But that's not what your patch does. To be clear, what > I mean is: > > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c > index 653b0b4..6f6e2dc 100644 > --- a/hw/i386/kvm/clock.c > +++ b/hw/i386/kvm/clock.c > @@ -97,6 +97,7 @@ static uint64_t kvm_get_clock(void) > fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret)); > abort(); > } > + s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE; I still don't understand the reasoning behind kvm_has_adjust_clock_stable() vs (flags & KVM_CLOCK_TSC_STABLE), but on either case, updating src_use_reliable_get_clock inside kvm_get_clock() looks like the right thing to do. > return data.clock; > } > > @@ -110,34 +111,19 @@ static void kvmclock_vm_state_change(void *opaque, int running, > > if (running) { > 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 last KVM_GET_CLOCK did not retrieve a reliable value, > + * we can't rely on the saved clock value. Just discard it and > + * read kvmclock value from memory > + */ > + if (!s->src_use_reliable_get_clock) { > + uint64_t pvclock_via_mem = kvmclock_current_nsec(s); > + if (pvclock_via_mem) { > + s->clock = pvclock_via_mem; > } > } This is equivalent to the logic I suggested on my reply to v3. Much simpler. > > - /* 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; > @@ -181,16 +168,6 @@ static void kvmclock_realize(DeviceState *dev, Error **errp) > { > 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); > } > -- Eduardo