[PATCH 0/2 v2] x86: kvm: global clock updates

[PATCH 0/2 v2] x86: kvm: global clock updates

[Andrew Jones]

This patch series addresses two issues with global clock updates.
The first fixes a bug found on hosts that have a tsc marked as
unstable. As global clock updates get triggered on every vcpu load
in these cases, guests with a large number of vcpus have their
progress nearly halted. The fix for that bug should also go to
stable. The second patch in this series ensures that NTP corrections
on the host, as well as on guests with all vcpus pinned, will be
propagated periodically. That patch improves things, but doesn't
fix a bug, thus it can be merged at a later time than the first.
I've posted them together as a series, as the second one builds
on the first.

Andrew Jones (2):
  x86: kvm: rate-limit global clock updates
  x86: kvm: introduce periodic global clock updates

 arch/x86/include/asm/kvm_host.h |  2 +
 arch/x86/kvm/x86.c              | 92 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 90 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[PATCH 1/2 v2] x86: kvm: rate-limit global clock updates

[Andrew Jones]

When we update a vcpu's local clock it may pick up an NTP correction.
We can't wait an indeterminate amount of time for other vcpus to pick
up that correction, so commit 0061d53daf26f introduced a global clock
update. However, we can't request a global clock update on every vcpu
load either (which is what happens if the tsc is marked as unstable).
The solution is to rate-limit the global clock updates. Marcelo
calculated that we should delay the global clock updates no more
than 0.1s as follows:

Assume an NTP correction c is applied to one vcpu, but not the other,
then in n seconds the delta of the vcpu system_timestamps will be
c * n. If we assume a correction of 500ppm (worst-case), then the two
vcpus will diverge 50us in 0.1s, which is a considerable amount.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
v2: switch from kvm_{get,put}_kvm() to cancel_delayed_work_sync()

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/x86.c              | 27 +++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e714f8c08ccf2..9aa09d330a4b5 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -598,6 +598,7 @@ struct kvm_arch {
 	bool use_master_clock;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
+	struct delayed_work kvmclock_update_work;
 
 	struct kvm_xen_hvm_config xen_hvm_config;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4cca45853dfeb..e656719750fdf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1628,14 +1628,21 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
  * the others.
  *
  * So in those cases, request a kvmclock update for all vcpus.
- * The worst case for a remote vcpu to update its kvmclock
- * is then bounded by maximum nohz sleep latency.
+ * We need to rate-limit these requests though, as they can
+ * considerably slow guests that have a large number of vcpus.
+ * The time for a remote vcpu to update its kvmclock is bound
+ * by the delay we use to rate-limit the updates.
  */
 
-static void kvm_gen_kvmclock_update(struct kvm_vcpu *v)
+#define KVMCLOCK_UPDATE_DELAY msecs_to_jiffies(100)
+
+static void kvmclock_update_fn(struct work_struct *work)
 {
 	int i;
-	struct kvm *kvm = v->kvm;
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct kvm_arch *ka = container_of(dwork, struct kvm_arch,
+					   kvmclock_update_work);
+	struct kvm *kvm = container_of(ka, struct kvm, arch);
 	struct kvm_vcpu *vcpu;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
@@ -1644,6 +1651,15 @@ static void kvm_gen_kvmclock_update(struct kvm_vcpu *v)
 	}
 }
 
+static void kvm_gen_kvmclock_update(struct kvm_vcpu *v)
+{
+	struct kvm *kvm = v->kvm;
+
+	set_bit(KVM_REQ_CLOCK_UPDATE, &v->requests);
+	schedule_delayed_work(&kvm->arch.kvmclock_update_work,
+					KVMCLOCK_UPDATE_DELAY);
+}
+
 static bool msr_mtrr_valid(unsigned msr)
 {
 	switch (msr) {
@@ -7019,6 +7035,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 	pvclock_update_vm_gtod_copy(kvm);
 
+	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
+
 	return 0;
 }
 
@@ -7056,6 +7074,7 @@ static void kvm_free_vcpus(struct kvm *kvm)
 
 void kvm_arch_sync_events(struct kvm *kvm)
 {
+	cancel_delayed_work_sync(&kvm->arch.kvmclock_update_work);
 	kvm_free_all_assigned_devices(kvm);
 	kvm_free_pit(kvm);
 }
-- 
1.8.1.4

[PATCH 2/2 v2] x86: kvm: introduce periodic global clock updates

[Andrew Jones]

commit 0061d53daf26f introduced a mechanism to execute a global clock
update for a vm. We can apply this periodically in order to propagate
host NTP corrections. Also, if all vcpus of a vm are pinned, then
without an additional trigger, no guest NTP corrections can propagate
either, as the current trigger is only vcpu cpu migration.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
v2: greatly simplify things by creating per vm sync work, and not
    worrying about vms making simultaneous update requests.

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/x86.c              | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9aa09d330a4b5..85be627ef5de2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -599,6 +599,7 @@ struct kvm_arch {
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 	struct delayed_work kvmclock_update_work;
+	struct delayed_work kvmclock_sync_work;
 
 	struct kvm_xen_hvm_config xen_hvm_config;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e656719750fdf..17c57c1ed1835 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1660,6 +1660,20 @@ static void kvm_gen_kvmclock_update(struct kvm_vcpu *v)
 					KVMCLOCK_UPDATE_DELAY);
 }
 
