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 <mtosa...@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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
