kvmclock: advance clock by time window between vm_stop and pre_save

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Please do not apply this yet as some codepaths still need checking, submitting early for comments.

Signed-off-by Marcelo Tosatti <[email protected]>

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

+static uint64_t clock_delta(struct timespec *before, struct timespec *after)
+{
+    if (before->tv_sec > after->tv_sec ||
+        (before->tv_sec == after->tv_sec &&

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

+
+    clock_gettime(CLOCK_MONOTONIC, &now);
+
+    ns = clock_delta(&s->t_aftervmstop, &now);

hw/i386/kvm/clock.c +13 -1

+    }
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    KVMClockState *s = opaque;
+
+    /* save the value from incoming migration */
+    s->advance_clock = s->ns;
+
+    return 0;
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
-    .version_id = 1,

Quoted 8 lines · Marcelo Tosatti

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Quoted 4 lines

Please do not apply this yet as some codepaths still need checking, submitting early for comments.

Signed-off-by Marcelo Tosatti <[email protected]>

You can use an optional section, and then you don't need to increase the version number.

I believe you that the clock manipulation is right, only talking about the migration bits.

Quoted 9 lines

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

+static uint64_t clock_delta(struct timespec *before, struct timespec *after)
+{
+    if (before->tv_sec > after->tv_sec ||
+        (before->tv_sec == after->tv_sec &&
+         before->tv_nsec > after->tv_nsec)) {
+        fprintf(stderr, "clock_delta failed: before=(%ld sec, %ld nsec),"
+                        "after=(%ld sec, %ld nsec)\n", before->tv_sec,
+                        before->tv_nsec, after->tv_sec, after->tv_nsec);
+        abort();

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 191

Quoted 5 lines

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

+    }
+
+    return (after->tv_sec - before->tv_sec) * 1000000000ULL +
+            after->tv_nsec - before->tv_nsec;
+}

I can't believe that we don't have a helper function already to calculate this....

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 201

Quoted 10 lines

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

+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    struct timespec now;
+    uint64_t ns;
+
+    if (s->t_aftervmstop.tv_sec == 0) {
+        return;
+    }

You have your test here.

Quoted 9 lines

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

+
+    clock_gettime(CLOCK_MONOTONIC, &now);
+
+    ns = clock_delta(&s->t_aftervmstop, &now);
+
+    /*
+     * Linux guests can overflow if time jumps
+     * forward in large increments.
+     * Cap maximum adjustment to 10 minutes.

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 215

Quoted 5 lines

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

+     */
+    ns = MIN(ns, 600000000000ULL);
+
+    if (s->clock + ns > s->clock) {
+        s->ns = ns;

Would it be a good idea to print an error message here? If it has been more than 10mins since we did the vmstop, something got wrong here.

Quoted 16 lines

hw/i386/kvm/clock.c +13 -1

+    }
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    KVMClockState *s = opaque;
+
+    /* save the value from incoming migration */
+    s->advance_clock = s->ns;
+
+    return 0;
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
-    .version_id = 1,

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 240

Quoted 10 lines

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

+    .version_id = 2,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
+    .post_load = kvmclock_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
+        VMSTATE_UINT64_V(ns, KVMClockState, 2),
         VMSTATE_END_OF_LIST()
     }
 };

If you need help with the subsection stuff, just ask.

Later, Juan.

Quoted 9 lines · Juan Quintela

Quoted 8 lines · Marcelo Tosatti

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Quoted 7 lines

Quoted 4 lines

Please do not apply this yet as some codepaths still need checking, submitting early for comments.

Signed-off-by Marcelo Tosatti <[email protected]>

You can use an optional section, and then you don't need to increase the version number.

Optional section is more appropriate, thanks.

Quoted 12 lines

I believe you that the clock manipulation is right, only talking about the migration bits.

Quoted 9 lines

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

+static uint64_t clock_delta(struct timespec *before, struct timespec *after)
+{
+    if (before->tv_sec > after->tv_sec ||
+        (before->tv_sec == after->tv_sec &&
+         before->tv_nsec > after->tv_nsec)) {
+        fprintf(stderr, "clock_delta failed: before=(%ld sec, %ld nsec),"
+                        "after=(%ld sec, %ld nsec)\n", before->tv_sec,
+                        before->tv_nsec, after->tv_sec, after->tv_nsec);
+        abort();

Quoted 8 lines

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 191

Quoted 5 lines

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

+    }
+
+    return (after->tv_sec - before->tv_sec) * 1000000000ULL +
+            after->tv_nsec - before->tv_nsec;
+}

