On Tue, Oct 22, 2019 at 08:46:46PM +0800, Zhenzhong Duan wrote:
> Hi Vitaly,
>
> On 2019/10/22 19:36, Vitaly Kuznetsov wrote:
>
> >Zhenzhong Duan<zhenzhong.d...@oracle.com> writes:
> >
> ...snip
>
> >>diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> >>index 249f14a..3945aa5 100644
> >>--- a/arch/x86/kernel/kvm.c
> >>+++ b/arch/x86/kernel/kvm.c
> >>@@ -825,18 +825,36 @@ __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))
> >>+ /*
> >>+ * 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.
> >>+ */
> >>+ 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)) {
> >>- static_branch_disable(&virt_spin_lock_key);
> >>- return;
> >>+ pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME
> >>hints.\n");
> >>+ goto out;
> >> }
> >>- /* Don't use the pvqspinlock code if there is only 1 vCPU. */
> >>- if (num_possible_cpus() == 1)
> >>- return;
> >>+ if (num_possible_cpus() == 1) {
> >>+ pr_info("PV spinlocks disabled, single CPU.\n");
> >>+ goto out;
> >>+ }
> >>+
> >>+ if (nopvspin) {
> >>+ pr_info("PV spinlocks disabled, forced by \"nopvspin\"
> >>parameter.\n");
> >>+ goto out;
> >>+ }
> >>+
> >>+ pr_info("PV spinlocks enabled\n");
> >> __pv_init_lock_hash();
> >> pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
> >>@@ -849,6 +867,8 @@ void __init kvm_spinlock_init(void)
> >> pv_ops.lock.vcpu_is_preempted =
> >> PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
> >> }
> >>+out:
> >>+ static_branch_disable(&virt_spin_lock_key);
> >You probably need to add 'return' before 'out:' as it seems you're
> >disabling virt_spin_lock_key in all cases now).
>
> virt_spin_lock_key is kept enabled in
> !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)
> case which is the only case virt_spin_lock() optimization is used.
>
> When PV qspinlock is enabled, virt_spin_lock() isn't called in
> __pv_queued_spin_lock_slowpath() in which case we don't care
> virt_spin_lock_key's value.
>
> So adding 'return' or not are both ok, I chosed to save a line,
> let me know if you prefer to add a 'return' and I'll change it.
It'd be worth adding a comment here if you end up spinning another version
to change the logging prefix. The logic is sound and I like the end
result, but I had the same knee jerk "this can't be right!?!?" reaction as
Vitaly.
