Re: [PATCH] kvmclock: clock should count only if vm is running (v2)

2013-07-01 Thread Paolo Bonzini 
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 mtosa...@redhat.com

 Applied, thanks.

Cc: qemu-sta...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmclock: clock should count only if vm is running (v2)

2013-07-01 Thread Gleb Natapov 
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 mtosa...@redhat.com
 
  Applied, thanks.
 
 Cc: qemu-sta...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmclock: clock should count only if vm is running (v2)

2013-06-25 Thread Gleb Natapov 
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 mtosa...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmclock: clock should count only if vm is running

2013-06-18 Thread Paolo Bonzini 
Hi Marcelo, sorry for the late review.

Il 08/06/2013 04:00, Marcelo Tosatti ha scritto:
 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).

Do you have any ideas on how to implement CLOCK_BOOTTIME for kvmclock?
I think we need to add more fields for the delta between CLOCK_MONOTONIC
and 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 mtosa...@redhat.com
 
 diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
 index 87d4d0f..7d2d005 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;
 +

Do we need an if (!s-clock_valid) return; here, or an assertion?  (Or
alternatively, what happens if s-clock_valid == false?)

  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 = !runstate_is_running();

Here we know that !runstate_is_running() is true (we're in the else
branch of if (running)) so it can just be s-clock_valid = true.
This matches the false assignment in the other branch.  This is just a
nit, but it makes the code clearer.

Paolo

  }
  }
  
 @@ -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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] kvmclock: clock should count only if vm is running (v2)

2013-06-18 Thread Marcelo Tosatti 

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 mtosa...@redhat.com

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmclock: clock should count only if vm is running

2013-06-18 Thread Marcelo Tosatti 
On Tue, Jun 18, 2013 at 11:02:27AM +0200, Paolo Bonzini wrote:
 Hi Marcelo, sorry for the late review.
 
 Il 08/06/2013 04:00, Marcelo Tosatti ha scritto:
  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).
 
 Do you have any ideas on how to implement CLOCK_BOOTTIME for kvmclock?
 I think we need to add more fields for the delta between CLOCK_MONOTONIC
 and CLOCK_BOOTTIME.

Unsure. An alternative to new fields is to use MSR_KVM_WALL_CLOCK
interface. Looking at two possibilities for catching up with real time
on unpause or vmload:

1) Catch up realtime via guest agent.
2) Catch up realtime via kvmclock interface.

But it would be good to have proper CLOCK_BOOTTIME behaviour at the same
time.

  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 mtosa...@redhat.com
  
  diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
  index 87d4d0f..7d2d005 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;
  +
 
 Do we need an if (!s-clock_valid) return; here, or an assertion?  (Or
 alternatively, what happens if s-clock_valid == false?)


s-clock_valid = true means the clock value at s-clock (saved in
QEMU) is valid. 

At the moment VM begins execution, it becomes invalid (which means
synchronizing that value is necessary).

For the stopped-running transition, it has no meaning (just as the load
functions assume the device state is valid when they are called).

That said, i see no reason for additional checks.

  + */
  +s-clock_valid = !runstate_is_running();
 
 Here we know that !runstate_is_running() is true (we're in the else
 branch of if (running)) so it can just be s-clock_valid = true.
 This matches the false assignment in the other branch.  This is just a
 nit, but it makes the code clearer.
 
 Paolo

Sure, resending.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] kvmclock: clock should count only if vm is running

2013-06-07 Thread Marcelo Tosatti 

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 mtosa...@redhat.com

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 87d4d0f..7d2d005 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 = !runstate_is_running();
 }
 }
 
@@ -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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html