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 12:50:57 -0200 Message-ID: <20161114145054.GA28663@amt.cnet> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <20161114140028.GA25935@amt.cnet> <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> 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]:58616 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751496AbcKNOvV (ORCPT ); Mon, 14 Nov 2016 09:51:21 -0500 Content-Disposition: inline In-Reply-To: <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 14, 2016 at 03:22:02PM +0100, Paolo Bonzini wrote: > > > 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. Well, i didnt want to mix the meaning of the variables: + /* whether machine supports reliable KVM_GET_CLOCK */ + bool mach_use_reliable_get_clock; + + /* whether source host supported reliable KVM_GET_CLOCK */ + bool src_use_reliable_get_clock; See the comments on top (later if you look at the variable, then have to think: well it has one name, but its disabled by that other path as well, so its more than its name,etc...). > I'm thinking "mach_use_reliable_get_clock is just for migration, Thats whether the machine supports it. New machines have it enabled, olders don't. > src_use_reliable_get_clock is the state". Thats whether the migration source supported it. > Perhaps you're thinking of > enabling/disabling the whole new code for old machines? source destination behaviour supports supports on migration use s->clock, on stop/cont as well supports ~supports on migration use s->clock, on stop/cont read from guest mem ~support supports on migration read from guest, on stop/cont use kvm_get_clock/kvm_set_clock ~support ~support always read from guest memory Thats what should happen (and thats what the patch should implement). "support" means host supports your new KVM_GET_CLOCK/KVM_SET_CLOCK. > What is the > advantage? Well its necessary to use the correct thing, otherwise you see a time backwards event. > > >>> + } > >>> + } > >>> > >>> - /* 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. "needed" is required so that the migration between: SRC DEST BEHAVIOUR ~support supports on migration read from guest, on stop/cont use kvm_get_clock/kvm_set_clock Destination does not use KVM_GET_CLOCK value (which is broken and should not be used). > If you return true here, you can still > migrate a "false" value for src_use_reliable_get_clock. But the source only uses a reliable KVM_GET_CLOCK if both conditions are true. And the subsection is only needed if the source uses a reliable KVM_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, Actually its "has a reliable KVM_GET_CLOCK" (which returns get_kernel_clock() + (rdtsc() - tsc_timestamp), "broken KVM_GET_CLOCK" = get_kernel_clock() > and then the idea is to treat > clock,src_use_reliable_get_clock as one tuple that is updated atomically. > > Paolo Hum, not sure i get this... From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51083) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6IbU-0007th-GB for qemu-devel@nongnu.org; Mon, 14 Nov 2016 09:51:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6IbR-00067j-Qo for qemu-devel@nongnu.org; Mon, 14 Nov 2016 09:51:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62497) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6IbR-00067e-JQ for qemu-devel@nongnu.org; Mon, 14 Nov 2016 09:51:21 -0500 Date: Mon, 14 Nov 2016 12:50:57 -0200 From: Marcelo Tosatti Message-ID: <20161114145054.GA28663@amt.cnet> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <20161114140028.GA25935@amt.cnet> <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> 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 03:22:02PM +0100, Paolo Bonzini wrote: > > > 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. Well, i didnt want to mix the meaning of the variables: + /* whether machine supports reliable KVM_GET_CLOCK */ + bool mach_use_reliable_get_clock; + + /* whether source host supported reliable KVM_GET_CLOCK */ + bool src_use_reliable_get_clock; See the comments on top (later if you look at the variable, then have to think: well it has one name, but its disabled by that other path as well, so its more than its name,etc...). > I'm thinking "mach_use_reliable_get_clock is just for migration, Thats whether the machine supports it. New machines have it enabled, olders don't. > src_use_reliable_get_clock is the state". Thats whether the migration source supported it. > Perhaps you're thinking of > enabling/disabling the whole new code for old machines? source destination behaviour supports supports on migration use s->clock, on stop/cont as well supports ~supports on migration use s->clock, on stop/cont read from guest mem ~support supports on migration read from guest, on stop/cont use kvm_get_clock/kvm_set_clock ~support ~support always read from guest memory Thats what should happen (and thats what the patch should implement). "support" means host supports your new KVM_GET_CLOCK/KVM_SET_CLOCK. > What is the > advantage? Well its necessary to use the correct thing, otherwise you see a time backwards event. > > >>> + } > >>> + } > >>> > >>> - /* 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. "needed" is required so that the migration between: SRC DEST BEHAVIOUR ~support supports on migration read from guest, on stop/cont use kvm_get_clock/kvm_set_clock Destination does not use KVM_GET_CLOCK value (which is broken and should not be used). > If you return true here, you can still > migrate a "false" value for src_use_reliable_get_clock. But the source only uses a reliable KVM_GET_CLOCK if both conditions are true. And the subsection is only needed if the source uses a reliable KVM_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, Actually its "has a reliable KVM_GET_CLOCK" (which returns get_kernel_clock() + (rdtsc() - tsc_timestamp), "broken KVM_GET_CLOCK" = get_kernel_clock() > and then the idea is to treat > clock,src_use_reliable_get_clock as one tuple that is updated atomically. > > Paolo Hum, not sure i get this...