I can't believe that we don't have a helper function already to calculate this....

Couldnt find any...

Quoted 22 lines

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 201

Quoted 10 lines

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

+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    struct timespec now;
+    uint64_t ns;
+
+    if (s->t_aftervmstop.tv_sec == 0) {
+        return;
+    }

You have your test here.

Quoted 9 lines

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

+
+    clock_gettime(CLOCK_MONOTONIC, &now);
+
+    ns = clock_delta(&s->t_aftervmstop, &now);
+
+    /*
+     * Linux guests can overflow if time jumps
+     * forward in large increments.
+     * Cap maximum adjustment to 10 minutes.

Quoted 8 lines

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 215

Quoted 5 lines

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

+     */
+    ns = MIN(ns, 600000000000ULL);
+
+    if (s->clock + ns > s->clock) {
+        s->ns = ns;

Would it be a good idea to print an error message here? If it has been more than 10mins since we did the vmstop, something got wrong here.

Not sure... is it not possible for the user to stop migration in some way?

What if network is very slow and maxdowntime very high?

Quoted 30 lines

Quoted 16 lines

hw/i386/kvm/clock.c +13 -1

+    }
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    KVMClockState *s = opaque;
+
+    /* save the value from incoming migration */
+    s->advance_clock = s->ns;
+
+    return 0;
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
-    .version_id = 1,

hw/i386/kvm/clock.c static void kvmclock_realize(DeviceState *dev, Error **errp) · line 240

Quoted 10 lines

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

+    .version_id = 2,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
+    .post_load = kvmclock_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
+        VMSTATE_UINT64_V(ns, KVMClockState, 2),
         VMSTATE_END_OF_LIST()
     }
 };

If you need help with the subsection stuff, just ask.

Quoted 1 line

Later, Juan.

Ok, i'll try to cook up an optional section and lets see what happens.

Thanks Juan.

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Please do not apply this yet as some codepaths still need checking, submitting early for comments.

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Quoted 5 lines

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

What Linux bug? The one that makes us use kvmclock_current_nsec?

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock. We wouldn't need this advance_clock hack if we could just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

Quoted 7 lines

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

I agree, that sounds like a nice improvement.

Quoted 13 lines

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 6 lines

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock. We wouldn't need this advance_clock hack if we could just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

There are two cases:

- migrating a paused guest

- pausing at the end of migration

In the first case, kvmclock_vm_state_change's !running branch will see state == RUN_STATE_FINISH_MIGRATE && s->clock_valid. In the second case, it will see state == RUN_STATE_FINISH_MIGRATE && !s->clock_valid.

In the second case it should do nothing. Then kvmclock_pre_save will see !s->clock_valid and call KVM_GET_CLOCK. A bonus is that, if migration fails and migration_thread() calls vm_start(), then the guest will not see the clock drift backwards.

Quoted 3 lines

Quoted 3 lines

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

Quoted 5 lines

Quoted 3 lines

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

I agree, that sounds like a nice improvement.

Ok, I'll send the KVM patch then. It can even be the value *returned* for KVM_CAP_ADJUST_CLOCK---right now it returns 1, we can return 3 (bit 0 for KVM_CAP_ADJUST_CLOCK_RESENT and bit 1 for returning the raw value). Any suggestion for the name?

Quoted 10 lines

Quoted 5 lines

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock. We wouldn't need this advance_clock hack if we could just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

There are two cases:

- migrating a paused guest

Quoted 5 lines

- pausing at the end of migration

In the first case, kvmclock_vm_state_change's !running branch will see state == RUN_STATE_FINISH_MIGRATE && s->clock_valid. In the second case, it will see state == RUN_STATE_FINISH_MIGRATE && !s->clock_valid.

I lift my case, marcelo's said that stopping the time is a feature ... (*kittens die*)

Oh, and this does introduce a minor bug to this patch -- the time counted by KVM_GET_CLOCK is has different frequency CLOCK_MONOTONIC. Not accounting for that is bearable.

Quoted 16 lines

Quoted 15 lines

Quoted 5 lines

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock. We wouldn't need this advance_clock hack if we could just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

There are two cases:

- migrating a paused guest

- pausing at the end of migration

In the first case, kvmclock_vm_state_change's !running branch will see state == RUN_STATE_FINISH_MIGRATE && s->clock_valid. In the second case, it will see state == RUN_STATE_FINISH_MIGRATE && !s->clock_valid.

Quoted 2 lines

I lift my case, marcelo's said that stopping the time is a feature ... (*kittens die*)

