From mboxrd@z Thu Jan  1 00:00:00 1970
From: Juan Quintela <quintela@redhat.com>
Subject: Re: [QEMU PATCH] kvmclock: advance clock by time window between vm_stop and pre_save
Date: Fri, 04 Nov 2016 13:28:48 +0100
Message-ID: <87h97npgxr.fsf@emacs.mitica>
References: <20161104094322.GA16930@amt.cnet>
Reply-To: quintela@redhat.com
Mime-Version: 1.0
Content-Type: text/plain
Cc: kvm@vger.kernel.org, qemu-devel <qemu-devel@nongnu.org>,
        "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Eduardo Habkost <ehabkost@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:59860 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S932486AbcKDM2y (ORCPT <rfc822;kvm@vger.kernel.org>);
        Fri, 4 Nov 2016 08:28:54 -0400
In-Reply-To: <20161104094322.GA16930@amt.cnet> (Marcelo Tosatti's message of
        "Fri, 4 Nov 2016 07:43:24 -0200")
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.