On Mon, Sep 11, 2023 at 9:03 PM Waiman Long <longman@xxxxxxxxxx> wrote: > > On 9/10/23 23:09, Guo Ren wrote: > > On Mon, Sep 11, 2023 at 10:35 AM Waiman Long <longman@xxxxxxxxxx> wrote: > >> > >> On 9/10/23 04:28, guoren@xxxxxxxxxx wrote: > >>> From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > >>> > >>> The target of xchg_tail is to write the tail to the lock value, so > >>> adding prefetchw could help the next cmpxchg step, which may > >>> decrease the cmpxchg retry loops of xchg_tail. Some processors may > >>> utilize this feature to give a forward guarantee, e.g., RISC-V > >>> XuanTie processors would block the snoop channel & irq for several > >>> cycles when prefetch.w instruction (from Zicbop extension) retired, > >>> which guarantees the next cmpxchg succeeds. > >>> > >>> Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > >>> Signed-off-by: Guo Ren <guoren@xxxxxxxxxx> > >>> --- > >>> kernel/locking/qspinlock.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c > >>> index d3f99060b60f..96b54e2ade86 100644 > >>> --- a/kernel/locking/qspinlock.c > >>> +++ b/kernel/locking/qspinlock.c > >>> @@ -223,7 +223,10 @@ static __always_inline void clear_pending_set_locked(struct qspinlock *lock) > >>> */ > >>> static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail) > >>> { > >>> - u32 old, new, val = atomic_read(&lock->val); > >>> + u32 old, new, val; > >>> + > >>> + prefetchw(&lock->val); > >>> + val = atomic_read(&lock->val); > >>> > >>> for (;;) { > >>> new = (val & _Q_LOCKED_PENDING_MASK) | tail; > >> That looks a bit weird. You pre-fetch and then immediately read it. How > >> much performance gain you get by this change alone? > >> > >> Maybe you can define an arch specific primitive that default back to > >> atomic_read() if not defined. > > Thx for the reply. This is a generic optimization point I would like > > to talk about with you. > > > > First, prefetchw() makes cacheline an exclusive state and serves for > > the next cmpxchg loop semantic, which writes the idx_tail part of > > arch_spin_lock. The atomic_read only makes cacheline in the shared > > state, which couldn't give any guarantee for the next cmpxchg loop > > semantic. Micro-architecture could utilize prefetchw() to provide a > > strong forward progress guarantee for the xchg_tail, e.g., the T-HEAD > > XuanTie processor would hold the exclusive cacheline state until the > > next cmpxchg write success. > > > > In the end, Let's go back to the principle: the xchg_tail is an atomic > > swap operation that contains write eventually, so giving a prefetchw() > > at the beginning is acceptable for all architectures.. > > •••••••••••• > > I did realize afterward that prefetchw gets the cacheline in exclusive > state. I will suggest you mention that in your commit log as well as > adding a comment about its purpose in the code. Okay, I would do that in v12, thx. > > Thanks, > Longman > > >> Cheers, > >> Longman > >> > > > -- Best Regards Guo Ren