Re: [patch 3/3] x86: kvm guest side support for KVM_HC_RT_PRIO hypercall

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Sep 21, 2017 at 04:06:28PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 21, 2017 at 09:36:53AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Thu, Sep 21, 2017 at 08:38:38AM -0300, Marcelo Tosatti wrote:
> > > Add hypercalls to spinlock/unlock to set/unset FIFO priority
> > > for the vcpu, protected by a static branch to avoid performance
> > > increase in the normal kernels.
> > > 
> > > Enable option by "kvmfifohc" kernel command line parameter (disabled
> > > by default).
> 
> WTF kind of fudge is this? Changelog completely fails to explain the
> problem this would solve. Why are you doing insane things like this?
> 
> 
> NAK!

Copy&pasting from the initial message, please point out whether this
explanation makes sense (better solutions to this problem are welcome):


When executing guest vcpu-0 with FIFO:1 priority, which is necessary
to
deal with the following situation:

VCPU-0 (housekeeping VCPU)              VCPU-1 (realtime VCPU)

raw_spin_lock(A)
interrupted, schedule task T-1          raw_spin_lock(A) (spin)

raw_spin_unlock(A)

Certain operations must interrupt guest vcpu-0 (see trace below).

To fix this issue, only change guest vcpu-0 to FIFO priority
on spinlock critical sections (see patch).

Hang trace
==========

Without FIFO priority:

qemu-kvm-6705  [002] ....1.. 767785.648964: kvm_exit: reason
IO_INSTRUCTION rip 0xe8fe info 1f00039 0
qemu-kvm-6705  [002] ....1.. 767785.648965: kvm_exit: reason
IO_INSTRUCTION rip 0xe911 info 3f60008 0
qemu-kvm-6705  [002] ....1.. 767785.648968: kvm_exit: reason
IO_INSTRUCTION rip 0x8984 info 608000b 0
qemu-kvm-6705  [002] ....1.. 767785.648971: kvm_exit: reason
IO_INSTRUCTION rip 0xb313 info 1f70008 0
qemu-kvm-6705  [002] ....1.. 767785.648974: kvm_exit: reason
IO_INSTRUCTION rip 0xb514 info 3f60000 0
qemu-kvm-6705  [002] ....1.. 767785.648977: kvm_exit: reason
PENDING_INTERRUPT rip 0x8052 info 0 0
qemu-kvm-6705  [002] ....1.. 767785.648980: kvm_exit: reason
IO_INSTRUCTION rip 0xeee6 info 200040 0
qemu-kvm-6705  [002] ....1.. 767785.648999: kvm_exit: reason
EPT_MISCONFIG rip 0x2120 info 0 0

With FIFO priority:

qemu-kvm-7636  [002] ....1.. 768218.205065: kvm_exit: reason
IO_INSTRUCTION rip 0xb313 info 1f70008 0
qemu-kvm-7636  [002] ....1.. 768218.205068: kvm_exit: reason
IO_INSTRUCTION rip 0x8984 info 608000b 0
qemu-kvm-7636  [002] ....1.. 768218.205071: kvm_exit: reason
IO_INSTRUCTION rip 0xb313 info 1f70008 0
qemu-kvm-7636  [002] ....1.. 768218.205074: kvm_exit: reason
IO_INSTRUCTION rip 0x8984 info 608000b 0
qemu-kvm-7636  [002] ....1.. 768218.205077: kvm_exit: reason
IO_INSTRUCTION rip 0xb313 info 1f70008 0
..

Performance numbers (kernel compilation with make -j2)
======================================================

With hypercall: 4:40.  (make -j2)
Without hypercall: 3:38.  (make -j2)

Note for NFV workloads spinlock performance is not relevant
since DPDK should not enter the kernel (and housekeeping vcpu
performance is far from a key factor).


> 
> > > Index: kvm.fifopriohc-submit/include/linux/spinlock_api_smp.h
> > > ===================================================================
> > > --- kvm.fifopriohc-submit.orig/include/linux/spinlock_api_smp.h
> > > +++ kvm.fifopriohc-submit/include/linux/spinlock_api_smp.h
> > > @@ -136,11 +136,28 @@ static inline void __raw_spin_lock_bh(ra
> > >  	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> > >  }
> > >  
> > > +#ifdef CONFIG_KVM_GUEST
> > > +DECLARE_STATIC_KEY_FALSE(kvm_fifo_hc_key);
> > > +#endif
> > > +
> > >  static inline void __raw_spin_lock(raw_spinlock_t *lock)
> > >  {
> > >  	preempt_disable();
> > > +
> > > +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_SMP)
> > > +	/* enable FIFO priority */
> > > +	if (static_branch_unlikely(&kvm_fifo_hc_key))
> > > +		kvm_hypercall1(KVM_HC_RT_PRIO, 0x1);
> > > +#endif
> > > +
> > >  	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> > >  	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> > > +
> > > +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_SMP)
> > > +	/* disable FIFO priority */
> > > +	if (static_branch_unlikely(&kvm_fifo_hc_key))
> > > +		kvm_hypercall1(KVM_HC_RT_PRIO, 0);
> > > +#endif
> > >  }
> > >  
> > >  #endif /* !CONFIG_GENERIC_LOCKBREAK || CONFIG_DEBUG_LOCK_ALLOC */