On Tue, Dec 01, 2020 at 02:30:39PM +0200, Maxim Levitsky wrote: > On Mon, 2020-11-30 at 16:16 -0300, Marcelo Tosatti wrote: > > Hi Maxim, > > > > On Mon, Nov 30, 2020 at 03:35:57PM +0200, Maxim Levitsky wrote: > > > Hi! > > > > > > This is the first version of the work to make TSC migration more accurate, > > > as was defined by Paulo at: > > > https://www.spinics.net/lists/kvm/msg225525.html > > > > Description from Oliver's patch: > > > > "To date, VMMs have typically restored the guest's TSCs by value using > > the KVM_SET_MSRS ioctl for each vCPU. However, restoring the TSCs by > > value introduces some challenges with synchronization as the TSCs > > continue to tick throughout the restoration process. As such, KVM has > > some heuristics around TSC writes to infer whether or not the guest or > > host is attempting to synchronize the TSCs." > > > > Not really. The synchronization logic tries to sync TSCs during > > BIOS boot (and CPU hotplug), because the TSC values are loaded > > sequentially, say: > > > > CPU realtime TSC val > > vcpu0 0 usec 0 > > vcpu1 100 usec 0 > > vcpu2 200 usec 0 > > ... > > > > And we'd like to see all vcpus to read the same value at all times. > > > > Other than that, comment makes sense. The problem with live migration > > is as follows: > > > > We'd like the TSC value to be written, ideally, just before the first > > VM-entry a vCPU (because at the moment the TSC_OFFSET has been written, > > the vcpus tsc is ticking, which will cause a visible forward jump > > in vcpus tsc time). > > > > Before the first VM-entry is the farthest point in time before guest > > entry that one could do that. > > > > The window (or forward jump) between KVM_SET_TSC and VM-entry was about > > 100ms last time i checked (which results in a 100ms time jump forward), > > See QEMU's 6053a86fe7bd3d5b07b49dae6c05f2cd0d44e687. > > > > Have we measured any improvement with this patchset? > > Its not about this window. > It is about time that passes between the point that we read the > TSC on source system (and we do it in qemu each time the VM is paused) > and the moment that we set the same TSC value on the target. > That time is unbounded. OK. Well, its the same problem: ideally you'd want to do that just before VCPU-entry. > Also this patchset should decrease TSC skew that happens > between restoring it on multiple vCPUs as well, since > KVM_SET_TSC_STATE doesn't have to happen at the same time, > as it accounts for time passed on each vCPU. > > > Speaking of kvmclock, somewhat offtopic since this is a different issue, > I found out that qemu reads the kvmclock value on each pause, > and then 'restores' on unpause, using > KVM_SET_CLOCK (this modifies the global kvmclock offset) > > This means (and I tested it) that if guest uses kvmclock > for time reference, it will not account for time passed in > the paused state. Yes, this is necessary because otherwise there might be an overflow in the kernel time accounting code (if the clock delta is too large). > > > > > Then Paolo mentions (with >), i am replying as usual. > > > > > Ok, after looking more at the code with Maxim I can confidently say that > > > it's a total mess. And a lot of the synchronization code is dead > > > because 1) as far as we could see no guest synchronizes the TSC using > > > MSR_IA32_TSC; > > > > Well, recent BIOS'es take care of synchronizing the TSC. So when Linux > > boots, it does not have to synchronize TSC in software. > > Do you have an example of such BIOS? I tested OVMF which I compiled > from git master a few weeks ago, and I also tested this with seabios > from qemu repo, and I have never seen writes to either TSC, or TSC_ADJUST > from BIOS. Oh, well, QEMU then. > Or do you refer to the native BIOS on the host doing TSC synchronization? No, virt. > > However, upon migration (and initialization), the KVM_SET_TSC's do > > not happen at exactly the same time (the MSRs for each vCPU are loaded > > in sequence). The synchronization code in kvm_set_tsc() is for those cases. > > I agree with that, and this is one of the issues that KVM_SET_TSC_STATE > is going to fix, since it accounts for it. > > > > > > > and 2) writing to MSR_IA32_TSC_ADJUST does not trigger the > > > synchronization code in kvm_write_tsc. > > > > Not familiar how guests are using MSR_IA32_TSC_ADJUST (or Linux)... > > Lets see: > > > > > > /* > > * Freshly booted CPUs call into this: > > */ > > void check_tsc_sync_target(void) > > { > > struct tsc_adjust *cur = this_cpu_ptr(&tsc_adjust); > > unsigned int cpu = smp_processor_id(); > > cycles_t cur_max_warp, gbl_max_warp; > > int cpus = 2; > > > > /* Also aborts if there is no TSC. */ > > if (unsynchronized_tsc()) > > return; > > > > /* > > * Store, verify and sanitize the TSC adjust register. If > > * successful skip the test. > > * > > * The test is also skipped when the TSC is marked reliable. This > > * is true for SoCs which have no fallback clocksource. On these > > * SoCs the TSC is frequency synchronized, but still the TSC ADJUST > > * register might have been wreckaged by the BIOS.. > > */ > > if (tsc_store_and_check_tsc_adjust(false) || tsc_clocksource_reliable) { > > atomic_inc(&skip_test); > > return; > > } > > > > retry: > > > > I'd force that synchronization path to be taken as a test-case. > > Or even better as I suggested, we might tell the guest kernel > to avoid this synchronization path when KVM is detected > (regardless of invtsc flag) > > > > > > > > I have a few thoughts about the kvm masterclock synchronization, > > > which relate to the Paulo's proposal that I implemented. > > > > > > The idea of masterclock is that when the host TSC is synchronized > > > (or as kernel call it, stable), and the guest TSC is synchronized as well, > > > then we can base the kvmclock, on the same pair of > > > (host time in nsec, host tsc value), for all vCPUs. > > > > We _have_ to base. See the comment which starts with > > > > "Assuming a stable TSC across physical CPUS, and a stable TSC" > > > > at x86.c. > > > > > This makes the random error in calculation of this value invariant > > > across vCPUS, and allows the guest to do kvmclock calculation in userspace > > > (vDSO) since kvmclock parameters are vCPU invariant. > > > > Actually, without synchronized host TSCs (and the masterclock scheme, > > with a single base read from a vCPU), kvmclock in kernel is buggy as > > well: > > > > u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src) > > { > > unsigned version; > > u64 ret; > > u64 last; > > u8 flags; > > > > do { > > version = pvclock_read_begin(src); > > ret = __pvclock_read_cycles(src, rdtsc_ordered()); > > flags = src->flags; > > } while (pvclock_read_retry(src, version)); > > > > if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) { > > src->flags &= ~PVCLOCK_GUEST_STOPPED; > > pvclock_touch_watchdogs(); > > } > > > > if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) && > > (flags & PVCLOCK_TSC_STABLE_BIT)) > > return ret; > > > > The code that follows this (including cmpxchg) is a workaround for that > > bug. > > I understand that. I am not arguing that we shoudn't use the masterclock! > I am just saying the facts about the condition when it works. Sure. > > Workaround would require each vCPU to write to a "global clock", on > > every clock read. > > > > > To ensure that the guest tsc is synchronized we currently track host/guest tsc > > > writes, and enable the master clock only when roughly the same guest's TSC value > > > was written across all vCPUs. > > > > Yes, because then you can do: > > > > vcpu0 vcpu1 > > > > A = read TSC > > ... elapsed time ... > > > > B = read TSC > > > > delta = B - A > > > > > Recently this was disabled by Paulo > > > > What was disabled exactly? > > The running of tsc synchronization code when the _guest_ writes the TSC. > > Which changes two things: > 1. If the guest de-synchronizes its TSC, we won't disable master clock. > 2. If the guest writes similar TSC values on each vCPU we won't detect > this as synchronization attempt, replace this with exactly the same > value and finally re-enable the master clock. > > I argue that this change is OK, because Linux guests don't write to TSC at all, > the virtual BIOSes seems not to write there either, and the only case in which > the Linux guest tries to change its TSC is on CPU hotplug as you mention and > it uses TSC_ADJUST, that currently doesn't trigger TSC synchronization code in > KVM anyway, so it is broken already. > > However I also argue that we should mention this in documentation just in case, > and we might also want (also just in case) to make Linux guests avoid even trying to > touch TSC_ADJUST register when running under KVM. > > To rehash my own words, the KVM_CLOCK_TSC_STABLE should be defined as: > 'kvmclock is vCPU invariant, as long as the guest doesn't mess with its TSC'. > > Having said all that, now that I know tsc sync code, and the > reasons why it is there, I wouldn't be arguing about putting it back either. Agree. > > > and I agree with this, because I think > > > that we indeed should only make the guest TSC synchronized by default > > > (including new hotplugged vCPUs) and not do any tsc synchronization beyond that. > > > (Trying to guess when the guest syncs the TSC can cause more harm that good). > > > > > > Besides, Linux guests don't sync the TSC via IA32_TSC write, > > > but rather use IA32_TSC_ADJUST which currently doesn't participate > > > in the tsc sync heruistics. > > > > Linux should not try to sync the TSC with IA32_TSC_ADJUST. It expects > > the BIOS to boot with synced TSCs. > > > > So i wonder what is making it attempt TSC sync in the first place? > > CPU hotplug. And the guest doesn't really write to TSC_ADJUST > since it's measurement code doesn't detect any tsc warps. > > I was just thinking that in theory since, this is a VM, and it can be > interrupted at any point, the measurement code should sometimes fall, > and cause trouble. > I didn't do much homework on this so I might be overreacting. That is true (and you can see it with a CPU starved guest). > As far as I see X86_FEATURE_TSC_RELIABLE was done mostly to support > running under Hyper-V and VMWARE, and these should be prone to similar > issues, supporting my theory. > > > > > (one might also want to have Linux's synchronization via IA32_TSC_ADJUST > > working, but it should not need to happen in the first place, as long as > > QEMU and KVM are behaving properly). > > > > > And as far as I know, Linux guest is the primary (only?) user of the kvmclock. > > > > Only AFAIK. > > > > > I *do think* however that we should redefine KVM_CLOCK_TSC_STABLE > > > in the documentation to state that it only guarantees invariance if the guest > > > doesn't mess with its own TSC. > > > > > > Also I think we should consider enabling the X86_FEATURE_TSC_RELIABLE > > > in the guest kernel, when kvm is detected to avoid the guest even from trying > > > to sync TSC on newly hotplugged vCPUs. > > > > See 7539b174aef405d9d57db48c58390ba360c91312. > > I know about this, and I personally always use invtsc > with my VMs. Well, we can't make it (-cpu xxx,+invtsc) the default if vm-stop/vm-cont are unstable with TSC! > > Was hoping to make that (-cpu xxx,+invtsc) the default in QEMU once invariant TSC code > > becomes stable. Should be tested enough by now? > > The issue is that Qemu blocks migration when invtsc is set, based on the > fact that the target machine might have different TSC frequency and no > support for TSC scaling. > There was a long debate on this long ago. Oh right. > It is possible though to override this by specifying the exact frequency > you want the guest TSC to run at, by using something like > (tsc-frequency=3500000000) > I haven't checked if libvirt does this or not. It does. > I do think that as long as the user uses modern CPUs (which have stable TSC > and support TSC scaling), there is no reason to disable invtsc, and > therefore no reason to use kvmclock. Yep. TSC is faster. > > > (The guest doesn't end up touching TSC_ADJUST usually, but it still might > > > in some cases due to scheduling of guest vCPUs) > > > > > > (X86_FEATURE_TSC_RELIABLE short circuits tsc synchronization on CPU hotplug, > > > and TSC clocksource watchdog, and the later we might want to keep). > > > > The latter we want to keep. > > > > > For host TSC writes, just as Paulo proposed we can still do the tsc sync, > > > unless the new code that I implemented is in use. > > > > So Paolo's proposal is to > > > > "- for live migration, userspace is expected to use the new > > KVM_GET/SET_TSC_PRECISE (or whatever the name will be) to get/set a > > (nanosecond, TSC, TSC_ADJUST) tuple." > > > > Makes sense, so that no time between KVM_SET_TSC and > > MSR_WRITE(TSC_ADJUST) elapses, which would cause the TSC to go out > > of what is desired by the user. > > > > Since you are proposing this new ioctl, perhaps its useful to also > > reduce the 100ms jump? > > Yep. As long as target and destantion clocks are synchronized, > it should make it better. > > > > > "- for live migration, userspace is expected to use the new > > KVM_GET/SET_TSC_PRECISE (or whatever the name will be) to get/set a > > (nanosecond, TSC, TSC_ADJUST) tuple. This value will be written > > to the guest before the first VM-entry" > > > > Sounds like a good idea (to integrate the values in a tuple). > > > > > Few more random notes: > > > > > > I have a weird feeling about using 'nsec since 1 January 1970'. > > > Common sense is telling me that a 64 bit value can hold about 580 years, > > > but still I see that it is more common to use timespec which is a (sec,nsec) pair. > > > > struct timespec { > > time_t tv_sec; /* seconds */ > > long tv_nsec; /* nanoseconds */ > > }; > > > > > I feel that 'kvm_get_walltime' that I added is a bit of a hack. > > > Some refactoring might improve things here. > > > > Haven't read the patchset yet... > > > > > For example making kvm_get_walltime_and_clockread work in non tsc case as well > > > might make the code cleaner. > > > > > > Patches to enable this feature in qemu are in process of being sent to > > > qemu-devel mailing list. > > > > > > Best regards, > > > Maxim Levitsky > > > > > > Maxim Levitsky (2): > > > KVM: x86: implement KVM_SET_TSC_PRECISE/KVM_GET_TSC_PRECISE > > > KVM: x86: introduce KVM_X86_QUIRK_TSC_HOST_ACCESS > > > > > > Documentation/virt/kvm/api.rst | 56 +++++++++++++++++++++ > > > arch/x86/include/uapi/asm/kvm.h | 1 + > > > arch/x86/kvm/x86.c | 88 +++++++++++++++++++++++++++++++-- > > > include/uapi/linux/kvm.h | 14 ++++++ > > > 4 files changed, 154 insertions(+), 5 deletions(-) > > > > > > -- > > > 2.26.2 > > > > > Best regards, > Maxim Levitsky >