But that's why separating the two cases brings us the best of both worlds. If migrating a paused guest, there's no need for any adjustment, so no advance_clock hack. If pausing at the end of migration, there's no need to pause kvmclock (this patch is effectively working around 00f4d64ee76e) and if we don't do that we can just call KVM_GET_CLOCK at pre_save time.

Quoted 4 lines

Oh, and this does introduce a minor bug to this patch -- the time counted by KVM_GET_CLOCK is has different frequency CLOCK_MONOTONIC. Not accounting for that is bearable.

Not really, I was going to point that out when I got to replying with a review. :)

Paolo

Quoted 5 lines

But that's why separating the two cases brings us the best of both worlds. If migrating a paused guest, there's no need for any adjustment, so no advance_clock hack. If pausing at the end of migration, there's no need to pause kvmclock (this patch is effectively working around 00f4d64ee76e) and if we don't do that we can just call KVM_GET_CLOCK at pre_save time.

That was my internal v1. But then, the destination ignores s->clock as follows:

    if (running) {
        struct kvm_clock_data data = {};
        uint64_t time_at_migration = kvmclock_current_nsec(s);

        s->clock_valid = false;

        /* We can't rely on the migrated clock value, just discard it */
        if (time_at_migration) {
            s->clock = time_at_migration;
        }

        data.clock = s->clock;
        ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);

So you need to send that "ns" value (difference of two clock reads) separately.

Quoted 9 lines

Quoted 4 lines

Oh, and this does introduce a minor bug to this patch -- the time counted by KVM_GET_CLOCK is has different frequency CLOCK_MONOTONIC. Not accounting for that is bearable.

Not really, I was going to point that out when I got to replying with a review. :)

Paolo

2016-11-04 16:57+0100, Paolo Bonzini:

Quoted 25 lines · Radim Krčmář

Quoted 13 lines

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 6 lines

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock. We wouldn't need this advance_clock hack if we could just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

There are two cases:

- migrating a paused guest

Quoted 5 lines

- pausing at the end of migration

In the first case, kvmclock_vm_state_change's !running branch will see state == RUN_STATE_FINISH_MIGRATE && s->clock_valid. In the second case, it will see state == RUN_STATE_FINISH_MIGRATE && !s->clock_valid.

I lift my case, marcelo's said that stopping the time is a feature ... (*kittens die*)

Quoted 26 lines · Radim Krčmář

2016-11-04 16:57+0100, Paolo Bonzini:

Quoted 25 lines · Radim Krčmář

Quoted 13 lines

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 6 lines

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock. We wouldn't need this advance_clock hack if we could just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

There are two cases:

- migrating a paused guest

Quoted 8 lines

Quoted 5 lines

- pausing at the end of migration

In the first case, kvmclock_vm_state_change's !running branch will see state == RUN_STATE_FINISH_MIGRATE && s->clock_valid. In the second case, it will see state == RUN_STATE_FINISH_MIGRATE && !s->clock_valid.

I lift my case, marcelo's said that stopping the time is a feature ... (*kittens die*)

Sorry to chime in in the middle of the thread, but I wonder how happy the guests are with this behavior. Intuitively pausing or snapshotting feels like closing the lid of a laptop, so every time I see the guest waking up in the past after a pause I get confused. It may also be unexpected by Windows guests who never had this overflow problem but now, being tied up with kvmclock, have to stop the time while in pause, too.

Roman.

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 9 lines · Radim Krčmář

Quoted 8 lines

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

Quoted 4 lines

Quoted 2 lines

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock.

We wouldn't need this advance_clock hack if we could
just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

Quoted 7 lines

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

I agree, that sounds like a nice improvement.

Quoted 13 lines

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 4 lines

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock.

Overflow when reading clocksource, in the timer interrupt.

Quoted 3 lines

We wouldn't need this advance_clock hack if we could
just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

People have requested that CLOCK_MONOTONIC stops when vm suspends.

Quoted 4 lines

Quoted 4 lines

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

Quoted 5 lines

Quoted 3 lines

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

I agree, that sounds like a nice improvement.

I fail to see how this (KVM_GET_CLOCK using raw value (vs NTP corrected) relates to clocksource overflow and CLOCK_MONOTONIC stop requests.

-----

Todo list (collective???):

1) Implement suggestion to Roman:
"Subject: Re: [PATCH] x86/kvm: fix condition to update kvm master clocks" to handle the time backwards when updating masterclock from kernel clock.

2) Review TSC scaling support (make sure scaled TSC is propagated properly everywhere).

3) Enable migration with invariant TSC.

4) Fullvirt fix for local APIC deadline timer bug

