On 2024-08-02 at 16:13:48 +0800, maobibo wrote:
>
> 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().
>
Thanks for this explaination in detail!
> > 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.
>
If I understand correctly, your concern about current logic is that, when
CONFIG_PARAVIRT_SPINLOCKS is set but unfortunately disabled at runtime, there is
inconsistence between using native qspinlock and test-set. My guess is that,
nopvspin is for user who wants non-paravirt and native qspin, no matter it is xen
or the kvm, all the other cases prefer test-set lock, no?
thanks,
Chenyu
