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 12:32:20 +0100 Message-ID: <20141113113219.GA5233@hawk.usersys.redhat.com> References: <20141113024439.GA7402@amt.cnet> <20141113084040.GA3193@hawk.usersys.redhat.com> <20141113104401.GA4325@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]:43660 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932329AbaKMLcY (ORCPT ); Thu, 13 Nov 2014 06:32:24 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sADBWOa3004080 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 13 Nov 2014 06:32:24 -0500 Content-Disposition: inline In-Reply-To: <20141113104401.GA4325@hawk.usersys.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Nov 13, 2014 at 11:44:02AM +0100, Andrew Jones wrote: > 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); > Oh man... I'll reply correctly eventually. The above should of course be @@ -1717,7 +1717,8 @@ static void kvmclock_sync_fn(struct work_struct *work) kvmclock_sync_work); struct kvm *kvm = container_of(ka, struct kvm, arch); - schedule_delayed_work(&kvm->arch.kvmclock_update_work, 0); + if (kvmclock_periodic_sync) + 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