On Wed, May 25, 2016 at 09:33:07PM +0300, Roman Kagan wrote:
> On Tue, Apr 26, 2016 at 01:34:56PM +0300, Roman Kagan wrote:
> > On Mon, Apr 25, 2016 at 11:47:23AM +0300, Roman Kagan wrote:
> > > On Fri, Apr 22, 2016 at 08:08:47PM +0200, Paolo Bonzini wrote:
> > > > On 22/04/2016 15:32, Roman Kagan wrote:
> > > > > The first value is derived from the kvm_clock's tsc_to_system_mul and
> > > > > tsc_shift, and matches hosts's vcpu->hw_tsc_khz. The second is
> > > > > calibrated using emulated HPET. The difference is those +14 ppm.
> > > > >
> > > > > This is on i7-2600, invariant TSC present, TSC scaling not present.
> > > > >
> > > > > I'll dig further but I'd appreciate any comment on whether it was within
> > > > > tolerance or not.
> > > >
> > > > The solution to the bug is to change the Hyper-V reference time MSR to
> > > > use the same formula as the Hyper-V TSC-based clock. Likewise,
> > > > KVM_GET_CLOCK and KVM_SET_CLOCK should not use ktime_get_ns().
> > >
> > > Umm, I'm not sure it's a good idea...
> > >
> > > E.g. virtualized HPET sits in userspace and thus uses
> > > clock_gettime(CLOCK_MONOTONIC), so the drift will remain.
> > >
> > > AFAICT the root cause is the following: KVM master clock uses the same
> > > multiplier/shift as the vsyscall time in host userspace. However, the
> > > offsets in vsyscall_gtod_data get updated all the time with corrections
> > > from NTP and so on. Therefore even if the TSC rate is somewhat
> > > miscalibrated, the error is kept small in vsyscall time functions. OTOH
> > > the offsets in KVM clock are basically never updated, so the error keeps
> > > linearly growing over time.
> >
> > This seems to be due to a typo:
Its not a typo, the code only updated the notifier on
VCLOCK_TSC -> !VCLOCK_TSC transition.
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5819,7 +5819,7 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> > /* disable master clock if host does not trust, or does not
> > * use, TSC clocksource
> > */
> > - if (gtod->clock.vclock_mode != VCLOCK_TSC &&
> > + if (gtod->clock.vclock_mode == VCLOCK_TSC &&
> > atomic_read(&kvm_guest_has_master_clock) != 0)
> > queue_work(system_long_wq, &pvclock_gtod_work);
> >
> >
> > as a result, the global pvclock_gtod_data was kept up to date, but the
> > requests to update per-vm copies were never issued.
> >
> > With the patch I'm now seeing different test failures which I'm looking
> > into.
The queue_work is not enough: it opens a window where guest clock
(via shared memory) and CLOCK_GETTIME can go out of sync.
> >
> > Meanwhile I'm wondering if this scheme is not too costly: on my machine
> > pvclock_gtod_notify() is called at kHz rate, and the work it schedules
> > does
> >
> > static void pvclock_gtod_update_fn(struct work_struct *work)
> > {
> > [...]
> > spin_lock(&kvm_lock);
> > list_for_each_entry(kvm, &vm_list, vm_list)
> > kvm_for_each_vcpu(i, vcpu, kvm)
> > kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> > atomic_set(&kvm_guest_has_master_clock, 0);
> > spin_unlock(&kvm_lock);
> > }
> >
> > KVM_REQ_MASTERCLOCK_UPDATE makes all VCPUs synchronize:
> >
> > static void kvm_gen_update_masterclock(struct kvm *kvm)
> > {
> > [...]
> > spin_lock(&ka->pvclock_gtod_sync_lock);
> > kvm_make_mclock_inprogress_request(kvm);
> > /* no guest entries from this point */
> > pvclock_update_vm_gtod_copy(kvm);
> >
> > kvm_for_each_vcpu(i, vcpu, kvm)
> > kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> >
> > /* guest entries allowed */
> > kvm_for_each_vcpu(i, vcpu, kvm)
> > clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
> >
> > spin_unlock(&ka->pvclock_gtod_sync_lock);
> > [...]
> > }
> >
> > so on a host with many VMs it may become an issue.
>
> Ping
>
> Roman.
1) Can call notifier only when frequency changes.
2) Can calculate how much drift between clocks and do not allow guest
entry.
Will post a patch soon, one or two weeks max (again, independent of your patchset).
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
