From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55334) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2mLO-0000Jv-JF for qemu-devel@nongnu.org; Fri, 04 Nov 2016 17:48:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2mLJ-0000LE-KS for qemu-devel@nongnu.org; Fri, 04 Nov 2016 17:48:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40660) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2mLJ-0000KR-Cj for qemu-devel@nongnu.org; Fri, 04 Nov 2016 17:48:09 -0400 Date: Fri, 4 Nov 2016 19:47:47 -0200 From: Marcelo Tosatti 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 Content-Disposition: inline In-Reply-To: <595276440.10962400.1478294976364.JavaMail.zimbra@redhat.com> Subject: Re: [Qemu-devel] [QEMU PATCH] kvmclock: advance clock by time window between vm_stop and pre_save List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Radim =?utf-8?B?S3LEjW3DocWZ?= , kvm@vger.kernel.org, qemu-devel , "Dr. David Alan Gilbert" , Juan Quintela , Eduardo Habkost , Roman Kagan 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