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

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

[Marcelo Tosatti]

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;
+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);
 }
 
@@ -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;

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

[Andrew Jones]

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

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

[Andrew Jones]

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

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

[Andrew Jones]

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

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

[Michael Tokarev]

13.11.2014 05:44, Marcelo Tosatti wrote:
> The periodic kvmclock sync can be an undesired source of latencies.

Shouldn't this be a per-vm property, not global host property?
Maybe it's better to control frequency of syncs (with 0=disabled)?

Thanks,

/mjt

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

[Radim Krčmář]

2014-11-13 12:32+0100, Andrew Jones:
> 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.
[...]
> > > > +static bool kvmclock_periodic_sync = 1;
> > > 
> > > Using 'true' would look nicer.
> > 
> > Ahh, disregard this comment. 1 matches what the user would input.

The output is always Y|N and the user can input that as well.
(bool = {true, false}, so I'd prefer 'true'.)

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

Yes, or add callbacks to sysfs writes that would schedule/cancel this
work.  (But having a for_every_vm loop is quite ugly.)

I'd be happy with a 'const kvmclock_periodic_sync'.
(Having useless timers is weird if we care about latencies.)

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

[Paolo Bonzini]

On 13/11/2014 18:46, Radim Krčmář wrote:
> Yes, or add callbacks to sysfs writes that would schedule/cancel this
> work.  (But having a for_every_vm loop is quite ugly.)
> 
> I'd be happy with a 'const kvmclock_periodic_sync'.
> (Having useless timers is weird if we care about latencies.)

I agree.

Paolo

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

[Andrew Jones]

On Thu, Nov 13, 2014 at 06:47:40PM +0100, Paolo Bonzini wrote:
> 
> 
> On 13/11/2014 18:46, Radim Krčmář wrote:
> > Yes, or add callbacks to sysfs writes that would schedule/cancel this
> > work.  (But having a for_every_vm loop is quite ugly.)
> > 
> > I'd be happy with a 'const kvmclock_periodic_sync'.
> > (Having useless timers is weird if we care about latencies.)
> 
> I agree.
> 

yeah, I guess Marcelo's thought was that users may turn it
off with sysfs, but once off, it'll never get to come back.
In that case the original patch is perfectly fine as is.

drew

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

[Radim Krčmář]

2014-11-13 18:57+0100, Andrew Jones:
> On Thu, Nov 13, 2014 at 06:47:40PM +0100, Paolo Bonzini wrote:
> > 
> > 
> > On 13/11/2014 18:46, Radim Krčmář wrote:
> > > Yes, or add callbacks to sysfs writes that would schedule/cancel this
> > > work.  (But having a for_every_vm loop is quite ugly.)
> > > 
> > > I'd be happy with a 'const kvmclock_periodic_sync'.
> > > (Having useless timers is weird if we care about latencies.)
> > 
> > I agree.
> > 
> 
> yeah, I guess Marcelo's thought was that users may turn it
> off with sysfs, but once off, it'll never get to come back.
> In that case the original patch is perfectly fine as is.

(Good point, I never thought of that.)

300 second "race", we'd want to have both under the 'if' at least.
Explicitly cancelling is better IMO.

(This would make more sense with the previously mentioned per-vm
 property, if we had those without ioctls, and if the toggle was turned
 read-only after it has been changed to 'N'.)

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

[Marcelo Tosatti]

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

The latter.