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 */
