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: Thu, 17 Nov 2016 10:16:40 -0200 Message-ID: <20161117121637.GA13404@amt.cnet> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Eduardo Habkost , kvm@vger.kernel.org, Juan Quintela , Radim Krcmar , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: <20161114181518.GA14076@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 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.