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 >