From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value Date: Fri, 12 May 2017 16:13:23 +0200 Message-ID: <20170512141322.GC2173@potion> References: <20170502213616.GA24837@amt.cnet> <2499ef65-1dfe-8460-ec41-661b05cc5023@redhat.com> <20170503134341.GB10468@amt.cnet> <20170510180430.GA2240@potion> <20170511153903.GC2308@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Paolo Bonzini , kvm-devel To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47844 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755963AbdELON0 (ORCPT ); Fri, 12 May 2017 10:13:26 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A96747AE8B for ; Fri, 12 May 2017 14:13:25 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20170511153903.GC2308@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 2017-05-11 12:39-0300, Marcelo Tosatti: > On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote: > > 2017-05-03 10:43-0300, Marcelo Tosatti: > > and the important fix for kvm master clock is the move of > > kvm_gen_update_masterclock() before we read the time. > > The rest is just a minor optimization that also ignores time since > > master_kernel_ns() and therefore pins user_ns.clock to a slightly > > earlier time. > > > > But all attention was given to the "minor optimization" -- have I missed > > something about the direct use of ka->master_kernel_ns? > > I haven't attempted to optimize anything. Not sure what you mean. I mean, why doesn't the patch look like this? diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 464da936c53d..8db1d09e59d7 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp, goto out; r = 0; + kvm_gen_update_masterclock(kvm); now_ns = get_kvmclock_ns(kvm); kvm->arch.kvmclock_offset += user_ns.clock - now_ns; - kvm_gen_update_masterclock(kvm); + kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE); break; } case KVM_GET_CLOCK: { > > disabled, but call to kvm_gen_update_masterclock() would enable it) > > The master clock will be updates with the kernel boot clock when a > > VCPU runs. This means that master clock and kernel boot clock will > > begin diverging at a later point, but the initial offset is the same. > > Userspace can only tell the difference by calling KVM_GET_CLOCK > > afterwards and seeing the stable bit unset. > > Guest is mostly unaffected -- the inaccuracy from calling > > KVM_SET_CLOCK is much bigger than accumulation of a slightly > > different frequency will in few jiffies. > > > > Do you see a reason to use kvm_gen_update_masterclock()? > > > > I think that the code could be just: > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 464da936c53d..f024216a858d 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -4177,7 +4177,7 @@ long kvm_arch_vm_ioctl(struct file *filp, > > r = 0; > > now_ns = get_kvmclock_ns(kvm); > > kvm->arch.kvmclock_offset += user_ns.clock - now_ns; > > - kvm_gen_update_masterclock(kvm); > > + kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE); > > No, this is wrong. kvm_gen_update_masterclock() must happen before > the assignment of kvm->arch.kvmclock_offset and get_kvmclock_ns(). > > (so that kvm_gen_update_masterclock sets ka->use_master_clock to true > and get_kvmclock_ns() enters this case: > > hv_clock.tsc_timestamp = ka->master_cycle_now; > hv_clock.system_time = ka->master_kernel_ns + > ka->kvmclock_offset; > spin_unlock(&ka->pvclock_gtod_sync_lock); > > kvm_get_time_scale(NSEC_PER_SEC, __this_cpu_read(cpu_tsc_khz) * > 1000LL, > &hv_clock.tsc_shift, > &hv_clock.tsc_to_system_mul); > return __pvclock_read_cycles(&hv_clock, rdtsc()); > > Makes sense? That doesn't explain the need for kvm_gen_update_masterclock(). kvm_gen_update_masterclock() moves the master clock base to the current kernel boot clock; the non-master clock case uses the same kernel boot clock to return the current kvmclock -- the computed offset is the same in both cases. Updating the master clock simplifies the case by resetting possible divergence (due to different frequencies) between master clock and kernel boot clock, but I'd still like to know (and document) if that is the reason. In what test does updating the master clock make a difference? Thanks.