Re: [PATCH v4 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Boris Ostrovsky]

On 10/6/19 3:49 AM, Zhenzhong Duan wrote:
> On 2019/10/4 22:52, Boris Ostrovsky wrote:
>
>> On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
>>> Â void __init kvm_spinlock_init(void)
>>> Â {
>>> -ÂÂÂ /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>>> -ÂÂÂ if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>>> -ÂÂÂÂÂÂÂ return;
>>> -
>>> -ÂÂÂ if (kvm_para_has_hint(KVM_HINTS_REALTIME))
>>> +ÂÂÂ /*
>>> +ÂÂÂÂ * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT
>>> feature
>>> +ÂÂÂÂ * support, or there is REALTIME hints or only 1 vCPU.
>>> +ÂÂÂÂ */
>>> +ÂÂÂ if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
>>> +ÂÂÂÂÂÂÂ kvm_para_has_hint(KVM_HINTS_REALTIME) ||
>>> +ÂÂÂÂÂÂÂ num_possible_cpus() == 1) {
>>> +ÂÂÂÂÂÂÂ pr_info("PV spinlocks disabled\n");
>>> ÂÂÂÂÂÂÂÂÂ return;
>>> +ÂÂÂ }
>>> Â -ÂÂÂ /* Don't use the pvqspinlock code if there is only 1 vCPU. */
>>> -ÂÂÂ if (num_possible_cpus() == 1)
>>> +ÂÂÂ if (nopvspin) {
>>> +ÂÂÂÂÂÂÂ pr_info("PV spinlocks disabled forced by \"nopvspin\"
>>> parameter.\n");
>>> +ÂÂÂÂÂÂÂ static_branch_disable(&virt_spin_lock_key);
>> Would it make sense to bring here the other site where the key is
>> disabled (in kvm_smp_prepare_cpus())?
>
> Thanks for point out, I'll do it. Just not clear if I should do that
> in a separate patch,
> there is a history about that code:
>
> Its original place was here and then moved to kvm_smp_prepare_cpus()
> by below commit:
> 34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static
> key get initialized")
> which fixed jump_label_init() calling late issue.
>
> Then 8990cac6 ("x86/jump_label: Initialize static branching early")
> move jump_label_init()
> early, so commit 34226b6b could be reverted.


Which is similar to what you did earlier for Xen.


>
>>
>> (and, in fact, shouldn't all of the checks that result in early return
>> above disable the key?)
>
> I think we should enable he key for
> !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case,
> there is lock holder preemption issue as qspinlock is fair lock,
> virt_spin_lock()
> is an optimization to that, imaging one pcpu running 10 vcpus of same
> guest
> contending a same lock.

Right. I conflated pv lock and virt_spin_lock_key, and that is wrong.

-boris


>
> For kvm_para_has_hint(KVM_HINTS_REALTIME) case, hypervisor hints there is
> no preemption and we should disable virt_spin_lock_key to use native
> qspinlock.
>
> For the UP case, we don't care virt_spin_lock_key value.
>
> For nopvspin case, we intentionally check native qspinlock code
> performance,
> compare it with PV qspinlock, etc. So virt_spin_lock() optimization
> should be disabled.
>
> Let me know if anything wrong with above understanding. Thanks
>
> Zhenzhong
>