From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration Date: Mon, 28 Nov 2016 14:36:50 -0200 Message-ID: <20161128163648.GA4028@amt.cnet> 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: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar , Eduardo Habkost To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:48920 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933239AbcK1QrM (ORCPT ); Mon, 28 Nov 2016 11:47:12 -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? If the source masterclock is disabled, then the guest does not enable the optimization to not use a global variable to guarantee monotonicity. Therefore there will be no time backwards events (the timer backwards events crashed guests, and are the reason for reading from guest memory). So if there are no flaws in the reasoning above, no, there is no need to read from memory if masterclock is disabled. Can you state the reasons why you think it should be enabled? > But that's not what your patch does. Its on purpose with the data available. > 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; > 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; > } > } > > - /* 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); > } > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60579) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cBP56-0000nz-9T for qemu-devel@nongnu.org; Mon, 28 Nov 2016 11:47:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cBP52-00039n-AW for qemu-devel@nongnu.org; Mon, 28 Nov 2016 11:47:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47020) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cBP52-00039T-2P for qemu-devel@nongnu.org; Mon, 28 Nov 2016 11:47:00 -0500 Date: Mon, 28 Nov 2016 14:36:50 -0200 From: Marcelo Tosatti Message-ID: <20161128163648.GA4028@amt.cnet> 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: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar , Eduardo Habkost 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? If the source masterclock is disabled, then the guest does not enable the optimization to not use a global variable to guarantee monotonicity. Therefore there will be no time backwards events (the timer backwards events crashed guests, and are the reason for reading from guest memory). So if there are no flaws in the reasoning above, no, there is no need to read from memory if masterclock is disabled. Can you state the reasons why you think it should be enabled? > But that's not what your patch does. Its on purpose with the data available. > 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; > 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; > } > } > > - /* 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); > } >