(BTW Paolo Windows is also vulnerable right).

2016-11-04 14:24-0200, Marcelo Tosatti:

Quoted 14 lines · Radim Krčmář

Quoted 13 lines

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

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

Quoted 5 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock.

Overflow when reading clocksource, in the timer interrupt.

Is it still there? I wonder why 10 minutes is the limit. :)

Quoted 6 lines

Quoted 3 lines

We wouldn't need this advance_clock hack if we could
just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

People have requested that CLOCK_MONOTONIC stops when vm suspends.

Ugh, we could have done that on the OS level,

but stopping kvmclock
means that we sabotaged CLOCK_BOOTTIME as it doesn't return CLOCK_MONOTONIC + time spent in suspend anymore.

And kvmclock is defined to count nanosecond (slightly different in practice) since some point in time, so pausing it is not really valid.

Quoted 10 lines

Quoted 9 lines

Quoted 7 lines

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

I agree, that sounds like a nice improvement.

Quoted 2 lines

I fail to see how this (KVM_GET_CLOCK using raw value (vs NTP corrected) relates to clocksource overflow and CLOCK_MONOTONIC stop requests.

If the guest uses master clock, then kvmclock drifts away from BOOTTIME, so returning the value of kvmclock using kernel_ns() would introduce a drift when setting the clock -- we must read the same way that the guest does.

Quoted 9 lines

-----

Todo list (collective???):

1) Implement suggestion to Roman:
"Subject: Re: [PATCH] x86/kvm: fix condition to update kvm master clocks" to handle the time backwards when updating masterclock from kernel clock.

Quoted 6 lines

2) Review TSC scaling support (make sure scaled TSC is propagated properly everywhere).

3) Enable migration with invariant TSC.

4) Fullvirt fix for local APIC deadline timer bug

The list looks good.

I'll resume work on (4) now that rework of APIC timers is merged. It's going to be ugly on the guest side, because linux could be using it even when not using kvmclock for sched clock ...

From previous discussion on the thread Eduardo started:

Quoted 7 lines

Quoted 2 lines

Actually, a nicer fix would be to check the different frequencies and scale the deadline relative to the difference.

You cannot know what exactly the guest was thinking when it set the TSC
deadline. Perhaps it wanted an interrupt when the TSC was exactly 9876543210.

You can't expect the correlation between TSC value and timer interrupt execution to be precise, because of the delay between HW timer expiration and interrupt execution.

So you have to live with the fact that the TSC deadline timer can be late (which is the same thing as with your paravirt solution, in case of migration to host with faster TSC freq) (which to me renders the argument above invalid).

Solution for old guests and new guests:
Just save how far ahead in the future the TSC deadline timer is supposed
to expire on the source (in ns), in the destination save all registers (but disable TSC deadline timer injection), arm a timer in QEMU for expiration time, reenable TSC deadline timer reinjection.

Quoted 1 line

(BTW Paolo Windows is also vulnerable right).

Ugh, we can't do much more than disabling TSC deadline there until QEMU can migrate it.

Quoted 15 lines

2016-11-04 14:24-0200, Marcelo Tosatti:

Quoted 14 lines · Radim Krčmář

Quoted 13 lines

2016-11-04 16:33+0100, Paolo Bonzini:

Quoted 12 lines · Radim Krčmář

Quoted 10 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

What Linux bug? The one that makes us use kvmclock_current_nsec?

No, the one that forced Marcelo to add the 10 minute limit to the advance_clock.

Overflow when reading clocksource, in the timer interrupt.

Is it still there? I wonder why 10 minutes is the limit. :)

Second question: I don't know, i guess it started as a "lets clamp this to values that make sense to catch any potential offenders" but now i think it makes sense to say:

"because it does not make sense to search for the smaller overflow of Linux guests and use that".

So 10minutes works for me using the sentence:
"migration should not take longer than 10 minutes".

You prefer to drop that 10 minutes check?

First question:

I suppose so: disable CLOCK_MONOTONIC counting on pause, pause your VM for two days, and resume.

