Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v6 3/5] x86/kvm: Add "nopvspin" parameter to disable PV
 spinlocks
To:     Vitaly Kuznetsov <vkuznets@redhat.com>,
        linux-kernel@vger.kernel.org
Cc:     linux-hyperv@vger.kernel.org, kvm@vger.kernel.org,
        kys@microsoft.com, haiyangz@microsoft.com, sthemmin@microsoft.com,
        sashal@kernel.org, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de, pbonzini@redhat.com, rkrcmar@redhat.com,
        sean.j.christopherson@intel.com, wanpengli@tencent.com,
        jmattson@google.com, joro@8bytes.org, boris.ostrovsky@oracle.com,
        jgross@suse.com, sstabellini@kernel.org, peterz@infradead.org,
        Jonathan Corbet <corbet@lwn.net>,
        "H. Peter Anvin" <hpa@zytor.com>, Will Deacon <will@kernel.org>
References: <1571102367-31595-1-git-send-email-zhenzhong.duan@oracle.com>
 <1571102367-31595-4-git-send-email-zhenzhong.duan@oracle.com>
 <87k18y1hc1.fsf@vitty.brq.redhat.com>
From:   Zhenzhong Duan <zhenzhong.duan@oracle.com>
Organization: Oracle Corporation
Message-ID: <a1a5b381-cd06-04ed-5d05-6cb7bfa070b8@oracle.com>
Date:   Tue, 22 Oct 2019 10:45:25 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <87k18y1hc1.fsf@vitty.brq.redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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