kvmclock: clock should count only if vm is running

PATCH In review Marcelo Tosatti 3 participants 7 messages Jun 8, 2013 – Jul 1, 2013 KVM

lore

Share & tools

lore

Marcelo Tosatti

author

2013-06-08 02:01 UTC #0 lore raw

PATCH 1/1 kvmclock: clock should count only if vm is running

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 <[email protected]>

hw/i386/kvm/clock.c +30 ~~-34~~

index 87d4d0f..7d2d005 100644
@@ -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()

Paolo Bonzini

maintainer

2013-06-18 09:02 UTC #1 lore raw

Hi Marcelo, sorry for the late review.

Il 08/06/2013 04:00, Marcelo Tosatti ha scritto:

Quoted 8 lines

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.

Quoted 50 lines

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 <[email protected]>

hw/i386/kvm/clock.c +0 ~~-32~~

index 87d4d0f..7d2d005 100644
@@ -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)

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 42

Quoted 5 lines

hw/i386/kvm/clock.c +2 -0

@@ -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?)

Quoted 27 lines

hw/i386/kvm/clock.c +18 -0

         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));

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 84

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+            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

Quoted 17 lines

hw/i386/kvm/clock.c +0 -3

     }
 }

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

Marcelo Tosatti

author

2013-06-18 23:42 UTC #2 lore raw

Quoted 9 lines · Paolo Bonzini

Hi Marcelo, sorry for the late review.
Il 08/06/2013 04:00, Marcelo Tosatti ha scritto:
Quoted 6 lines
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 52 lines

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 <[email protected]>

hw/i386/kvm/clock.c +0 ~~-32~~

index 87d4d0f..7d2d005 100644
@@ -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;

Quoted 6 lines

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 42
Quoted 3 lines
hw/i386/kvm/clock.c +2 -0
     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.

Quoted 9 lines

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 84
Quoted 2 lines
hw/i386/kvm/clock.c +2 -0
+         */
+        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.

Marcelo Tosatti

author

2013-06-18 23:42 UTC #3 lore raw

PATCH 1/1 kvmclock: clock should count only if vm is running (v2)

In reply to 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 <[email protected]>

hw/i386/kvm/clock.c +30 ~~-34~~

index 87d4d0f..98e5ca5 100644
@@ -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()

Gleb Natapov

Applied

2013-06-25 12:18 UTC #4 lore raw

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

Quoted 11 lines · 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.

Quoted 9 lines

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 <[email protected]>

Applied, thanks.

Quoted 99 lines

hw/i386/kvm/clock.c +30 ~~-35~~

index 87d4d0f..98e5ca5 100644
@@ -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()
--

Quoted 3 lines

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

--
Gleb.

Paolo Bonzini

maintainer

2013-07-01 11:44 UTC #5 lore raw

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

Il 25/06/2013 14:18, Gleb Natapov ha scritto:

Quoted 15 lines · Marcelo Tosatti

Quoted 14 lines
Quoted 14 lines
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).

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
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 <[email protected]>
Applied, thanks.

Cc [email protected]

Gleb, the commit hash is fixed now that you applied the patch---so I cannot just amend it to include the Cc, right?

Paolo

Gleb Natapov

2013-07-01 11:54 UTC #6 lore raw

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

Quoted 20 lines · Paolo Bonzini

Il 25/06/2013 14:18, Gleb Natapov ha scritto:
Quoted 19 lines · Marcelo Tosatti
Quoted 18 lines
Quoted 18 lines
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.

Quoted 9 lines

Quoted 3 lines
Quoted 1 line
Quoted 1 line
Signed-off-by Marcelo Tosatti <[email protected]>
Applied, thanks.
Cc [email protected]
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.