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
