From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH] x86/kvm: fix condition to update kvm master clocks Date: Thu, 9 Jun 2016 00:27:10 -0300 Message-ID: <20160609032710.GA13318@amt.cnet> References: <1464274195-31296-1-git-send-email-rkagan@virtuozzo.com> <20160529233844.GA14374@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, "Denis V. Lunev" , Owen Hofmann , Paolo Bonzini To: Roman Kagan Return-path: Received: from mx1.redhat.com ([209.132.183.28]:36894 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1423052AbcFIDbX (ORCPT ); Wed, 8 Jun 2016 23:31:23 -0400 Content-Disposition: inline In-Reply-To: <20160529233844.GA14374@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, May 29, 2016 at 08:38:44PM -0300, Marcelo Tosatti wrote: > On Thu, May 26, 2016 at 05:49:55PM +0300, Roman Kagan wrote: > > The condition to schedule per-VM master clock updates is inversed; as a > > result the master clocks are never updated and the kvm_clock drift WRT > > host's NTP-disciplined clock (which was the motivation to introduce this > > machinery in the first place) still remains. > > > > Fix it, and reword the comment to make it more apparent what the desired > > behavior is. > > > > Cc: Owen Hofmann > > Cc: Paolo Bonzini > > Cc: Marcelo Tosatti > > Signed-off-by: Roman Kagan > > --- > > arch/x86/kvm/x86.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index c805cf4..d8f591c 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -5810,10 +5810,10 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused, > > > > update_pvclock_gtod(tk); > > > > - /* disable master clock if host does not trust, or does not > > - * use, TSC clocksource > > + /* only schedule per-VM master clock updates if the host uses TSC and > > + * there's at least one VM in need of an update > > */ > > - if (gtod->clock.vclock_mode != VCLOCK_TSC && > > + if (gtod->clock.vclock_mode == VCLOCK_TSC && > > atomic_read(&kvm_guest_has_master_clock) != 0) > > queue_work(system_long_wq, &pvclock_gtod_work); > > > > -- > > 2.5.5 > > NAK, as stated this leaves clocks out of sync between execution of > pvclock_gtod_notify and pvclock_gtod_work execution. Ok, its not feasible to keep both REF_CLOCK (MSR) and shared memory (REF_PAGE) clocks in sync. Even if the frequency correction is the same for both, the kernel updates monotonic clock differently than the stated frequency that is: monotonic clock (advertised via vsyscall notifier to use mult/freq pair) != tsc*freq So the best solution IMO is to: reads of guest clock = max(shared memory clock, get_kernel_ns() + kvmclock_offset) Where reads of guest clock include: 1) kvm_get_time_and_clockread (updates to kvmclock areas), 2) rdmsr(REF_CLOCK). Unless someone has a better idea, Roman, can you update your patch to include such solution? for kvm_get_time_and_clockread, can fast forward kvmclock_offset so that kvmclock_offset + get_kernel_ns() = shared memory clock