Re: [QEMU PATCH] kvmclock: advance clock by time window between vm_stop and pre_save

Juan Quintela
2016-11-04T12:28:54Z

Marcelo Tosatti <mtosatti@redhat.com> 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.
>
> 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 <mtosatti@redhat.com>

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.

> +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;
> +}

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

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

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

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.

> + }
> +}
> +
> +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,
> + .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.