On 2019/10/21 19:14, Vitaly Kuznetsov wrote:
index 249f14a..e9c76d8 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -825,18 +825,44 @@ __visible bool __kvm_vcpu_is_preempted(long cpu) */ void __init kvm_spinlock_init(void) { - /* Does host kernel support KVM_FEATURE_PV_UNHALT? */ - if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) + /* + * PV spinlocks is disabled if no host side support, then native + * qspinlock will be used. As native qspinlock is a fair lock, there is + * lock holder preemption issue using it in a guest, imaging one pCPU + * running 10 vCPUs of same guest contending same lock. + * + * virt_spin_lock() is introduced as an optimization for that scenario + * which is enabled by virt_spin_lock_key key. To use that optimization, + * virt_spin_lock_key isn't disabled here. + */My take (if I properly understood what you say) would be: "In case host doesn't support KVM_FEATURE_PV_UNHALT there is still an advantage of keeping virt_spin_lock_key enabled: virt_spin_lock() is preferred over native qspinlock when vCPU is preempted."
Yes, that's what I mean, maybe I didn't explain clearly due to my pool english,
I'll use your explanation instead.
+ if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) { + pr_info("PV spinlocks disabled, no host support.\n"); return; + }+ /*+ * Disable PV qspinlock and use native qspinlock when dedicated pCPUs + * are available. + */ if (kvm_para_has_hint(KVM_HINTS_REALTIME)) { + pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n"); + static_branch_disable(&virt_spin_lock_key); + return; + } + + if (num_possible_cpus() == 1) { + pr_info("PV spinlocks disabled, single CPU.\n"); static_branch_disable(&virt_spin_lock_key); 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); return;You could've replaced this 'static_branch_disable(); return;' pattern with a goto to the end of the function to save a few lines but this looks good anyways. Reviewed-by: Vitaly Kuznetsov<vkuzn...@redhat.com>
Ok, will do, thanks for review. Zhenzhong
