Message-ID: <c543a2e1-a4ea-5f81-cc3f-6bd0185c7461@loongson.cn>
Date: Fri, 2 Aug 2024 16:13:48 +0800
From: maobibo <maobibo@...ngson.cn>
To: Chen Yu <yu.c.chen@...el.com>
Cc: Dave Hansen <dave.hansen@...ux.intel.com>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, "H. Peter Anvin" <hpa@...or.com>,
 Arnd Bergmann <arnd@...db.de>, virtualization@...ts.linux.dev,
 linux-kernel@...r.kernel.org, Juergen Gross <jgross@...e.com>,
 Nikolay Borisov <nik.borisov@...e.com>, Qiuxu Zhuo <qiuxu.zhuo@...el.com>,
 Prem Nath Dey <prem.nath.dey@...el.com>,
 Xiaoping Zhou <xiaoping.zhou@...el.com>
Subject: Re: [PATCH v4] x86/paravirt: Disable virt spinlock on bare metal


Hi Chenyu,
On 2024/8/2 下午3:56, Chen Yu wrote:
> On 2024-08-02 at 09:27:32 +0800, maobibo wrote:
>> Hi Chenyu,
>>
>> On 2024/8/1 下午10:40, Chen Yu wrote:
>>> Hi Bibo,
>>>
>>> On 2024-08-01 at 16:00:19 +0800, maobibo wrote:
>>>> Chenyu,
>>>>
>>>> I do not know much about x86, just give some comments(probably incorrected)
>>>> from the code.
>>>>
>>>> On 2024/7/29 下午2:52, Chen Yu wrote:
>>>>> X86_FEATURE_HYPERVISOR         Y    Y    Y     N
>>>>> CONFIG_PARAVIRT_SPINLOCKS      Y    Y    N     Y/N
>>>>> PV spinlock                    Y    N    N     Y/N
>>>>>
>>>>> virt_spin_lock_key             N    N    Y     N
>>>>>
>>>>> -DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key);
>>>>> +DECLARE_STATIC_KEY_FALSE(virt_spin_lock_key);
>>>>
>>>> @@ -87,7 +87,7 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
>>>>    {
>>>>           int val;
>>>>
>>>> -       if (!static_branch_likely(&virt_spin_lock_key))
>>>> +       if (!static_branch_unlikely(&virt_spin_lock_key))
>>>>                   return false;
>>>>
>>>> Do we need change it with static_branch_unlikely() if value of key is false
>>>> by fault?
>>>
>>> My understanding is that, firstly, whether it is likely() or unlikely()
>>> depends on the 'expected' value of the key, rather than its default
>>> initialized value. The compiler can arrange the if 'jump' condition to
>>> avoid the overhead of branch jump(to keep the instruction pipeline)
>>> as much as possible. Secondly, before this patch, the 'expected' value
>>> of virt_spin_lock_key is 'true', so I'm not sure if we should change
>>> its behavior. Although it seems that in most VM guest, with para-virt
>>> spinlock enabled, this key should be false at most time, but just in
>>> case of any regression...
>> yes, it does not inflect the result, it is a trivial thing and depends on
>> personal like or dislike.
>>
>>>
>>>>>     /*
>>>>>      * Shortcut for the queued_spin_lock_slowpath() function that allows
>>>>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
>>>>> index 5358d43886ad..fec381533555 100644
>>>>> --- a/arch/x86/kernel/paravirt.c
>>>>> +++ b/arch/x86/kernel/paravirt.c
>>>>> @@ -51,13 +51,12 @@ DEFINE_ASM_FUNC(pv_native_irq_enable, "sti", .noinstr.text);
>>>>>     DEFINE_ASM_FUNC(pv_native_read_cr2, "mov %cr2, %rax", .noinstr.text);
>>>>>     #endif
>>>>> -DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
>>>>> +DEFINE_STATIC_KEY_FALSE(virt_spin_lock_key);
>>>>>     void __init native_pv_lock_init(void)
>>>>>     {
>>>>> -	if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) &&
>>>>> -	    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>>>> -		static_branch_disable(&virt_spin_lock_key);
>>>>> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>>>> +		static_branch_enable(&virt_spin_lock_key);
>>>>>     }
>>>>
>>>>   From my point, the sentence static_branch_disable(&virt_spin_lock_key) can
>>>> be removed in file arch/x86/xen/spinlock.c and arch/x86/kernel/kvm.c, since
>>>> function native_smp_prepare_boot_cpu() is already called by
>>>> xen_smp_prepare_boot_cpu() and kvm_smp_prepare_boot_cpu().
>>>>
>>>
>>> The key is enabled by native_smp_prepare_boot_cpu() for VM guest as
>>> the initial value(default to true). And later either arch/x86/xen/spinlock.c
>>> or arch/x86/kernel/kvm.c disable this key in a on-demand manner.
>> I understand that you only care about host machine and do not want to change
>> behavior of VM. Only that from the view of VM, there are two conditions such
>> as:
>>
>> 1. If option CONFIG_PARAVIRT_SPINLOCKS is disabled, virt_spin_lock_key is
>> disabled with your patch. VM will use test-set spinlock rather than
>> qspinlock to avoid the over-preemption of native qspinlock, just the same
>> with commit 2aa79af64263. And it is the same for all the hypervisor types.
>>
>> 2. If option CONFIG_PARAVIRT_SPINLOCKS is enable, pv spinlock cannot be used
>> because some reasons, such as host hypervisor has no KVM_FEATURE_PV_UNHALT
>> feature or nopvspin kernel parameter is added. The behavior to use test-set
>> spinlock or native qspinlock is different on different hypervisor types.
>>
>> Even on KVM hypervisor, if KVM_FEATURE_PV_UNHALT is not supported, test-set
>> spinlock will be used on VM; For nopvspin kernel parameter, native spinlock
>> is used on VM. What is the rule for this? :)
>>
> 
> If CONFIG_PARAVIRT_SPINLOCKS is enabled, the test-set spinlock has nothing to do
> with the lock path, because if pv_enabled() is true, it will skip the
IIRC, if CONFIG_PARAVIRT_SPINLOCKS is enabled, there is two qspinlock 
path: native_queued_spin_lock_slowpath() and 
__pv_queued_spin_lock_slowpath(). pv_enabled is false for native 
qspinlock path native_queued_spin_lock_slowpath(), test-set spinlock can 
be used in function native_queued_spin_lock_slowpath(). pv_enabled() is 
true only for function __pv_queued_spin_lock_slowpath().

> test-set spinlock and go to pv_queue section. If for some reason the pv spinlock
> can not be used because KVM_FEATURE_PV_UNHALT is not supported, it will fall into
> the default qpinlock without pv-qspinlock(no pv_wait hook because it is NULL).
yes, if pv spinlock cannot be used, native_queued_spin_lock_slowpath() 
will be called for spin_lock_slowpath, then there will be native 
qspinlock and test-test spinlock.

Regards
Bibo Mao
> 
> thanks,
> Chenyu
> 

Powered by blists - more mailing lists