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

[Andrew Jones <drjones@redhat.com>]

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 <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);
> 

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