timekeeping_resume

        cycle_now = tk->tkr_mono.read(clock);
        if ((clock->flags & CLOCK_SOURCE_SUSPEND_NONSTOP) &&
                cycle_now > tk->tkr_mono.cycle_last) {
                u64 num, max = ULLONG_MAX;
                u32 mult = clock->mult;
                u32 shift = clock->shift;
                s64 nsec = 0;

                cycle_delta = clocksource_delta(cycle_now,
tk->tkr_mono.cycle_last,
                                                tk->tkr_mono.mask);

                /*
                 * "cycle_delta * mutl" may cause 64 bits overflow, if
                 * the
                 * suspended time is too long. In that case we need do
                 * the
                 * 64 bits math carefully

                 */
                do_div(max, mult);
                if (cycle_delta > max) {
                        num = div64_u64(cycle_delta, max);
                        nsec = (((u64) max * mult) >> shift) * num;
                        cycle_delta -= num * max;
                }
                nsec += ((u64) cycle_delta * mult) >> shift;

For example.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

We wouldn't need this advance_clock hack if we could
just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
clock should count only if vm is running").

People have requested that CLOCK_MONOTONIC stops when vm suspends.

Ugh, we could have done that on the OS level,

Done it how at the OS level ?

Quoted 3 lines

but stopping kvmclock
means that we sabotaged CLOCK_BOOTTIME as it doesn't return CLOCK_MONOTONIC + time spent in suspend anymore.

It does:
So this patchset introduces CLOCK_BOOTTIME, which is identical to CLOCK_MONOTONIC, but includes any time spent in suspend.

"suspend" refers to suspend-to-RAM, not "virtual machine stopped".

CLOCK_BOOTTIME fails to work when you extend "suspend" to "suspend AND virtual machine pause or restoration".

(if you want that, can be fixed easily i supposed, but nobody ever asked for it).

Quoted 2 lines

And kvmclock is defined to count nanosecond (slightly different in practice) since some point in time, so pausing it is not really valid.

Sorry, where is that defined? Nobody makes that assumption (that kvmclock should be correlated to some physical time and that it can't stop counting).

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 7 lines

It should work with 4.9-rc (well, once Linus applies my pull request). 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return the raw value from the kernel timekeeper.

I'm thinking that we should add a KVM capability for this, and skip kvmclock_current_nsec if the capability is present. The first part is trivial, so we can do it even during Linux rc period.

I agree, that sounds like a nice improvement.

Quoted 7 lines

Quoted 2 lines

I fail to see how this (KVM_GET_CLOCK using raw value (vs NTP corrected) relates to clocksource overflow and CLOCK_MONOTONIC stop requests.

If the guest uses master clock, then kvmclock drifts away from BOOTTIME, so returning the value of kvmclock using kernel_ns() would introduce a drift when setting the clock -- we must read the same way that the guest does.

Does not work:

(periodic incremental reads from TSC, using the offset from
previous TSC read, storing to scaling to a memory location)
and reads interpolating with TSC (what kernel does)

!= (different values than)

(TSC scaled to nanoseconds)

Its not obvious to me this should be the case, perhaps if done carefully can work.

(See the thread with Roman, this was determined empirically).

Quoted 16 lines

Quoted 15 lines

-----

Todo list (collective???):

1) Implement suggestion to Roman:
"Subject: Re: [PATCH] x86/kvm: fix condition to update kvm master clocks" to handle the time backwards when updating masterclock from kernel clock.

2) Review TSC scaling support (make sure scaled TSC is propagated properly everywhere).

3) Enable migration with invariant TSC.

4) Fullvirt fix for local APIC deadline timer bug

Quoted 5 lines

The list looks good.

I'll resume work on (4) now that rework of APIC timers is merged. It's going to be ugly on the guest side, because linux could be using it even when not using kvmclock for sched clock ...

From previous discussion on the thread Eduardo started:

Quoted 4 lines

Quoted 1 line

(BTW Paolo Windows is also vulnerable right).

Ugh, we can't do much more than disabling TSC deadline there until QEMU can migrate it.

So the idea was to convert to nanoseconds, count a timer in nanoseconds, and when it expires inject immediatelly. (Yes its ugly).

(btw i suppose you can talk to destination via the command interface on the postcopy patches now), say to get the destination tsc frequency.

2016-11-04 07:43-0200, Marcelo Tosatti:

Quoted 8 lines

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Quoted 2 lines

Please do not apply this yet as some codepaths still need checking, submitting early for comments.

The time computation looks ok.

We could make it slightly more precise by returning the CLOCK_MONOTONIC at which KVM_GET_CLOCK is read with the IOCTL, but we don't account the migration time anyway, so that precision would be wasted.

Quoted 7 lines

Signed-off-by Marcelo Tosatti <[email protected]>

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

@@ -100,6 +106,11 @@ static void kvmclock_vm_state_change(void *opaque, int running,
             s->clock = time_at_migration;
         }
 

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

Quoted 4 lines

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

+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }

Can't the advance_clock added to the migrated KVMClockState instead of passing it as another parameter?

