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 16:00:38 +0100 Message-ID: <67bffd95-2e4e-7273-c154-a3fdfe622387@redhat.com> References: <20161114123628.703911091@redhat.com> <20161114123700.158592605@redhat.com> <20161114140028.GA25935@amt.cnet> <62d634ab-70ad-4be7-1622-f2e3a9d865fe@redhat.com> <20161114145054.GA28663@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: Eduardo Habkost , kvm@vger.kernel.org, Juan Quintela , Radim Krcmar , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" To: Marcelo Tosatti Return-path: In-Reply-To: <20161114145054.GA28663@amt.cnet> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org On 14/11/2016 15:50, Marcelo Tosatti wrote: > Well, i didnt want to mix the meaning of the variables: >=20 > + /* 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; >=20 > See the comments on top (later if you look at the variable,=20 > then have to think: well it has one name, but its disabled=20 > by that other path as well, so its more than its=20 > name,etc...). >=20 >> I'm thinking "mach_use_reliable_get_clock is just for migration, >=20 > Thats whether the machine supports it. New machines have it enabled, > olders don't. Yes. >> src_use_reliable_get_clock is the state".=20 >=20 > Thats whether the migration source supported it. But it's not used only for migration. It's used on every vmstate change (running->stop and stop->running, isn't it? I think that, apart from the migration case, it's better to use s->clock if kvmclock is stable, even on older machine types. >>>>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque) >>>>> +{ >>>>> + KVMClockState *s =3D 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=3Dtrue so that destination >>>>> + * avoids reading kvmclock from memory. >>>>> + */ >>>>> + if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_sta= ble()) { >>>>> + s->src_use_reliable_get_clock =3D true; >>>>> + } >>>>> + >>>>> + return s->src_use_reliable_get_clock; >>>>> +} >>>> >>>> Here you can just return s->mach_use_reliable_get_clock.=20 >>> >>> 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.=20 >=20 > "needed" is required so that the migration between: >=20 > SRC DEST BEHAVIOUR > ~support supports on migration read from guest, > on stop/cont use > kvm_get_clock/kvm_set_clock >=20 > Destination does not use KVM_GET_CLOCK value (which is > broken and should not be used). If needed returns false, the destination will see src_use_reliable_get_clock =3D false anyway. If needed returns true, the destination can still see src_use_reliable_get_clock =3D false if that's the value. needed src_use_reliable_get_clock effect false false use kvmclock_current_nsec false true use kvmclock_current_nsec true false use kvmclock_current_nsec true true use s->clock So the idea is: - set s->clock and s->reliable_get_clock on every KVM_GET_CLOCK - on migration source, use subsections and s->mach_use_reliable_get_clock to avoid breaking migration on pre-2.8 machine types - on migration destination, use .pre_load so that s->reliable_get_clock is initialized to false on older machine types >> If you return true here, you can still >> migrate a "false" value for src_use_reliable_get_clock. >=20 > But the source only uses a reliable KVM_GET_CLOCK if=20 > both conditions are true. >=20 > And the subsection is only needed if the source > uses a reliable KVM_GET_CLOCK. Yes, but the point of the subsection is just to avoid breaking migration format. >> 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,=20 >=20 > Actually its "has a reliable KVM_GET_CLOCK" (which returns=20 > get_kernel_clock() + (rdtsc() - tsc_timestamp),=20 >=20 > "broken KVM_GET_CLOCK" =3D get_kernel_clock() Yes. And these end up being the same. Paolo