On 10/09/2017 09:39 AM, Paolo Bonzini wrote: > On 06/10/2017 23:52, Eduardo Habkost wrote: >> This series enables kvm_pv_unhalt by default on pc-*-2.11 and >> newer. >> >> To do that, I first reworked the existing >> x86_cpu_change_kvm_default() logic to use compat_props instead, >> so we don't need to make the chain of pc_compat_*() functions >> grow. > I've discussed PV spinlocks with some folks at Microsoft for a few weeks > now, and I'm not 100% sure about enabling kvm_pv_unhalt by default. > It's probably a good idea overall, but it does come with some caveats. > > The problem is that there were two different implementations of fair > spinlocks in Linux, ticket spinlocks and queued spinlocks. When > kvm_pv_unhalt is disabled, ticket spinlocks sucked badly indeed; queued > spinlocks however simply revert to unfair spinlocks, which loses the > fairness but has the best performance. See virt_spin_lock in > arch/x86/include/asm/qspinlock.h. > > Now, fair spinlocks are only really needed for large NUMA machines. > With a single NUMA node, for example, test-and-set spinlocks work well > enough; there's not _much_ need for fairness in practice, and the > paravirtualization does introduce some overhead. > > Therefore, the best performance would be achieved with kvm_pv_unhalt > disabled on small VMs, and enabled on large VMs spanning multiple host > NUMA nodes. > > Waiman, Davidlohr, do you have an opinion on this as well?
I agree. Using unfair lock in a small VM is good for performance. The only problem I see is how do we define what is small. Now, even a machine with a single socket can have up to 28 cores, 56 threads. If a VM has up to 56 vCPUs, we may still want pvqspinlock to be used. Is the kvm_pv_unhalt flag a global one for all VMs within a machine? Or can it be different for each VM? We may want to have this flag dynamically determined depending on the configuration of the VM. Regards, Longman
