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 13:37:35 -0200 Message-ID: <20161114153733.GA31848@amt.cnet> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <874m3af905.fsf@emacs.mitica> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Paolo Bonzini , Radim Krcmar , Eduardo Habkost To: Juan Quintela Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60172 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753205AbcKNPkq (ORCPT ); Mon, 14 Nov 2016 10:40:46 -0500 Content-Disposition: inline In-Reply-To: <874m3af905.fsf@emacs.mitica> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 14, 2016 at 03:09:46PM +0100, Juan Quintela wrote: > Marcelo Tosatti wrote: > > Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which > > indicates that KVM_GET_CLOCK returns a value as seen by the guest at > > that moment. > > > > For new machine types, use this value rather than reading > > from guest memory. > > > > This reduces kvmclock difference on migration from 5s to 0.1s > > (when max_downtime == 5s). > > > > Signed-off-by: Marcelo Tosatti > > Acked-by: Juan Quintela > > But, if you have to respin it .... > > > > + /* 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; > > This two names are really long, but I don't have better suggesitons :-() > > if (running) { > > struct kvm_clock_data data = {}; > > - uint64_t time_at_migration = kvmclock_current_nsec(s); > > + uint64_t time_at_migration = 0; > > This was not "time_at_migration", it was not already before, but just > now it looks really weird. (as it was already faulty, this is why it is > only a suggestion.) > > > > > - s->clock_valid = false; > > + /* 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); > > + } > > + /* 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 > > + */ > > This comment is just weird. Simplifying > /* If A and B do C */ > if (!A and || !B) { > then D(); > } > > Doing the opposite comment? > > Migration code looks rigth. Fixed those. > Once said that, I continue hating clocks. Me too, especially the biological one. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34446) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JNK-0002Cm-DG for qemu-devel@nongnu.org; Mon, 14 Nov 2016 10:40:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6JNH-0007OI-AW for qemu-devel@nongnu.org; Mon, 14 Nov 2016 10:40:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54774) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6JNG-0007OB-VL for qemu-devel@nongnu.org; Mon, 14 Nov 2016 10:40:47 -0500 Date: Mon, 14 Nov 2016 13:37:35 -0200 From: Marcelo Tosatti Message-ID: <20161114153733.GA31848@amt.cnet> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <874m3af905.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <874m3af905.fsf@emacs.mitica> 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: Juan Quintela Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Paolo Bonzini , Radim Krcmar , Eduardo Habkost On Mon, Nov 14, 2016 at 03:09:46PM +0100, Juan Quintela wrote: > Marcelo Tosatti wrote: > > Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which > > indicates that KVM_GET_CLOCK returns a value as seen by the guest at > > that moment. > > > > For new machine types, use this value rather than reading > > from guest memory. > > > > This reduces kvmclock difference on migration from 5s to 0.1s > > (when max_downtime == 5s). > > > > Signed-off-by: Marcelo Tosatti > > Acked-by: Juan Quintela > > But, if you have to respin it .... > > > > + /* 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; > > This two names are really long, but I don't have better suggesitons :-() > > if (running) { > > struct kvm_clock_data data = {}; > > - uint64_t time_at_migration = kvmclock_current_nsec(s); > > + uint64_t time_at_migration = 0; > > This was not "time_at_migration", it was not already before, but just > now it looks really weird. (as it was already faulty, this is why it is > only a suggestion.) > > > > > - s->clock_valid = false; > > + /* 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); > > + } > > + /* 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 > > + */ > > This comment is just weird. Simplifying > /* If A and B do C */ > if (!A and || !B) { > then D(); > } > > Doing the opposite comment? > > Migration code looks rigth. Fixed those. > Once said that, I continue hating clocks. Me too, especially the biological one.