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 04:25:23PM +0100, Radim Krčmář wrote:
> 2016-11-04 07:43-0200, 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.
> > 
> > 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.

Yes, can be enhanced later... That should be about 1us? (return to
userspace).

> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> > @@ -100,6 +106,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;
> > +        }
> 
> Can't the advance_clock added to the migrated KVMClockState instead of
> passing it as another parameter?

Don't want to be fragile to ->ns being overwritten by pre_save executing
on the destination and overwriting the value passed in from
migration... 

> (It is sad that we can't just query KVMClockState in kvmclock_pre_save
>  because of the Linux bug.)
> 
> > @@ -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?

Haven't checked, but the clock delta is now 0.2s. 
So i assume postcopy (stopping the VM, saving the device states)
should be about 0.2s.

I'll run and let you know.