On Wed, Sep 13, 2023 at 9:06 PM Waiman Long <longman@xxxxxxxxxx> wrote:
>
> On 9/13/23 08:52, Guo Ren wrote:
> > On Wed, Sep 13, 2023 at 4:55 PM Leonardo Bras <leobras@xxxxxxxxxx> wrote:
> >> On Tue, Sep 12, 2023 at 09:10:08AM +0800, Guo Ren wrote:
> >>> 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.
> >> I would suggest adding a snippet from the ISA Extenstion doc:
> >>
> >> "A prefetch.w instruction indicates to hardware that the cache block whose
> >> effective address is the sum of the base address specified in rs1 and the
> >> sign-extended offset encoded in imm[11:0], where imm[4:0] equals 0b00000,
> >> is likely to be accessed by a data write (i.e. store) in the near future."
> > Good point, thx.
>
> qspinlock is generic code. I suppose this is for the RISCV architecture.
> You can mention that in the commit log as an example, but I prefer more
> generic comment especially in the code.
Okay, I would only leave a generic comment on it and move Leonardo's
advice into this patch:
https://lore.kernel.org/linux-riscv/ZQF3qS1KRYAt3coC@xxxxxxxxxx/
>
> Cheers,
> Longman
>
--
Best Regards
Guo Ren
