| Message ID | 20161129105600.GA12426@amt.cnet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 29, 2016 at 08:56:00AM -0200, Marcelo Tosatti wrote: > 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 <mtosatti@redhat.com> > > > > > > > > > > --- > > > > > 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 */ I would make the comment more explicit about its meaning: /* whether the 'clock' value was obtained in a host with * reliable KVM_GET_CLOCK */ > + 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); > - } > + A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory". > + 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) { Why the s->clock_valid check? It seems unnecessary. (If this is an optimization to avoid extra ioctl() calls, you can cache it inside kvm_has_adjust_clock_stable()). > + s->clock_is_reliable = kvm_has_adjust_clock_stable(); > } This should go to kvm_get_clock(), so s->clock and s->clock_is_reliable are guaranteed to be always in sync. > > /* 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() > } > };
On Tue, Nov 29, 2016 at 10:34:05AM -0200, Eduardo Habkost wrote: > On Tue, Nov 29, 2016 at 08:56:00AM -0200, Marcelo Tosatti wrote: > > 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 <mtosatti@redhat.com> > > > > > > > > > > > > --- > > > > > > 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 */ > > I would make the comment more explicit about its meaning: > /* whether the 'clock' value was obtained in a host with > * reliable KVM_GET_CLOCK */ Done. > > + 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); > > - } > > + > > A comment here would be welcome. Something like: "If the host > where s->clock was read did not support reliable KVM_GET_CLOCK, > read kvmclock value from memory". Done. > > + 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) { > > Why the s->clock_valid check? It seems unnecessary. Because if s->clock_valid == false (from migration), kvm_has_adjust_clock_stable() from the local host is irrelevant. If s->clock_valid == true, kvm_has_adjust_clock_stable() is what decides whether to use KVM_GET_CLOCK or not. > (If this is an optimization to avoid extra ioctl() calls, you can > cache it inside kvm_has_adjust_clock_stable()). Its not. > > + s->clock_is_reliable = kvm_has_adjust_clock_stable(); > > } > > This should go to kvm_get_clock(), so s->clock and > s->clock_is_reliable are guaranteed to be always in sync. No: the point to change clock_is_reliable is the following: whether its the first read of the clock (migration), or subsequent reads (normal machine operation). Now kvm_get_clock() just reads the clock, so i don't see it as the right place to change the variable. Agreed? > > /* 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() > > } > > }; > > -- > Eduardo
On Tue, Nov 29, 2016 at 09:54:29PM -0200, Marcelo Tosatti wrote: > On Tue, Nov 29, 2016 at 10:34:05AM -0200, Eduardo Habkost wrote: > > On Tue, Nov 29, 2016 at 08:56:00AM -0200, Marcelo Tosatti wrote: > > > 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 <mtosatti@redhat.com> > > > > > > > > > > > > > > --- > > > > > > > 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 */ > > > > I would make the comment more explicit about its meaning: > > /* whether the 'clock' value was obtained in a host with > > * reliable KVM_GET_CLOCK */ > > Done. > > > > + 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); > > > - } > > > + > > > > A comment here would be welcome. Something like: "If the host > > where s->clock was read did not support reliable KVM_GET_CLOCK, > > read kvmclock value from memory". > > Done. > > > > + 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) { > > > > Why the s->clock_valid check? It seems unnecessary. > > Because if s->clock_valid == false (from migration), > kvm_has_adjust_clock_stable() from the local host is irrelevant. That's true, and that's why updating it on kvm_get_clock() looks like a more logical place. > > If s->clock_valid == true, kvm_has_adjust_clock_stable() is > what decides whether to use KVM_GET_CLOCK or not. > > > (If this is an optimization to avoid extra ioctl() calls, you can > > cache it inside kvm_has_adjust_clock_stable()). > > Its not. > > > > + s->clock_is_reliable = kvm_has_adjust_clock_stable(); > > > } > > > > This should go to kvm_get_clock(), so s->clock and > > s->clock_is_reliable are guaranteed to be always in sync. > > No: the point to change clock_is_reliable is the following: whether > its the first read of the clock (migration), or subsequent > reads (normal machine operation). > > Now kvm_get_clock() just reads the clock, so i don't see it as > the right place to change the variable. > > Agreed? I don't agree. clock_is_reliable is information about the current value in s->clock, I don't see why we need to change s->clock and s->clock_is_reliable at different places in the code. Your code might work too, but I can't tell without reviewing very carefully the state transitions KVMClockState could go through. I don't see the point of making the state transitions in this code even more complex, if we can simply stick to a simple "s->clock and s->clock_is_reliable are always in sync" rule. > > > > /* 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() > > > } > > > }; > > > > -- > > Eduardo
On Wed, Nov 30, 2016 at 11:09:28AM -0200, Eduardo Habkost wrote: > On Tue, Nov 29, 2016 at 09:54:29PM -0200, Marcelo Tosatti wrote: > > On Tue, Nov 29, 2016 at 10:34:05AM -0200, Eduardo Habkost wrote: > > > On Tue, Nov 29, 2016 at 08:56:00AM -0200, Marcelo Tosatti wrote: > > > > 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 <mtosatti@redhat.com> > > > > > > > > > > > > > > > > --- > > > > > > > > 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 */ > > > > > > I would make the comment more explicit about its meaning: > > > /* whether the 'clock' value was obtained in a host with > > > * reliable KVM_GET_CLOCK */ > > > > Done. > > > > > > + 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); > > > > - } > > > > + > > > > > > A comment here would be welcome. Something like: "If the host > > > where s->clock was read did not support reliable KVM_GET_CLOCK, > > > read kvmclock value from memory". > > > > Done. > > > > > > + 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) { > > > > > > Why the s->clock_valid check? It seems unnecessary. > > > > Because if s->clock_valid == false (from migration), > > kvm_has_adjust_clock_stable() from the local host is irrelevant. > > That's true, and that's why updating it on kvm_get_clock() looks > like a more logical place. But the current patch updates s->clock_is_reliable at the "if (running)" case of kvmclock_vm_state_change(). There, both s->clock_is_reliable and s->clock are updated in sync: 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 */ if (pvclock_via_mem) { s->clock = pvclock_via_mem; } However, there is no need to call kvm_get_clock() there. kvm_get_clock() is called at the "} else {" case. Are you suggesting to * move s->clock_is_reliable assignment to kvm_get_clock() * move s->clock assignment to kvm_get_clock() * call kvm_get_clock() from the "if (running)" because the assignments are in that function now. But you don't need KVM_GET_CLOCK to be called there. > > > > > If s->clock_valid == true, kvm_has_adjust_clock_stable() is > > what decides whether to use KVM_GET_CLOCK or not. > > > > > (If this is an optimization to avoid extra ioctl() calls, you can > > > cache it inside kvm_has_adjust_clock_stable()). > > > > Its not. > > > > > > + s->clock_is_reliable = kvm_has_adjust_clock_stable(); > > > > } > > > > > > This should go to kvm_get_clock(), so s->clock and > > > s->clock_is_reliable are guaranteed to be always in sync. > > > > No: the point to change clock_is_reliable is the following: whether > > its the first read of the clock (migration), or subsequent > > reads (normal machine operation). > > > > Now kvm_get_clock() just reads the clock, so i don't see it as > > the right place to change the variable. > > > > Agreed? > > I don't agree. clock_is_reliable is information about the current > value in s->clock, I don't see why we need to change s->clock and > s->clock_is_reliable at different places in the code. > > Your code might work too, but I can't tell without reviewing very > carefully the state transitions KVMClockState could go through. I > don't see the point of making the state transitions in this code > even more complex, if we can simply stick to a simple "s->clock > and s->clock_is_reliable are always in sync" rule. It is updating from the same place: /* 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 */ if (pvclock_via_mem) { s->clock = pvclock_via_mem; } > > > > > > > /* 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() > > > > } > > > > }; > > > > > > -- > > > Eduardo > > -- > 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: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() } };