Re: [PATCH v6 3/5] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

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<vkuznets@xxxxxxxxxx>

Ok, will do, thanks for review.

Zhenzhong