Re: KVM: x86: add module parameter to disable periodic kvmclock sync

[Andrew Jones <drjones@redhat.com>]

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 <mtosatti@redhat.com>
> > 
> > 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