From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration Date: Mon, 14 Nov 2016 15:22:02 +0100 Message-ID: <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <20161114140028.GA25935@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar , Eduardo Habkost To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34070 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932607AbcKNOWH (ORCPT ); Mon, 14 Nov 2016 09:22:07 -0500 In-Reply-To: <20161114140028.GA25935@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 14/11/2016 15:00, Marcelo Tosatti wrote: > On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote: >> >> >> On 14/11/2016 13:36, Marcelo Tosatti wrote: >>> + /* local (running VM) restore */ >>> + if (s->clock_valid) { >>> + /* >>> + * if host does not support reliable KVM_GET_CLOCK, >>> + * read kvmclock value from memory >>> + */ >>> + if (!kvm_has_adjust_clock_stable()) { >>> + time_at_migration = kvmclock_current_nsec(s); >> >> Just assign to s->clock here... > > If kvmclock is not enabled, you want to use s->clock, > rather than 0. > >>> + } >>> + /* migration/savevm/init restore */ >>> + } else { >>> + /* >>> + * use s->clock in case machine uses reliable >>> + * get clock and host where vm was executing >>> + * supported reliable get clock >>> + */ >>> + if (!s->mach_use_reliable_get_clock || >>> + !s->src_use_reliable_get_clock) { >>> + time_at_migration = kvmclock_current_nsec(s); >> >> ... and here, so that time_at_migration is not needed anymore. > > Same as above. You're right. >> Also here it's enough to look at s->src_user_reliable_get_clock, because >> if s->mach_use_reliable_get_clock is false, >> s->src_use_reliable_get_clock will be false as well. > > Yes, but i like the code annotation. Ah, I think we're looking at it differently. I'm thinking "mach_use_reliable_get_clock is just for migration, src_use_reliable_get_clock is the state". Perhaps you're thinking of enabling/disabling the whole new code for old machines? What is the advantage? >>> + } >>> + } >>> >>> - /* We can't rely on the migrated clock value, just discard it */ >>> + /* We can't rely on the saved clock value, just discard it */ >>> if (time_at_migration) { >>> s->clock = time_at_migration; >> >> [...] >> >>> >>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque) >>> +{ >>> + KVMClockState *s = opaque; >>> + >>> + /* >>> + * On machine types that support reliable KVM_GET_CLOCK, >>> + * if host kernel does provide reliable KVM_GET_CLOCK, >>> + * set src_use_reliable_get_clock=true so that destination >>> + * avoids reading kvmclock from memory. >>> + */ >>> + if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) { >>> + s->src_use_reliable_get_clock = true; >>> + } >>> + >>> + return s->src_use_reliable_get_clock; >>> +} >> >> Here you can just return s->mach_use_reliable_get_clock. > > mach_use_reliable_get_clock can be true but host might not support it. Yes, but the "needed" function is only required to avoid breaking pc-i440fx-2.7 and earlier. If you return true here, you can still migrate a "false" value for src_use_reliable_get_clock. >> To set >> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look >> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags. > > KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != > KVM_GET_CLOCK returns reliable value, right? It is the same as "is using masterclock", which is actually a stricter condition than the KVM_CHECK_EXTENSION return value. The right check to use is whether masterclock is in use, and then the idea is to treat clock,src_use_reliable_get_clock as one tuple that is updated atomically. Paolo