[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a3c4b603-fc8d-bfa4-7e5d-0b2d8043131b@loongson.cn>
Date: Fri, 2 Aug 2024 09:27:32 +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/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? :)
Regards
Bibo Mao
>
> thanks,
> Chenyu
>
Powered by blists - more mailing lists