[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock differenc
From: |
Marcelo Tosatti |
Subject: |
Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration |
Date: |
Tue, 29 Nov 2016 08:56:00 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Nov 28, 2016 at 03:12:01PM -0200, Eduardo Habkost wrote:
> On Mon, Nov 28, 2016 at 02:45:24PM -0200, Marcelo Tosatti wrote:
> > On Mon, Nov 28, 2016 at 12:13:22PM -0200, Eduardo Habkost wrote:
> > > Sorry for not noticing the following issues on the previous
> > > reviews. I was only paying attention to the vmstate and machine
> > > code:
> > >
> > > On Mon, Nov 21, 2016 at 08:50:04AM -0200, Marcelo Tosatti wrote:
> > > > Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which
> > > > indicates that KVM_GET_CLOCK returns a value as seen by the guest at
> > > > that moment.
> > > >
> > > > For new machine types, use this value rather than reading
> > > > from guest memory.
> > > >
> > > > This reduces kvmclock difference on migration from 5s to 0.1s
> > > > (when max_downtime == 5s).
> > > >
> > > > Signed-off-by: Marcelo Tosatti <address@hidden>
> > > >
> > > > ---
> > > > hw/i386/kvm/clock.c | 107
> > > > ++++++++++++++++++++++++++++++++++++++++++-------
> > > > include/hw/i386/pc.h | 5 ++
> > > > target-i386/kvm.c | 7 +++
> > > > target-i386/kvm_i386.h | 1
> > > > 4 files changed, 106 insertions(+), 14 deletions(-)
> > > >
> > > > v2:
> > > > - improve variable names (Juan)
> > > > - consolidate code on kvm_get_clock function (Paolo)
> > > > - return mach_use_reliable_get_clock from needed function (Paolo)
> > > > v3:
> > > > - simplify check for src_use_reliable_get_clock (Eduardo)
> > > > - move src_use_reliable_get_clock initialization to realize (Eduardo)
> > > >
> > > >
> > > > Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
> > > > ===================================================================
> > > > --- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-17
> > > > 15:07:11.220632761 -0200
> > > > +++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-11-17
> > > > 15:11:51.372048640 -0200
> > > > @@ -36,6 +36,12 @@
> > > >
> > > > uint64_t clock;
> > > > bool clock_valid;
> > > > +
> > > > + /* whether machine supports reliable KVM_GET_CLOCK */
> > > > + bool mach_use_reliable_get_clock;
> > > > +
> > > > + /* whether source host supported reliable KVM_GET_CLOCK */
> > > > + bool src_use_reliable_get_clock;
> > > > } KVMClockState;
> > > >
> > > > struct pvclock_vcpu_time_info {
> > > > @@ -81,6 +87,19 @@
> > > > return nsec + time.system_time;
> > > > }
> > > >
> > > > +static uint64_t kvm_get_clock(void)
> > > > +{
> > > > + struct kvm_clock_data data;
> > > > + int ret;
> > > > +
> > > > + ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> > > > + if (ret < 0) {
> > > > + fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> > > > + abort();
> > > > + }
> > > > + return data.clock;
> > > > +}
> > > > +
> > > > static void kvmclock_vm_state_change(void *opaque, int running,
> > > > RunState state)
> > > > {
> > > > @@ -91,15 +110,36 @@
> > >
> > > Can you please use "diff -p" on your patches?
> > >
> > > >
> > > > if (running) {
> > > > struct kvm_clock_data data = {};
> > > > - uint64_t time_at_migration = kvmclock_current_nsec(s);
> > > > + uint64_t pvclock_via_mem = 0;
> > > >
> > > > - s->clock_valid = false;
> > > > + /* local (running VM) restore */
> > > > + if (s->clock_valid) {
> > > > + /*
> > > > + * if host does not support reliable KVM_GET_CLOCK,
> > > > + * read kvmclock value from memory
> > > > + */
> > > > + if (!kvm_has_adjust_clock_stable()) {
> > >
> > > Why not use src_use_reliable_get_clock here, too? (Maybe rename
> > > it to simply clock_is_reliable?)
> >
> > Because you end up mixing two mental ideas (one: whether the source
> > has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK
> > into one variable). I find it more confusing than not.
> > Maybe its just my limited brain but i find it
> > confusing.
>
> I find it simpler and easier to reason about.
I disagree, but i don't mind. Is this what you refer to:
Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-17
15:11:51.372048640 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-11-29 08:53:20.609817026
-0200
@@ -37,11 +37,11 @@
uint64_t clock;
bool clock_valid;
- /* whether machine supports reliable KVM_GET_CLOCK */
+ /* whether machine type supports reliable get clock */
bool mach_use_reliable_get_clock;
- /* whether source host supported reliable KVM_GET_CLOCK */
- bool src_use_reliable_get_clock;
+ /* whether clock is reliable */
+ bool clock_is_reliable;
} KVMClockState;
struct pvclock_vcpu_time_info {
@@ -112,25 +112,17 @@
struct kvm_clock_data data = {};
uint64_t pvclock_via_mem = 0;
- /* local (running VM) restore */
- if (s->clock_valid) {
- /*
- * if host does not support reliable KVM_GET_CLOCK,
- * read kvmclock value from memory
- */
- if (!kvm_has_adjust_clock_stable()) {
- pvclock_via_mem = kvmclock_current_nsec(s);
- }
- /* migration/savevm/init restore */
- } else {
- /*
- * use s->clock in case machine uses reliable
- * get clock and source host supported
- * reliable get clock
- */
- if (!s->src_use_reliable_get_clock) {
- pvclock_via_mem = kvmclock_current_nsec(s);
- }
+
+ if (!s->clock_is_reliable) {
+ pvclock_via_mem = kvmclock_current_nsec(s);
+ }
+
+ /* migration/savevm/init restore
+ * update clock_is_reliable to match local
+ * host capabilities.
+ */
+ if (s->clock_valid == false) {
+ s->clock_is_reliable = kvm_has_adjust_clock_stable();
}
/* We can't rely on the saved clock value, just discard it */
@@ -181,20 +173,14 @@
{
KVMClockState *s = KVM_CLOCK(dev);
- /*
- * On machine types that support reliable KVM_GET_CLOCK,
- * if host kernel does provide reliable KVM_GET_CLOCK,
- * set src_use_reliable_get_clock=true so that destination
- * avoids reading kvmclock from memory.
- */
- if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
- s->src_use_reliable_get_clock = true;
+ if (kvm_has_adjust_clock_stable()) {
+ s->clock_is_reliable = true;
}
qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
}
-static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
{
KVMClockState *s = opaque;
@@ -202,12 +188,12 @@
}
static const VMStateDescription kvmclock_reliable_get_clock = {
- .name = "kvmclock/src_use_reliable_get_clock",
+ .name = "kvmclock/clock_is_reliable",
.version_id = 1,
.minimum_version_id = 1,
- .needed = kvmclock_src_use_reliable_get_clock,
+ .needed = kvmclock_clock_is_reliable_needed,
.fields = (VMStateField[]) {
- VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+ VMSTATE_BOOL(clock_is_reliable, KVMClockState),
VMSTATE_END_OF_LIST()
}
};
- [Qemu-devel] [qemu patch V3 0/2] improve kvmclock difference on migration, Marcelo Tosatti, 2016/11/21
- [Qemu-devel] [qemu patch V3 1/2] kvm: sync linux headers, Marcelo Tosatti, 2016/11/21
- [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Marcelo Tosatti, 2016/11/21
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Eduardo Habkost, 2016/11/28
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Marcelo Tosatti, 2016/11/28
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Eduardo Habkost, 2016/11/28
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Paolo Bonzini, 2016/11/28
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration,
Marcelo Tosatti <=
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Eduardo Habkost, 2016/11/29
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Marcelo Tosatti, 2016/11/29
- Re: [Qemu-devel] [qemu patch V3 2/2] kvmclock: reduce kvmclock difference on migration, Eduardo Habkost, 2016/11/30
Re: [Qemu-devel] [qemu patch V3 0/2] improve kvmclock difference on migration, Marcelo Tosatti, 2016/11/28