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
