On Thu, May 26, 2016 at 10:19:36PM +0200, Radim Krčmář wrote: > 2016-05-26 17:49+0300, Roman Kagan: > > The condition to schedule per-VM master clock updates is inversed; as a > > result the master clocks are never updated and the kvm_clock drift WRT > > host's NTP-disciplined clock (which was the motivation to introduce this > > machinery in the first place) still remains. > > > > Fix it, and reword the comment to make it more apparent what the desired > > behavior is. > > > > Cc: Owen Hofmann <osh@xxxxxxxxxx> > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx> > > Signed-off-by: Roman Kagan <rkagan@xxxxxxxxxxxxx> > > --- > > arch/x86/kvm/x86.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index c805cf4..d8f591c 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -5810,10 +5810,10 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused, > > > > update_pvclock_gtod(tk); > > > > - /* disable master clock if host does not trust, or does not > > - * use, TSC clocksource > > + /* only schedule per-VM master clock updates if the host uses TSC and > > + * there's at least one VM in need of an update > > */ > > - if (gtod->clock.vclock_mode != VCLOCK_TSC && > > + if (gtod->clock.vclock_mode == VCLOCK_TSC && > > I think we still want to disable master clock when vclock_mode is not > VCLOCK_TSC. Hmm, indeed. I missed that this thing is used both to update the contents and to enable/disable it. I'd say it's confusing... > > > atomic_read(&kvm_guest_has_master_clock) != 0) > > And I don't see why we don't want to enable master clock if the host > switches back to TSC. Agreed (even though I guess it's not very likely: AFAICS once switched to a different clocksource, the host can switch back to TSC only upon human manipulating /sys/devices/system/clocksource). > > queue_work(system_long_wq, &pvclock_gtod_work); > > Queueing unconditionally seems to be the correct thing to do. The notifier is registered at kvm module init, so the work will be scheduled even when there are no VMs at all. > Interaction between kvm_gen_update_masterclock(), pvclock_gtod_work(), > and NTP could be a problem: kvm_gen_update_masterclock() only has to > run once per VM, but pvclock_gtod_work() calls it on every VCPU, so > frequent NTP updates on bigger guests could kill performance. Unfortunately, things are worse than that: this stuff is updated on every *tick* on the timekeeping CPU, so, as long as you keep at least one of your CPUs busy, the update rate can reach HZ. The frequency of NTP updates is unimportant; it happens without NTP updates at all. So I tend to agree that we're perhaps better off not fixing this bug and leaving the kvm_clocks to drift until we figure out how to do it with acceptable overhead. Roman. -- 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
