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

[Zhenzhong Duan <zhenzhong.duan@oracle.com>]

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@redhat.com>

Ok, will do, thanks for review.

Zhenzhong