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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Nov 04, 2016 at 01:28:48PM +0100, Juan Quintela wrote:
> 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.

Optional section is more appropriate, thanks.

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

Couldnt find any...

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

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?

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

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

Thanks Juan.