(It is sad that we can't just query KVMClockState in kvmclock_pre_save
because of the Linux bug.)

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

Quoted 12 lines

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

@@ -135,6 +146,18 @@ static void kvmclock_vm_state_change(void *opaque, int running,
             abort();
         }
         s->clock = data.clock;
+        /*
+         * Transition from VM-running to VM-stopped via migration?
+         * Record when the VM was stopped.
+         */
+
+        if (state == RUN_STATE_FINISH_MIGRATE &&
+            !migration_in_postcopy(migrate_get_current())) {
+            clock_gettime(CLOCK_MONOTONIC, &s->t_aftervmstop);

How big (more like small) was the clock delta between here and kvmclock_pre_save with postcopy?

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

Quoted 8 lines

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

+        /*
+         * Transition from VM-running to VM-stopped via migration?
+         * Record when the VM was stopped.
+         */
+
+        if (state == RUN_STATE_FINISH_MIGRATE &&
+            !migration_in_postcopy(migrate_get_current())) {
+            clock_gettime(CLOCK_MONOTONIC, &s->t_aftervmstop);

How big (more like small) was the clock delta between here and kvmclock_pre_save with postcopy?

Thanks.

Quoted 12 lines

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

Quoted 8 lines

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

+        /*
+         * Transition from VM-running to VM-stopped via migration?
+         * Record when the VM was stopped.
+         */
+
+        if (state == RUN_STATE_FINISH_MIGRATE &&
+            !migration_in_postcopy(migrate_get_current())) {
+            clock_gettime(CLOCK_MONOTONIC, &s->t_aftervmstop);

How big (more like small) was the clock delta between here and kvmclock_pre_save with postcopy?

Quoted 1 line

Thanks.

qemu-system-x86_64: postcopy_ram_supported_by_host: userfaultfd not
available: Function not implemented

But should be about the same as precopy+this patch (guess as i don't know the postcopy path).

2016-11-04 15:07-0200, Marcelo Tosatti:

Quoted 18 lines · Radim Krčmář

Quoted 9 lines

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

Quoted 8 lines

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

+        /*
+         * Transition from VM-running to VM-stopped via migration?
+         * Record when the VM was stopped.
+         */
+
+        if (state == RUN_STATE_FINISH_MIGRATE &&
+            !migration_in_postcopy(migrate_get_current())) {
+            clock_gettime(CLOCK_MONOTONIC, &s->t_aftervmstop);

Quoted 4 lines

How big (more like small) was the clock delta between here and kvmclock_pre_save with postcopy?

Thanks.

qemu-system-x86_64: postcopy_ram_supported_by_host: userfaultfd not
available: Function not implemented

Quoted 2 lines

But should be about the same as precopy+this patch (guess as i don't know the postcopy path).

I was wondering about the improvement we could achieve by not excluding postcopy from the time fixup. (i.e. how much time elapses between pausing and migrating the vm in postcopy)

I would also guess that it is not significant.

2016-11-04 15:07-0200, Marcelo Tosatti:

Quoted 18 lines · Radim Krčmář

Quoted 9 lines

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

Quoted 8 lines

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

+        /*
+         * Transition from VM-running to VM-stopped via migration?
+         * Record when the VM was stopped.
+         */
+
+        if (state == RUN_STATE_FINISH_MIGRATE &&
+            !migration_in_postcopy(migrate_get_current())) {
+            clock_gettime(CLOCK_MONOTONIC, &s->t_aftervmstop);

Quoted 4 lines

How big (more like small) was the clock delta between here and kvmclock_pre_save with postcopy?

Thanks.

qemu-system-x86_64: postcopy_ram_supported_by_host: userfaultfd not
available: Function not implemented

Quoted 2 lines

But should be about the same as precopy+this patch (guess as i don't know the postcopy path).

I was wondering about the improvement we could achieve by not excluding postcopy from the time fixup. (i.e. how much time elapses between pausing and migrating the vm in postcopy)

I would also guess that it is not significant.

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

Signed-off-by Marcelo Tosatti <[email protected]>

---

v2: use subsection (Juan Quintela)
fix older machine types support

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

Signed-off-by Marcelo Tosatti <[email protected]>

---

v2: use subsection (Juan Quintela)
fix older machine types support

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

index 0f75dd3..a2a02ac 100644
@@ -22,9 +22,11 @@
 #include "kvm_i386.h"
 #include "hw/sysbus.h"
 #include "hw/kvm/clock.h"
+#include "migration/migration.h"
 
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
+#include <time.h>
 
 #define TYPE_KVM_CLOCK "kvmclock"
 #define KVM_CLOCK(obj) OBJECT_CHECK(KVMClockState, (obj), TYPE_KVM_CLOCK)

@@ -35,7 +37,13 @@ typedef struct KVMClockState {
     /*< public >*/
 
     uint64_t clock;
+    uint64_t ns;
     bool clock_valid;
+
+    uint64_t advance_clock;
+    struct timespec t_aftervmstop;
+
+    bool adv_clock_enabled;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -100,6 +108,11 @@ static void kvmclock_vm_state_change(void *opaque, int running,
             s->clock = time_at_migration;
         }
 
+        if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
+            s->clock += s->advance_clock;
+            s->advance_clock = 0;
+        }
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -135,6 +148,18 @@ static void kvmclock_vm_state_change(void *opaque, int running,
             abort();
         }
         s->clock = data.clock;
+        /*
+         * Transition from VM-running to VM-stopped via migration?
+         * Record when the VM was stopped.
+         */
+
+        if (state == RUN_STATE_FINISH_MIGRATE &&
+            !migration_in_postcopy(migrate_get_current())) {
+            clock_gettime(CLOCK_MONOTONIC, &s->t_aftervmstop);
+        } else {
+            s->t_aftervmstop.tv_sec = 0;
+            s->t_aftervmstop.tv_nsec = 0;
+        }
 
         /*
          * If the VM is stopped, declare the clock state valid to

@@ -152,6 +177,77 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static uint64_t clock_delta(struct timespec *before, struct timespec *after)
+{
+    if (before->tv_sec > after->tv_sec ||
+        (before->tv_sec == after->tv_sec &&
+         before->tv_nsec > after->tv_nsec)) {
+        fprintf(stderr, "clock_delta failed: before=(%ld sec, %ld nsec),"
+                        "after=(%ld sec, %ld nsec)\n", before->tv_sec,
+                        before->tv_nsec, after->tv_sec, after->tv_nsec);
+        abort();
+    }
+
+    return (after->tv_sec - before->tv_sec) * 1000000000ULL +
+            after->tv_nsec - before->tv_nsec;
+}
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    struct timespec now;
+    uint64_t ns;
+
+    if (s->t_aftervmstop.tv_sec == 0) {
+        return;
+    }
+
+    clock_gettime(CLOCK_MONOTONIC, &now);
+
+    ns = clock_delta(&s->t_aftervmstop, &now);
+
+    /*
+     * Linux guests can overflow if time jumps
+     * forward in large increments.
+     * Cap maximum adjustment to 10 minutes.
+     */
+    ns = MIN(ns, 600000000000ULL);
+
+    if (s->clock + ns > s->clock) {
+        s->ns = ns;
+    }
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    KVMClockState *s = opaque;
+
+    /* save the value from incoming migration */
+    s->advance_clock = s->ns;
+
+    return 0;
+}
+
+static bool kvmclock_ns_needed(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->adv_clock_enabled;
+}
+
+static const VMStateDescription kvmclock_advance_ns = {
+    .name = "kvmclock/advance_ns",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_ns_needed,
+    .pre_save = kvmclock_pre_save,
+    .post_load = kvmclock_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(ns, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,

@@ -159,15 +255,25 @@ static const VMStateDescription kvmclock_vmsd = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_advance_ns,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("advance_clock", KVMClockState, adv_clock_enabled, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {

include/hw/i386/pc.h +0 -0

index 98dc772..243352e 100644
@@ -370,6 +370,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 #define PC_COMPAT_2_7 \

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

Quoted 33 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 5 lines

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

It seems unlikely to me that x86 is the only one that will want to do something similar.

Quoted 38 lines · Dr. David Alan Gilbert

Quoted 37 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

Quoted 4 lines

Quoted 1 line

In theory, yes. In practice, i don't see the need for this...

It seems unlikely to me that x86 is the only one that will want to do something similar.

Can't they copy what kvmclock is doing today?

Quoted 41 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 40 lines · Dr. David Alan Gilbert

Quoted 38 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

Quoted 4 lines

Quoted 2 lines

It seems unlikely to me that x86 is the only one that will want to do something similar.

Can't they copy what kvmclock is doing today?

We shouldn't have copies of code all over should we?

Dave

Quoted 45 lines · Dr. David Alan Gilbert

Quoted 44 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 42 lines · Dr. David Alan Gilbert

Quoted 33 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 5 lines

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

It seems unlikely to me that x86 is the only one that will want to do something similar.

Quoted 5 lines

Quoted 1 line

Can't they copy what kvmclock is doing today?

We shouldn't have copies of code all over should we?

Dave

Fine i'll add a notifier.

Quoted 39 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 38 lines · Dr. David Alan Gilbert

Quoted 37 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

In theory, yes. In practice, i don't see the need for this...

It seems unlikely to me that x86 is the only one that will want to do something similar.

Can't they copy what kvmclock is doing today?

We shouldn't have copies of code all over should we?

Quoted 42 lines · Dr. David Alan Gilbert

Quoted 41 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 40 lines · Dr. David Alan Gilbert

Quoted 38 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

It seems unlikely to me that x86 is the only one that will want to do something similar.

Can't they copy what kvmclock is doing today?

We shouldn't have copies of code all over should we?

Let's cross the bridge when we get there.

For now I'm more interested in having a version of the patch that is clean and doesn't need advance_clock.

Paolo

Quoted 50 lines · * Paolo Bonzini ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 45 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 34 lines · Dr. David Alan Gilbert

Quoted 33 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

It seems unlikely to me that x86 is the only one that will want to do something similar.

Can't they copy what kvmclock is doing today?

Quoted 3 lines

Quoted 1 line

We shouldn't have copies of code all over should we?

Let's cross the bridge when we get there.

That will mean it has the migration data in the wrong place and any other clocks that need to be incremented by the same offset will need a hook or be inconsistent with this calculation.

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 45 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 34 lines · Dr. David Alan Gilbert

Quoted 33 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert

Quoted 5 lines · * Marcelo Tosatti ([email protected]) wrote:

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

It seems unlikely to me that x86 is the only one that will want to do something similar.

Can't they copy what kvmclock is doing today?

Quoted 1 line

We shouldn't have copies of code all over should we?

Let's cross the bridge when we get there.

For now I'm more interested in having a version of the patch that is clean and doesn't need advance_clock.

Paolo

Destination has to run the following logic:

If (source has KVM_CAP_ADVANCE_CLOCK)
use KVM_GET_CLOCK value
Else
read pvclock from guest

To support migration from older QEMU versions which do not have KVM_CAP_ADVANCE_CLOCK (or new QEMU versions running on old hosts without KVM_CAP_ADVANCE_CLOCK).

I don't see any clean way to give that information, except changing the migration format to pass "host: kvm_cap_advance_clock=true/false" information.

Quoted 11 lines · Marcelo Tosatti

Destination has to run the following logic:

If (source has KVM_CAP_ADVANCE_CLOCK)
use KVM_GET_CLOCK value
Else
read pvclock from guest

To support migration from older QEMU versions which do not have KVM_CAP_ADVANCE_CLOCK (or new QEMU versions running on old hosts without KVM_CAP_ADVANCE_CLOCK).

Quoted 3 lines

I don't see any clean way to give that information, except changing the migration format to pass "host: kvm_cap_advance_clock=true/false" information.

If you make it only affect new machine types, you could transmit a dummy clock value such as -1 if the source does not have KVM_CLOCK_TSC_STABLE.

Paolo

Quoted 6 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 5 lines

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

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

2016-11-08 3:41 GMT+08:00 Marcelo Tosatti <[email protected]>:

Quoted 16 lines · Dr. David Alan Gilbert

Quoted 6 lines · * Marcelo Tosatti ([email protected]) wrote:

Quoted 5 lines

This patch, relative to pre-copy migration codepath, measures the time between vm_stop() and pre_save(), which includes copying the remaining RAM to destination, and advances the clock by that amount.

Quoted 8 lines

Quoted 4 lines

In a VM with 5 seconds downtime, this reduces the guest clock difference on destination from 5s to 0.2s.

Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.

One thing that bothers me is that it's only this clock that's getting corrected; doesn't it cause things to get upset when one clock moves and the others dont?

Quoted 6 lines

If you are correlating the clocks, then yes.

Older Linux guests get upset (marking the TSC clocksource unstable because the watchdog checks TSC vs kvmclock), but there is a workaround for it in newer guests
(kvmclock interface to notify watchdog to not complain).

Could you point out which interface? I didn't find it.

Regards,
Wanpeng Li

Quoted 22 lines

Note marking TSC clocksource unstable on older guests is harmless because kvmclock is the standard clocksource.

For Windows guests, i don't know that Windows correlates between different clocks.

That is, there is relative control as to which software reads kvmclock or Windows TIMER MSR, so i don't see the need to advance every clock exposed.

Quoted 3 lines

Shouldn't the pause delay be recorded somewhere architecturally independent and then be a thing that kvm-clock happens to use and other clocks might as well?

In theory, yes. In practice, i don't see the need for this...

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