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

[Andrew Jones <drjones@redhat.com>]

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.

> +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