* [PATCH v2 0/2] maintaining of kvm_clock stability on guest vCPU hotplug
@ 2017-04-07 9:06 Denis Plotnikov
2017-04-07 9:06 ` [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code Denis Plotnikov
2017-04-07 9:06 ` [PATCH 2/2] KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug Denis Plotnikov
0 siblings, 2 replies; 5+ messages in thread
From: Denis Plotnikov @ 2017-04-07 9:06 UTC (permalink / raw)
To: pbonzini, rkrcmar, mtosatti, kvm; +Cc: rkagan, den, dplotnikov
new in v2:
- fix possible wraparound on the synchronization detection
- lines exceeding 80 chars are re-formatted
- excess parentheses are removed
======================
It was found, that after adding a new vCPU to a running VM (CPU hotplug)
kvm_clock lost the stability property in case of using it. This happened
because KVM didn't treated cpu hotplug as a special case of TSC
synchronization.
This patch series adds cpu hotplug to the TSC synchronization cases.
Denis Plotnikov (2):
KVM: x86: remaster kvm_write_tsc code
KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug
arch/x86/kvm/x86.c | 61 ++++++++++++++++++++----------------------------------
1 file changed, 22 insertions(+), 39 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code
2017-04-07 9:06 [PATCH v2 0/2] maintaining of kvm_clock stability on guest vCPU hotplug Denis Plotnikov
@ 2017-04-07 9:06 ` Denis Plotnikov
2017-04-07 9:06 ` [PATCH 2/2] KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug Denis Plotnikov
1 sibling, 0 replies; 5+ messages in thread
From: Denis Plotnikov @ 2017-04-07 9:06 UTC (permalink / raw)
To: pbonzini, rkrcmar, mtosatti, kvm; +Cc: rkagan, den, dplotnikov
Reuse existing code instead of using inline asm.
Make the code more concise and clear in the TSC
synchronization part.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
arch/x86/kvm/x86.c | 52 +++++++++++++---------------------------------------
1 file changed, 13 insertions(+), 39 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..c59e38f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1444,10 +1444,10 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
struct kvm *kvm = vcpu->kvm;
u64 offset, ns, elapsed;
unsigned long flags;
- s64 usdiff;
bool matched;
bool already_matched;
u64 data = msr->data;
+ bool synchronizing = false;
raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
offset = kvm_compute_tsc_offset(vcpu, data);
@@ -1455,51 +1455,25 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
elapsed = ns - kvm->arch.last_tsc_nsec;
if (vcpu->arch.virtual_tsc_khz) {
- int faulted = 0;
-
- /* n.b - signed multiplication and division required */
- usdiff = data - kvm->arch.last_tsc_write;
-#ifdef CONFIG_X86_64
- usdiff = (usdiff * 1000) / vcpu->arch.virtual_tsc_khz;
-#else
- /* do_div() only does unsigned */
- asm("1: idivl %[divisor]\n"
- "2: xor %%edx, %%edx\n"
- " movl $0, %[faulted]\n"
- "3:\n"
- ".section .fixup,\"ax\"\n"
- "4: movl $1, %[faulted]\n"
- " jmp 3b\n"
- ".previous\n"
-
- _ASM_EXTABLE(1b, 4b)
-
- : "=A"(usdiff), [faulted] "=r" (faulted)
- : "A"(usdiff * 1000), [divisor] "rm"(vcpu->arch.virtual_tsc_khz));
-
-#endif
- do_div(elapsed, 1000);
- usdiff -= elapsed;
- if (usdiff < 0)
- usdiff = -usdiff;
-
- /* idivl overflow => difference is larger than USEC_PER_SEC */
- if (faulted)
- usdiff = USEC_PER_SEC;
- } else
- usdiff = USEC_PER_SEC; /* disable TSC match window below */
+ u64 tsc_exp = kvm->arch.last_tsc_write +
+ nsec_to_cycles(vcpu, elapsed);
+ u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
+ /*
+ * Special case: TSC write with a small delta (1 second)
+ * of virtual cycle time against real time is
+ * interpreted as an attempt to synchronize the CPU.
+ */
+ synchronizing = data < tsc_exp + tsc_hz &&
+ data + tsc_hz > tsc_exp;
+ }
/*
- * Special case: TSC write with a small delta (1 second) of virtual
- * cycle time against real time is interpreted as an attempt to
- * synchronize the CPU.
- *
* For a reliable TSC, we can match TSC offsets, and for an unstable
* TSC, we add elapsed time in this computation. We could let the
* compensation code attempt to catch up if we fall behind, but
* it's better to try to match offsets from the beginning.
*/
- if (usdiff < USEC_PER_SEC &&
+ if (synchronizing &&
vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
if (!check_tsc_unstable()) {
offset = kvm->arch.cur_tsc_offset;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug
2017-04-07 9:06 [PATCH v2 0/2] maintaining of kvm_clock stability on guest vCPU hotplug Denis Plotnikov
2017-04-07 9:06 ` [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code Denis Plotnikov
@ 2017-04-07 9:06 ` Denis Plotnikov
1 sibling, 0 replies; 5+ messages in thread
From: Denis Plotnikov @ 2017-04-07 9:06 UTC (permalink / raw)
To: pbonzini, rkrcmar, mtosatti, kvm; +Cc: rkagan, den, dplotnikov
VCPU TSC synchronization is perfromed in kvm_write_tsc() when the TSC
value being set is within 1 second from the expected, as obtained by
extrapolating of the TSC in already synchronized VCPUs.
This is naturally achieved on all VCPUs at VM start and resume;
however on VCPU hotplug it is not: the newly added VCPU is created
with TSC == 0 while others are well ahead.
To compensate for that, consider host-initiated kvm_write_tsc() with
TSC == 0 a special case requiring synchronization regardless of the
current TSC on other VCPUs.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
arch/x86/kvm/x86.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c59e38f..cf67091 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1455,16 +1455,25 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
elapsed = ns - kvm->arch.last_tsc_nsec;
if (vcpu->arch.virtual_tsc_khz) {
- u64 tsc_exp = kvm->arch.last_tsc_write +
- nsec_to_cycles(vcpu, elapsed);
- u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
- /*
- * Special case: TSC write with a small delta (1 second)
- * of virtual cycle time against real time is
- * interpreted as an attempt to synchronize the CPU.
- */
- synchronizing = data < tsc_exp + tsc_hz &&
- data + tsc_hz > tsc_exp;
+ if (data == 0 && msr->host_initiated) {
+ /*
+ * detection of vcpu initialization -- need to sync
+ * with other vCPUs. This particularly helps to keep
+ * kvm_clock stable after CPU hotplug
+ */
+ synchronizing = true;
+ } else {
+ u64 tsc_exp = kvm->arch.last_tsc_write +
+ nsec_to_cycles(vcpu, elapsed);
+ u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
+ /*
+ * Special case: TSC write with a small delta (1 second)
+ * of virtual cycle time against real time is
+ * interpreted as an attempt to synchronize the CPU.
+ */
+ synchronizing = data < tsc_exp + tsc_hz &&
+ data + tsc_hz > tsc_exp;
+ }
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code
2017-04-06 8:08 ` [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code Denis Plotnikov
@ 2017-04-06 17:07 ` Radim Krčmář
0 siblings, 0 replies; 5+ messages in thread
From: Radim Krčmář @ 2017-04-06 17:07 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: pbonzini, mtosatti, kvm, rkagan, den
2017-04-06 11:08+0300, Denis Plotnikov:
> Reuse existing code instead of using inline asm.
> Make the code more concise and clear in the TSC
> synchronization part.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> arch/x86/kvm/x86.c | 51 ++++++++++++---------------------------------------
> 1 file changed, 12 insertions(+), 39 deletions(-)
I like this patch a lot.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -1455,51 +1455,24 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
> elapsed = ns - kvm->arch.last_tsc_nsec;
>
> if (vcpu->arch.virtual_tsc_khz) {
> - int faulted = 0;
> -
> - /* n.b - signed multiplication and division required */
> - usdiff = data - kvm->arch.last_tsc_write;
> -#ifdef CONFIG_X86_64
> - usdiff = (usdiff * 1000) / vcpu->arch.virtual_tsc_khz;
> -#else
> - /* do_div() only does unsigned */
> - asm("1: idivl %[divisor]\n"
> - "2: xor %%edx, %%edx\n"
> - " movl $0, %[faulted]\n"
> - "3:\n"
> - ".section .fixup,\"ax\"\n"
> - "4: movl $1, %[faulted]\n"
> - " jmp 3b\n"
> - ".previous\n"
> -
> - _ASM_EXTABLE(1b, 4b)
> -
> - : "=A"(usdiff), [faulted] "=r" (faulted)
> - : "A"(usdiff * 1000), [divisor] "rm"(vcpu->arch.virtual_tsc_khz));
> -
> -#endif
> - do_div(elapsed, 1000);
Oh, this is actually fixing a bug, because we later consider elapsed in
nanoseconds, but this one converts it to microsends ...
> - usdiff -= elapsed;
> - if (usdiff < 0)
> - usdiff = -usdiff;
> -
> - /* idivl overflow => difference is larger than USEC_PER_SEC */
> - if (faulted)
> - usdiff = USEC_PER_SEC;
> - } else
> - usdiff = USEC_PER_SEC; /* disable TSC match window below */
> + u64 tsc_exp = kvm->arch.last_tsc_write +
> + nsec_to_cycles(vcpu, elapsed);
> + u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
> + /*
> + * Special case: TSC write with a small delta (1 second) of virtual
> + * cycle time against real time is interpreted as an attempt to
> + * synchronize the CPU.
> + */
> + synchronizing = data < tsc_exp + tsc_hz && data > tsc_exp - tsc_hz;
This condition is wrong -- if tsc_exp < tsc_hz, then it will wraparound
and resolve as false. It should read:
synchronizing = data < tsc_exp + tsc_hz && data + tsc_hz > tsc_exp;
> + }
>
> /*
> - * Special case: TSC write with a small delta (1 second) of virtual
> - * cycle time against real time is interpreted as an attempt to
> - * synchronize the CPU.
> - *
> * For a reliable TSC, we can match TSC offsets, and for an unstable
> * TSC, we add elapsed time in this computation. We could let the
> * compensation code attempt to catch up if we fall behind, but
> * it's better to try to match offsets from the beginning.
> */
> - if (usdiff < USEC_PER_SEC &&
> + if (synchronizing &&
> vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
> if (!check_tsc_unstable()) {
> offset = kvm->arch.cur_tsc_offset;
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code
2017-04-06 8:08 [PATCH 0/2] maintaining of kvm_clock stability on guest vCPU hotplug Denis Plotnikov
@ 2017-04-06 8:08 ` Denis Plotnikov
2017-04-06 17:07 ` Radim Krčmář
0 siblings, 1 reply; 5+ messages in thread
From: Denis Plotnikov @ 2017-04-06 8:08 UTC (permalink / raw)
To: pbonzini, rkrcmar, mtosatti, kvm; +Cc: rkagan, den, dplotnikov
Reuse existing code instead of using inline asm.
Make the code more concise and clear in the TSC
synchronization part.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
arch/x86/kvm/x86.c | 51 ++++++++++++---------------------------------------
1 file changed, 12 insertions(+), 39 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..4c674eb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1444,10 +1444,10 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
struct kvm *kvm = vcpu->kvm;
u64 offset, ns, elapsed;
unsigned long flags;
- s64 usdiff;
bool matched;
bool already_matched;
u64 data = msr->data;
+ bool synchronizing = false;
raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
offset = kvm_compute_tsc_offset(vcpu, data);
@@ -1455,51 +1455,24 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
elapsed = ns - kvm->arch.last_tsc_nsec;
if (vcpu->arch.virtual_tsc_khz) {
- int faulted = 0;
-
- /* n.b - signed multiplication and division required */
- usdiff = data - kvm->arch.last_tsc_write;
-#ifdef CONFIG_X86_64
- usdiff = (usdiff * 1000) / vcpu->arch.virtual_tsc_khz;
-#else
- /* do_div() only does unsigned */
- asm("1: idivl %[divisor]\n"
- "2: xor %%edx, %%edx\n"
- " movl $0, %[faulted]\n"
- "3:\n"
- ".section .fixup,\"ax\"\n"
- "4: movl $1, %[faulted]\n"
- " jmp 3b\n"
- ".previous\n"
-
- _ASM_EXTABLE(1b, 4b)
-
- : "=A"(usdiff), [faulted] "=r" (faulted)
- : "A"(usdiff * 1000), [divisor] "rm"(vcpu->arch.virtual_tsc_khz));
-
-#endif
- do_div(elapsed, 1000);
- usdiff -= elapsed;
- if (usdiff < 0)
- usdiff = -usdiff;
-
- /* idivl overflow => difference is larger than USEC_PER_SEC */
- if (faulted)
- usdiff = USEC_PER_SEC;
- } else
- usdiff = USEC_PER_SEC; /* disable TSC match window below */
+ u64 tsc_exp = kvm->arch.last_tsc_write +
+ nsec_to_cycles(vcpu, elapsed);
+ u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
+ /*
+ * Special case: TSC write with a small delta (1 second) of virtual
+ * cycle time against real time is interpreted as an attempt to
+ * synchronize the CPU.
+ */
+ synchronizing = data < tsc_exp + tsc_hz && data > tsc_exp - tsc_hz;
+ }
/*
- * Special case: TSC write with a small delta (1 second) of virtual
- * cycle time against real time is interpreted as an attempt to
- * synchronize the CPU.
- *
* For a reliable TSC, we can match TSC offsets, and for an unstable
* TSC, we add elapsed time in this computation. We could let the
* compensation code attempt to catch up if we fall behind, but
* it's better to try to match offsets from the beginning.
*/
- if (usdiff < USEC_PER_SEC &&
+ if (synchronizing &&
vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
if (!check_tsc_unstable()) {
offset = kvm->arch.cur_tsc_offset;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-07 9:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 9:06 [PATCH v2 0/2] maintaining of kvm_clock stability on guest vCPU hotplug Denis Plotnikov
2017-04-07 9:06 ` [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code Denis Plotnikov
2017-04-07 9:06 ` [PATCH 2/2] KVM: x86: fix maintaining of kvm_clock stability on guest CPU hotplug Denis Plotnikov
-- strict thread matches above, loose matches on Subject: below --
2017-04-06 8:08 [PATCH 0/2] maintaining of kvm_clock stability on guest vCPU hotplug Denis Plotnikov
2017-04-06 8:08 ` [PATCH 1/2] KVM: x86: remaster kvm_write_tsc code Denis Plotnikov
2017-04-06 17:07 ` Radim Krčmář
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.