From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration Date: Thu, 17 Nov 2016 14:03:36 +0100 Message-ID: <07f60f05-4e4a-7217-23dc-d687423736b3@redhat.com> References: <20161114140028.GA25935@amt.cnet> <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> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: Eduardo Habkost , kvm@vger.kernel.org, Radim Krcmar , Juan Quintela , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" To: Marcelo Tosatti Return-path: In-Reply-To: <20161117121637.GA13404@amt.cnet> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org On 17/11/2016 13:16, Marcelo Tosatti wrote: > On Mon, Nov 14, 2016 at 04:15:18PM -0200, Marcelo Tosatti wrote: >> On Mon, Nov 14, 2016 at 06:20:29PM +0100, Paolo Bonzini wrote: >>> >>> >>> On 14/11/2016 18:13, Marcelo Tosatti wrote: >>>> On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote: >>>>> >>>>> >>>>> On 14/11/2016 16:40, Marcelo Tosatti wrote: >>>>>> static bool kvmclock_src_use_reliable_get_clock(void *opaque) >>>>>> { >>>>>> KVMClockState *s = opaque; >>>>>> >>>>>> /* >>>>>> * 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; >>>>>> } >>>>>> >>>>>> return s->mach_use_reliable_get_clock; >>>>>> } >>>>>> >>>>>> >>>>>> Ah, OK, done. >>>>> >>>>> s->src_use_reliable_get_clock should not be set with >>>>> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK. >>>> >>>> Well, thats not right: What matters is the presence of get_kvmclock_ns >>>> which returns a value that the guest sees. >>>> >>>> get_kernel_monotonic_clock() + kvmclock_offset + >>>> (rdtsc() - tsc_timestamp) >>>> >>>> IOW what the guest sees. And you changed that in >>>> >>>> commit 108b249c453dd7132599ab6dc7e435a7036c193f >>>> Author: Paolo Bonzini >>>> Date: Thu Sep 1 14:21:03 2016 +0200 >>>> >>>> KVM: x86: introduce get_kvmclock_ns >>>> >>>> And the correct behaviour (once KVM_GET_CLOCK is fixed per >>>> previous message to return rdtsc - tsc_timestamp for the >>>> non masterclock case) depends on this commit above, >>>> not on masterclock. >>> >>> This commit in turn only gets the correct behavior if >>> "vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be >>> changed soon to ka->use_masterclock). KVM_CHECK_EXTENSION can still >>> return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled, >>> because KVM_CHECK_EXTENSION only tells you which flags are known for >>> this version of the KVM module. >> >> 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. >> >>> To see if the masterclock is enabled _now_, you need to check what >>> KVM_GET_CLOCK sets in the flags. From the KVM_CLOCK_TSC_STABLE patch: >>> >>> user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0; >> >> Again, whether masterclock is enable is independent of >> being able to use KVM_GET_CLOCK at pre_save. > > Is this point OK ? > > Using > > break; > + case KVM_CAP_ADJUST_CLOCK: > + r = KVM_CLOCK_TSC_STABLE; > + break; > > To infer whether KVM_GET_CLOCK is fixed for the monotonic case. Yes, I still haven't digested why it is correct (I need to read again what you wrote), but it is indeed correct to use KVM_CAP_ADJUST_CLOCK this way. Paolo