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:45:21 -0300 Message-ID: <20160609034521.GA15072@amt.cnet> References: <1464274195-31296-1-git-send-email-rkagan@virtuozzo.com> <20160529233844.GA14374@amt.cnet> <20160609032710.GA13318@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]:33991 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751664AbcFIDpn (ORCPT ); Wed, 8 Jun 2016 23:45:43 -0400 Content-Disposition: inline In-Reply-To: <20160609032710.GA13318@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jun 09, 2016 at 12:27:10AM -0300, Marcelo Tosatti wrote: > 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 Also please update the kvm-unit-test to check both ways (i did here and what happens is that): Case1: with frequency correction from vsyscall: a = read_shared_clock; b = rdmsr(REF_CLOCK); fail: a > b Case2: without frequency correction from vsyscall: a = rdmsr(REF_CLOCK); b = read_shared_clock; fail: b < a And the frequency correction advertised via syscall is the same as what get_kernel_ns() is using. Conclusion: update_wall_time() updates to monotonic clock do not match tsc*freqcorrection. So there is nothing you can do to keep these clocks in sync (other then checking what is the largest of them when reading REF_CLOCK).