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

PATCH Marcelo Tosatti 5 participants 10 messages Nov 13, 2014 – Nov 14, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2014-11-13 02:44 UTC #0 lore raw

The periodic kvmclock sync can be an undesired source of latencies.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +7 -2

index 0033df3..be56fd3 100644
@@ -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;

Andrew Jones

2014-11-13 08:40 UTC #1 lore raw

Quoted 10 lines · Marcelo Tosatti

The periodic kvmclock sync can be an undesired source of latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +0 -0
index 0033df3..be56fd3 100644

arch/x86/kvm/x86.c module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR); · line 101

Quoted 4 lines

arch/x86/kvm/x86.c +1 -0

@@ -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.

Quoted 6 lines

arch/x86/kvm/x86.c +2 -0

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

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 1727

Quoted 8 lines

arch/x86/kvm/x86.c +2 -1

@@ -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.

arch/x86/kvm/x86.c int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) · line 6982

Quoted 13 lines

arch/x86/kvm/x86.c +2 -1

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

Andrew Jones

2014-11-13 10:44 UTC #2 lore raw

Quoted 14 lines · Andrew Jones

Quoted 13 lines · Marcelo Tosatti
The periodic kvmclock sync can be an undesired source of latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +0 -0
index 0033df3..be56fd3 100644
@@ -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);
 

Quoted 3 lines

arch/x86/kvm/x86.c module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR); · line 101
Quoted 1 line
arch/x86/kvm/x86.c +1 -0
+static bool kvmclock_periodic_sync = 1;
Using 'true' would look nicer.

Ahh, disregard this comment. 1 matches what the user would input.

Quoted 9 lines

arch/x86/kvm/x86.c +2 -0

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

Quoted 9 lines

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 1727
Quoted 6 lines
arch/x86/kvm/x86.c +2 -1
 	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.

Quoted 6 lines

arch/x86/kvm/x86.c +0 -0

@@ -6971,7 +6975,8 @@ int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	kvm_write_tsc(vcpu, &msr);
 	vcpu_put(vcpu);

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) · line 6982
Quoted 8 lines
arch/x86/kvm/x86.c +2 -1
-	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);

Quoted 7 lines

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

Andrew Jones

2014-11-13 11:32 UTC #3 lore raw

Quoted 65 lines · Andrew Jones

Quoted 14 lines · Andrew Jones
Quoted 13 lines · Marcelo Tosatti
The periodic kvmclock sync can be an undesired source of latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +0 -0
index 0033df3..be56fd3 100644
@@ -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);
 
Quoted 3 lines
arch/x86/kvm/x86.c module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR); · line 101
Quoted 1 line
arch/x86/kvm/x86.c +1 -0
+static bool kvmclock_periodic_sync = 1;
Using 'true' would look nicer.
Ahh, disregard this comment. 1 matches what the user would input.
Quoted 9 lines
Quoted 9 lines
arch/x86/kvm/x86.c +2 -0
+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);
 
Quoted 9 lines
arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 1727
Quoted 6 lines
arch/x86/kvm/x86.c +2 -1
 	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.
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/x86.c +0 -0
@@ -6971,7 +6975,8 @@ int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	kvm_write_tsc(vcpu, &msr);
 	vcpu_put(vcpu);
 
Quoted 8 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) · line 6982
Quoted 8 lines
arch/x86/kvm/x86.c +2 -1
-	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

Review static void kvmclock_sync_fn(struct work_struct *work) · line 1725

Quoted 9 lines

(quoted diff) +3 -0

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

Quoted 8 lines

Quoted 8 lines
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

Radim Krčmář

2014-11-13 17:47 UTC #4 lore raw

2014-11-13 12:32+0100, Andrew Jones:

Quoted 4 lines · Andrew Jones

Quoted 3 lines · Andrew Jones
Quoted 2 lines · Marcelo Tosatti
Quoted 1 line
The periodic kvmclock sync can be an undesired source of latencies.

[...]

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+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'.)

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 1723

Quoted 9 lines

arch/x86/kvm/x86.c +2 -1

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

Paolo Bonzini

maintainer

2014-11-13 17:47 UTC #5 lore raw

Quoted 5 lines · Radim Krčmář

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

Andrew Jones

2014-11-13 17:58 UTC #6 lore raw

Quoted 11 lines · Paolo Bonzini

Quoted 5 lines · Radim Krčmář
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

Radim Krčmář

2014-11-13 18:24 UTC #7 lore raw

2014-11-13 18:57+0100, Andrew Jones:

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · Radim Krčmář
Quoted 5 lines
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.)

Quoted 6 lines

Quoted 2 lines
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'.)

Marcelo Tosatti

author

2014-11-14 18:52 UTC #8 lore raw

In reply to Andrew Jones

Quoted 46 lines · Andrew Jones

Quoted 10 lines · Marcelo Tosatti
The periodic kvmclock sync can be an undesired source of latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +0 -0
index 0033df3..be56fd3 100644
arch/x86/kvm/x86.c module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR); · line 101
Quoted 4 lines
arch/x86/kvm/x86.c +1 -0
@@ -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.
Quoted 6 lines
arch/x86/kvm/x86.c +2 -0
+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;
arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 1727
Quoted 8 lines
arch/x86/kvm/x86.c +2 -1
@@ -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.
Quoted 10 lines
arch/x86/kvm/x86.c +2 -1
@@ -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);
 

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) · line 6982
Quoted 3 lines
arch/x86/kvm/x86.c +0 -0
 	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.

Michael Tokarev

2014-11-13 14:15 UTC #9 lore raw

In reply to Marcelo Tosatti

Quoted 1 line · Marcelo Tosatti

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