From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: KVM: x86: add module parameter to disable periodic kvmclock sync Date: Thu, 13 Nov 2014 09:40:41 +0100 Message-ID: <20141113084040.GA3193@hawk.usersys.redhat.com> References: <20141113024439.GA7402@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Paolo Bonzini , kvm-devel To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43363 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751013AbaKMIkp (ORCPT ); Thu, 13 Nov 2014 03:40:45 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAD8ei3v016100 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 13 Nov 2014 03:40:44 -0500 Content-Disposition: inline In-Reply-To: <20141113024439.GA7402@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Nov 13, 2014 at 12:44:39AM -0200, Marcelo Tosatti wrote: > > The periodic kvmclock sync can be an undesired source of latencies. > > Signed-off-by: Marcelo Tosatti > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 0033df3..be56fd3 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -98,6 +98,9 @@ module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR); > unsigned int min_timer_period_us = 500; > module_param(min_timer_period_us, uint, S_IRUGO | S_IWUSR); > > +static bool kvmclock_periodic_sync = 1; Using 'true' would look nicer. > +module_param(kvmclock_periodic_sync, bool, S_IRUGO | S_IWUSR); > + > bool kvm_has_tsc_control; > EXPORT_SYMBOL_GPL(kvm_has_tsc_control); > u32 kvm_max_guest_tsc_khz; > @@ -1718,7 +1721,8 @@ static void kvmclock_sync_fn(struct work_struct *work) > struct kvm *kvm = container_of(ka, struct kvm, arch); > > schedule_delayed_work(&kvm->arch.kvmclock_update_work, 0); > - schedule_delayed_work(&kvm->arch.kvmclock_sync_work, > + if (kvmclock_periodic_sync) > + schedule_delayed_work(&kvm->arch.kvmclock_sync_work, > KVMCLOCK_SYNC_PERIOD); > } The above hunk shouldn't be necessary, as we'll never get there if we don't do the first scheduling with the below hunk. > > @@ -6971,7 +6975,8 @@ int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) > kvm_write_tsc(vcpu, &msr); > vcpu_put(vcpu); > > - schedule_delayed_work(&kvm->arch.kvmclock_sync_work, > + if (kvmclock_periodic_sync) > + schedule_delayed_work(&kvm->arch.kvmclock_sync_work, > KVMCLOCK_SYNC_PERIOD); > > return r; > > I'm not opposed to making this optional, but just curious. Were general use cases getting adversely affected? Or is this part of some RT work trying to kill as many sources of asynchronous latency as possible? drew