From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration Date: Mon, 14 Nov 2016 15:13:21 -0200 Message-ID: <20161114171318.GA6336@amt.cnet> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <20161114140028.GA25935@amt.cnet> <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> <20161114145054.GA28663@amt.cnet> <67bffd95-2e4e-7273-c154-a3fdfe622387@redhat.com> <20161114154015.GA30048@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar , Eduardo Habkost To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:42348 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933841AbcKNRNo (ORCPT ); Mon, 14 Nov 2016 12:13:44 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote: > > > On 14/11/2016 16:40, Marcelo Tosatti wrote: > > 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->mach_use_reliable_get_clock; > > } > > > > > > Ah, OK, done. > > s->src_use_reliable_get_clock should not be set with > KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK. Well, thats not right: What matters is the presence of get_kvmclock_ns which returns a value that the guest sees. get_kernel_monotonic_clock() + kvmclock_offset + (rdtsc() - tsc_timestamp) IOW what the guest sees. And you changed that in commit 108b249c453dd7132599ab6dc7e435a7036c193f Author: Paolo Bonzini Date: Thu Sep 1 14:21:03 2016 +0200 KVM: x86: introduce get_kvmclock_ns And the correct behaviour (once KVM_GET_CLOCK is fixed per previous message to return rdtsc - tsc_timestamp for the non masterclock case) depends on this commit above, not on masterclock. > > So s->src_use_reliable_get_clock is only used to indicate > > to the destination that: "you can use KVM_GET_CLOCK value, > > its safe". > > Yes, we agree. I was listing all the points, not just those where we > disagree. Actually I'm not sure where we disagree, except on using > flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK... > > Paolo From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6KpJ-0002X7-5a for qemu-devel@nongnu.org; Mon, 14 Nov 2016 12:13:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6KpF-0000Xr-3A for qemu-devel@nongnu.org; Mon, 14 Nov 2016 12:13:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33686) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6KpE-0000Xc-To for qemu-devel@nongnu.org; Mon, 14 Nov 2016 12:13:45 -0500 Date: Mon, 14 Nov 2016 15:13:21 -0200 From: Marcelo Tosatti Message-ID: <20161114171318.GA6336@amt.cnet> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <20161114140028.GA25935@amt.cnet> <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> <20161114145054.GA28663@amt.cnet> <67bffd95-2e4e-7273-c154-a3fdfe622387@redhat.com> <20161114154015.GA30048@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Juan Quintela , Radim Krcmar , Eduardo Habkost On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote: > > > On 14/11/2016 16:40, Marcelo Tosatti wrote: > > 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->mach_use_reliable_get_clock; > > } > > > > > > Ah, OK, done. > > s->src_use_reliable_get_clock should not be set with > KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK. Well, thats not right: What matters is the presence of get_kvmclock_ns which returns a value that the guest sees. get_kernel_monotonic_clock() + kvmclock_offset + (rdtsc() - tsc_timestamp) IOW what the guest sees. And you changed that in commit 108b249c453dd7132599ab6dc7e435a7036c193f Author: Paolo Bonzini Date: Thu Sep 1 14:21:03 2016 +0200 KVM: x86: introduce get_kvmclock_ns And the correct behaviour (once KVM_GET_CLOCK is fixed per previous message to return rdtsc - tsc_timestamp for the non masterclock case) depends on this commit above, not on masterclock. > > So s->src_use_reliable_get_clock is only used to indicate > > to the destination that: "you can use KVM_GET_CLOCK value, > > its safe". > > Yes, we agree. I was listing all the points, not just those where we > disagree. Actually I'm not sure where we disagree, except on using > flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK... > > Paolo