Re: [patch 2/3] KVM: x86: add option to advance tscdeadline hrtimer expiration

Marcelo Tosatti
2015-01-08T17:41:50Z

On Mon, Jan 05, 2015 at 07:12:36PM +0100, Radim Krcmar wrote:
> 2014-12-23 15:58-0500, Marcelo Tosatti:
> > For the hrtimer which emulates the tscdeadline timer in the guest,
> > add an option to advance expiration, and busy spin on VM-entry waiting
> > for the actual expiration time to elapse.
> >
> > This allows achieving low latencies in cyclictest (or any scenario
> > which requires strict timing regarding timer expiration).
> >
> > Reduces average cyclictest latency from 12us to 8us
> > on Core i5 desktop.
> >
> > Note: this option requires tuning to find the appropriate value
> > for a particular hardware/guest combination. One method is to measure the
> > average delay between apic_timer_fn and VM-entry.
> > Another method is to start with 1000ns, and increase the value
> > in say 500ns increments until avg cyclictest numbers stop decreasing.
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
>
> (Other patches weren't touched, so my previous Reviewed-by holds.)
>
> > +++ kvm/arch/x86/kvm/x86.c
> > @@ -108,6 +108,10 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz)
> > static u32 tsc_tolerance_ppm = 250;
> > module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
> >
> > +/* lapic timer advance (tscdeadline mode only) in nanoseconds */
> > +unsigned int lapic_timer_advance_ns = 0;
> > +module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
> > +
> > static bool backwards_tsc_observed = false;
> >
> > #define KVM_NR_SHARED_MSRS 16
> > @@ -5625,6 +5629,10 @@ static void kvm_timer_init(void)
> > __register_hotcpu_notifier(&kvmclock_cpu_notifier_block);
> > cpu_notifier_register_done();
> >
> > + if (check_tsc_unstable() && lapic_timer_advance_ns) {
> > + pr_info("kvm: unstable TSC, disabling lapic_timer_advance_ns\n");
> > + lapic_timer_advance_ns = 0;
>
> Does unstable TSC invalidate this feature?
> (lapic_timer_advance_ns can be overridden, so we don't differentiate
> workflows that calibrate after starting with 0.)

Hum, i don't see why. Paolo?

> And cover letter is a bit misleading: The condition does nothing to
> guarantee TSC based __delay() loop. (Right now, __delay() = delay_tsc()
> whenever the hardware has TSC, regardless of stability, thus always.)

OK.