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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.