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 11:44:02 +0100 Message-ID: <20141113104401.GA4325@hawk.usersys.redhat.com> References: <20141113024439.GA7402@amt.cnet> <20141113084040.GA3193@hawk.usersys.redhat.com> 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]:52181 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932355AbaKMKoH (ORCPT ); Thu, 13 Nov 2014 05:44:07 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sADAi6rJ022249 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 13 Nov 2014 05:44:06 -0500 Content-Disposition: inline In-Reply-To: <20141113084040.GA3193@hawk.usersys.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Nov 13, 2014 at 09:40:41AM +0100, Andrew Jones wrote: > 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. Ahh, disregard this comment. 1 matches what the user would input. > > > +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. Disregard this comment too. I didn't pay enough attention to the module param permissions. We definitely need this here to modify behaviour of running VMs when the parameter gets updated with writes to sysfs. > > > > > @@ -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; > > > > But... if the kvmclock_periodic_sync is false here, then it won't matter if we turn it on later. Maybe we don't care about that, but if we do, then we should remove this hunk, and also change the hunk above to be @@ -1717,6 +1717,9 @@ static void kvmclock_sync_fn(struct work_struct *work) kvmclock_sync_work); struct kvm *kvm = container_of(ka, struct kvm, arch); + if (!kvmclock_periodic_sync) + return; + schedule_delayed_work(&kvm->arch.kvmclock_update_work, 0); schedule_delayed_work(&kvm->arch.kvmclock_sync_work, KVMCLOCK_SYNC_PERIOD); > > 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