+#define KVMCLOCK_SYNC_PERIOD (300 * HZ)
+
+static void kvmclock_sync_fn(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct kvm_arch *ka = container_of(dwork, struct kvm_arch,
+					   kvmclock_sync_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,
+					KVMCLOCK_SYNC_PERIOD);
+}
+
 static bool msr_mtrr_valid(unsigned msr)
 {
 	switch (msr) {
@@ -6733,6 +6747,7 @@ int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 {
 	int r;
 	struct msr_data msr;
+	struct kvm *kvm = vcpu->kvm;
 
 	r = vcpu_load(vcpu);
 	if (r)
@@ -6743,6 +6758,9 @@ 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,
+					KVMCLOCK_SYNC_PERIOD);
+
 	return r;
 }
 
@@ -7036,6 +7054,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	pvclock_update_vm_gtod_copy(kvm);
 
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
+	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
 
 	return 0;
 }
@@ -7074,6 +7093,7 @@ static void kvm_free_vcpus(struct kvm *kvm)
 
 void kvm_arch_sync_events(struct kvm *kvm)
 {
+	cancel_delayed_work_sync(&kvm->arch.kvmclock_sync_work);
 	cancel_delayed_work_sync(&kvm->arch.kvmclock_update_work);
 	kvm_free_all_assigned_devices(kvm);
 	kvm_free_pit(kvm);
-- 
1.8.1.4

Re: [PATCH 0/2 v2] x86: kvm: global clock updates

[Marcelo Tosatti]

On Fri, Feb 28, 2014 at 12:52:53PM +0100, Andrew Jones wrote:
> This patch series addresses two issues with global clock updates.
> The first fixes a bug found on hosts that have a tsc marked as
> unstable. As global clock updates get triggered on every vcpu load
> in these cases, guests with a large number of vcpus have their
> progress nearly halted. The fix for that bug should also go to
> stable. The second patch in this series ensures that NTP corrections
> on the host, as well as on guests with all vcpus pinned, will be
> propagated periodically. That patch improves things, but doesn't
> fix a bug, thus it can be merged at a later time than the first.
> I've posted them together as a series, as the second one builds
> on the first.
> 
> Andrew Jones (2):
>   x86: kvm: rate-limit global clock updates
>   x86: kvm: introduce periodic global clock updates
> 
>  arch/x86/include/asm/kvm_host.h |  2 +
>  arch/x86/kvm/x86.c              | 92 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 90 insertions(+), 4 deletions(-)
> 
> -- 
> 1.8.1.4

Looks good. 

Re: [PATCH 0/2 v2] x86: kvm: global clock updates

[Paolo Bonzini]

Il 28/02/2014 15:52, Marcelo Tosatti ha scritto:
> On Fri, Feb 28, 2014 at 12:52:53PM +0100, Andrew Jones wrote:
>> This patch series addresses two issues with global clock updates.
>> The first fixes a bug found on hosts that have a tsc marked as
>> unstable. As global clock updates get triggered on every vcpu load
>> in these cases, guests with a large number of vcpus have their
>> progress nearly halted. The fix for that bug should also go to
>> stable. The second patch in this series ensures that NTP corrections
>> on the host, as well as on guests with all vcpus pinned, will be
>> propagated periodically. That patch improves things, but doesn't
>> fix a bug, thus it can be merged at a later time than the first.
>> I've posted them together as a series, as the second one builds
>> on the first.
>>
>> Andrew Jones (2):
>>   x86: kvm: rate-limit global clock updates
>>   x86: kvm: introduce periodic global clock updates
>>
>>  arch/x86/include/asm/kvm_host.h |  2 +
>>  arch/x86/kvm/x86.c              | 92 +++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 90 insertions(+), 4 deletions(-)
>>
>> --
>> 1.8.1.4
>
> Looks good.
>

Applied to kvm/queue, thanks to both.

Paolo