From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [QEMU PATCH] kvmclock: advance clock by time window between
 vm_stop and pre_save
Date: Fri, 4 Nov 2016 19:47:47 -0200
Message-ID: <20161104214747.GA17560@amt.cnet>
References: <20161104094322.GA16930@amt.cnet>
 <20161104152522.GC5388@potion>
 <1c69a083-eef0-8fa0-0e74-5a4e25a066a0@redhat.com>
 <20161104154827.GD5388@potion>
 <1217f6ce-8143-8d0e-97f6-470926438992@redhat.com>
 <20161104171605.GE5388@potion>
 <595276440.10962400.1478294976364.JavaMail.zimbra@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        kvm@vger.kernel.org, qemu-devel <qemu-devel@nongnu.org>,
        "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
        Juan Quintela <quintela@redhat.com>,
        Eduardo Habkost <ehabkost@redhat.com>,
        Roman Kagan <rkagan@virtuozzo.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:37504 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752946AbcKDVsJ (ORCPT <rfc822;kvm@vger.kernel.org>);
        Fri, 4 Nov 2016 17:48:09 -0400
Content-Disposition: inline
In-Reply-To: <595276440.10962400.1478294976364.JavaMail.zimbra@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Fri, Nov 04, 2016 at 05:29:36PM -0400, Paolo Bonzini wrote:
> 
> > >> 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.
> > 
> > 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.

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.

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