Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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. +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. -- 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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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. +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 -- 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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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); 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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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 -- 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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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.)
--
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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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 -- 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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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 -- 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
Re: KVM: x86: add module parameter to disable periodic kvmclock sync
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'.) -- 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
