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@xxxxxxxxxx> > > > > > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
