| Message ID | 20130618233825.GA19042@amt.cnet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 18, 2013 at 08:38:25PM -0300, Marcelo Tosatti wrote: > > v2: remove unnecessary runstate_is_running() usage (Paolo) > > -- > > kvmclock should not count while vm is paused, because: > > 1) if the vm is paused for long periods, timekeeping > math can overflow while converting the (large) clocksource > delta to nanoseconds. > > 2) Users rely on CLOCK_MONOTONIC to count run time, that is, > time which OS has been in a runnable state (see CLOCK_BOOTTIME). > > Change kvmclock driver so as to save clock value when vm transitions > from runnable to stopped state, and to restore clock value from stopped > to runnable transition. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > Applied, thanks. > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c > index 87d4d0f..98e5ca5 100644 > --- a/hw/i386/kvm/clock.c > +++ b/hw/i386/kvm/clock.c > @@ -28,38 +28,6 @@ typedef struct KVMClockState { > bool clock_valid; > } KVMClockState; > > -static void kvmclock_pre_save(void *opaque) > -{ > - KVMClockState *s = opaque; > - struct kvm_clock_data data; > - int ret; > - > - if (s->clock_valid) { > - return; > - } > - ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data); > - if (ret < 0) { > - fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret)); > - data.clock = 0; > - } > - s->clock = data.clock; > - /* > - * If the VM is stopped, declare the clock state valid to avoid re-reading > - * it on next vmsave (which would return a different value). Will be reset > - * when the VM is continued. > - */ > - s->clock_valid = !runstate_is_running(); > -} > - > -static int kvmclock_post_load(void *opaque, int version_id) > -{ > - KVMClockState *s = opaque; > - struct kvm_clock_data data; > - > - data.clock = s->clock; > - data.flags = 0; > - return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); > -} > > static void kvmclock_vm_state_change(void *opaque, int running, > RunState state) > @@ -70,8 +38,18 @@ static void kvmclock_vm_state_change(void *opaque, int running, > int ret; > > if (running) { > + struct kvm_clock_data data; > + > s->clock_valid = false; > > + data.clock = s->clock; > + data.flags = 0; > + ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); > + if (ret < 0) { > + fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret)); > + abort(); > + } > + > if (!cap_clock_ctrl) { > return; > } > @@ -84,6 +62,26 @@ static void kvmclock_vm_state_change(void *opaque, int running, > return; > } > } > + } else { > + struct kvm_clock_data data; > + int ret; > + > + if (s->clock_valid) { > + return; > + } > + ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data); > + if (ret < 0) { > + fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret)); > + abort(); > + } > + s->clock = data.clock; > + > + /* > + * If the VM is stopped, declare the clock state valid to > + * avoid re-reading it on next vmsave (which would return > + * a different value). Will be reset when the VM is continued. > + */ > + s->clock_valid = true; > } > } > > @@ -100,8 +98,6 @@ static const VMStateDescription kvmclock_vmsd = { > .version_id = 1, > .minimum_version_id = 1, > .minimum_version_id_old = 1, > - .pre_save = kvmclock_pre_save, > - .post_load = kvmclock_post_load, > .fields = (VMStateField[]) { > VMSTATE_UINT64(clock, KVMClockState), > VMSTATE_END_OF_LIST() > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 25/06/2013 14:18, Gleb Natapov ha scritto: > On Tue, Jun 18, 2013 at 08:38:25PM -0300, Marcelo Tosatti wrote: >> > >> > v2: remove unnecessary runstate_is_running() usage (Paolo) >> > >> > -- >> > >> > kvmclock should not count while vm is paused, because: >> > >> > 1) if the vm is paused for long periods, timekeeping >> > math can overflow while converting the (large) clocksource >> > delta to nanoseconds. >> > >> > 2) Users rely on CLOCK_MONOTONIC to count run time, that is, >> > time which OS has been in a runnable state (see CLOCK_BOOTTIME). >> > >> > Change kvmclock driver so as to save clock value when vm transitions >> > from runnable to stopped state, and to restore clock value from stopped >> > to runnable transition. >> > >> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > Applied, thanks. Cc: qemu-stable@nongnu.org Gleb, the commit hash is fixed now that you applied the patch---so I cannot just amend it to include the Cc, right? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 01, 2013 at 01:44:42PM +0200, Paolo Bonzini wrote: > Il 25/06/2013 14:18, Gleb Natapov ha scritto: > > On Tue, Jun 18, 2013 at 08:38:25PM -0300, Marcelo Tosatti wrote: > >> > > >> > v2: remove unnecessary runstate_is_running() usage (Paolo) > >> > > >> > -- > >> > > >> > kvmclock should not count while vm is paused, because: > >> > > >> > 1) if the vm is paused for long periods, timekeeping > >> > math can overflow while converting the (large) clocksource > >> > delta to nanoseconds. > >> > > >> > 2) Users rely on CLOCK_MONOTONIC to count run time, that is, > >> > time which OS has been in a runnable state (see CLOCK_BOOTTIME). > >> > > >> > Change kvmclock driver so as to save clock value when vm transitions > >> > from runnable to stopped state, and to restore clock value from stopped > >> > to runnable transition. > >> > > >> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > > Applied, thanks. > > Cc: qemu-stable@nongnu.org > > Gleb, the commit hash is fixed now that you applied the patch---so I > cannot just amend it to include the Cc, right? > Qemu upstream has weird rule that requires patches to be sent on the list with each pull request and this effectively means that it is impossible to do merges which, in turn, means that sometimes rebase is unavoidable. But we try not to do them anyway. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c index 87d4d0f..98e5ca5 100644 --- a/hw/i386/kvm/clock.c +++ b/hw/i386/kvm/clock.c @@ -28,38 +28,6 @@ typedef struct KVMClockState { bool clock_valid; } KVMClockState; -static void kvmclock_pre_save(void *opaque) -{ - KVMClockState *s = opaque; - struct kvm_clock_data data; - int ret; - - if (s->clock_valid) { - return; - } - ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data); - if (ret < 0) { - fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret)); - data.clock = 0; - } - s->clock = data.clock; - /* - * If the VM is stopped, declare the clock state valid to avoid re-reading - * it on next vmsave (which would return a different value). Will be reset - * when the VM is continued. - */ - s->clock_valid = !runstate_is_running(); -} - -static int kvmclock_post_load(void *opaque, int version_id) -{ - KVMClockState *s = opaque; - struct kvm_clock_data data; - - data.clock = s->clock; - data.flags = 0; - return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); -} static void kvmclock_vm_state_change(void *opaque, int running, RunState state) @@ -70,8 +38,18 @@ static void kvmclock_vm_state_change(void *opaque, int running, int ret; if (running) { + struct kvm_clock_data data; + s->clock_valid = false; + data.clock = s->clock; + data.flags = 0; + ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data); + if (ret < 0) { + fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret)); + abort(); + } + if (!cap_clock_ctrl) { return; } @@ -84,6 +62,26 @@ static void kvmclock_vm_state_change(void *opaque, int running, return; } } + } else { + struct kvm_clock_data data; + int ret; + + if (s->clock_valid) { + return; + } + ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data); + if (ret < 0) { + fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret)); + abort(); + } + s->clock = data.clock; + + /* + * If the VM is stopped, declare the clock state valid to + * avoid re-reading it on next vmsave (which would return + * a different value). Will be reset when the VM is continued. + */ + s->clock_valid = true; } } @@ -100,8 +98,6 @@ static const VMStateDescription kvmclock_vmsd = { .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, - .pre_save = kvmclock_pre_save, - .post_load = kvmclock_post_load, .fields = (VMStateField[]) { VMSTATE_UINT64(clock, KVMClockState), VMSTATE_END_OF_LIST()
v2: remove unnecessary runstate_is_running() usage (Paolo) -- kvmclock should not count while vm is paused, because: 1) if the vm is paused for long periods, timekeeping math can overflow while converting the (large) clocksource delta to nanoseconds. 2) Users rely on CLOCK_MONOTONIC to count run time, that is, time which OS has been in a runnable state (see CLOCK_BOOTTIME). Change kvmclock driver so as to save clock value when vm transitions from runnable to stopped state, and to restore clock value from stopped to runnable transition. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html