From: Leonardo Bras <leobras@redhat.com>
To: Guo Ren <guoren@kernel.org>
Cc: Waiman Long <longman@redhat.com>,
paul.walmsley@sifive.com, anup@brainfault.org,
peterz@infradead.org, mingo@redhat.com, will@kernel.org,
palmer@rivosinc.com, boqun.feng@gmail.com, tglx@linutronix.de,
paulmck@kernel.org, rostedt@goodmis.org, rdunlap@infradead.org,
catalin.marinas@arm.com, conor.dooley@microchip.com,
xiaoguang.xing@sophgo.com, bjorn@rivosinc.com,
alexghiti@rivosinc.com, keescook@chromium.org,
greentime.hu@sifive.com, ajones@ventanamicro.com,
jszhang@kernel.org, wefu@redhat.com, wuwei2016@iscas.ac.cn,
linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org,
linux-doc@vger.kernel.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
linux-csky@vger.kernel.org, Guo Ren <guoren@linux.alibaba.com>
Subject: Re: [PATCH V11 04/17] locking/qspinlock: Improve xchg_tail for number of cpus >= 16k
Date: Wed, 13 Sep 2023 05:55:16 -0300 [thread overview]
Message-ID: <ZQF49GIZoFceUGYH@redhat.com> (raw)
In-Reply-To: <CAJF2gTQ3Q7f+FGorVTR66c6TGWsSeeKVvLF+LH1_m3kSHrm0yA@mail.gmail.com>
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@redhat.com> wrote:
> >
> > On 9/10/23 23:09, Guo Ren wrote:
> > > On Mon, Sep 11, 2023 at 10:35 AM Waiman Long <longman@redhat.com> wrote:
> > >>
> > >> On 9/10/23 04:28, guoren@kernel.org wrote:
> > >>> From: Guo Ren <guoren@linux.alibaba.com>
> > >>>
> > >>> 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@linux.alibaba.com>
> > >>> Signed-off-by: Guo Ren <guoren@kernel.org>
> > >>> ---
> > >>> 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."
Other than that,
Reviewed-by: Leonardo Bras <leobras@redhat.com>
>
> >
> > Thanks,
> > Longman
> >
> > >> Cheers,
> > >> Longman
> > >>
> > >
> >
>
>
> --
> Best Regards
> Guo Ren
>
next prev parent reply other threads:[~2023-09-13 8:56 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-10 8:28 [PATCH V11 00/17] riscv: Add Native/Paravirt qspinlock support guoren
2023-09-10 8:28 ` [PATCH V11 01/17] asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock guoren
2023-09-11 19:05 ` Leonardo Brás
2023-09-13 1:55 ` Guo Ren
2023-09-13 7:59 ` Leonardo Bras
2023-09-10 8:28 ` [PATCH V11 02/17] asm-generic: ticket-lock: Move into ticket_spinlock.h guoren
2023-09-13 8:15 ` Leonardo Bras
2023-09-10 8:28 ` [PATCH V11 03/17] riscv: Use Zicbop in arch_xchg when available guoren
2023-09-13 8:49 ` Leonardo Bras
2023-09-15 12:36 ` Guo Ren
2023-09-16 1:25 ` Leonardo Bras
2023-09-17 14:34 ` Guo Ren
2023-09-19 5:13 ` Leonardo Bras
2023-09-19 7:53 ` Guo Ren
2023-09-19 14:38 ` Leonardo Bras
2023-09-14 13:47 ` Andrew Jones
2023-09-15 8:22 ` Leonardo Bras
2023-09-15 11:07 ` Andrew Jones
2023-09-15 11:26 ` Conor Dooley
2023-09-15 12:22 ` Andrew Jones
2023-09-15 12:42 ` Conor Dooley
2023-09-16 0:05 ` Conor Dooley
2023-09-15 20:32 ` Leonardo Bras
2023-09-14 14:25 ` Andrew Jones
2023-09-14 14:47 ` Andrew Jones
2023-09-15 11:37 ` Conor Dooley
2023-09-15 12:14 ` Andrew Jones
2023-09-15 12:53 ` Conor Dooley
2023-12-31 8:29 ` guoren
2023-09-10 8:28 ` [PATCH V11 04/17] locking/qspinlock: Improve xchg_tail for number of cpus >= 16k guoren
2023-09-11 2:35 ` Waiman Long
2023-09-11 3:09 ` Guo Ren
2023-09-11 13:03 ` Waiman Long
2023-09-12 1:10 ` Guo Ren
2023-09-13 8:55 ` Leonardo Bras [this message]
2023-09-13 12:52 ` Guo Ren
2023-09-13 13:06 ` Waiman Long
2023-09-14 3:45 ` Guo Ren
2023-09-10 8:28 ` [PATCH V11 05/17] riscv: qspinlock: Add basic queued_spinlock support guoren
2023-09-13 20:28 ` Leonardo Bras
2023-09-14 4:46 ` Guo Ren
2023-09-14 9:43 ` Leonardo Bras
2023-09-15 2:10 ` Guo Ren
2023-09-15 9:08 ` Leonardo Bras
2023-09-17 15:02 ` Guo Ren
2023-09-19 5:20 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 06/17] riscv: qspinlock: Introduce combo spinlock guoren
2023-09-10 11:06 ` Guo Ren
2023-09-13 20:37 ` Leonardo Bras
2023-09-13 20:49 ` Leonardo Bras
2023-09-14 4:49 ` Guo Ren
2023-09-14 7:17 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 07/17] riscv: qspinlock: Introduce qspinlock param for command line guoren
2023-09-11 15:22 ` Waiman Long
2023-09-12 1:06 ` Guo Ren
2023-09-11 15:34 ` Waiman Long
2023-09-12 1:08 ` Guo Ren
2023-09-14 7:32 ` Leonardo Bras
2023-09-14 17:23 ` Waiman Long
2023-09-10 8:29 ` [PATCH V11 08/17] riscv: qspinlock: Add virt_spin_lock() support for KVM guest guoren
2023-09-14 8:02 ` Leonardo Bras
2023-09-17 15:12 ` Guo Ren
2023-09-19 5:30 ` Leonardo Bras
2023-09-19 8:04 ` Guo Ren
2023-09-19 14:40 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 09/17] riscv: qspinlock: errata: Add ERRATA_THEAD_WRITE_ONCE fixup guoren
2023-09-14 8:32 ` Leonardo Bras
2023-09-17 15:15 ` Guo Ren
2023-09-19 5:34 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 10/17] riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors guoren
2023-09-14 9:36 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 11/17] RISC-V: paravirt: pvqspinlock: Add paravirt qspinlock skeleton guoren
2023-09-15 5:42 ` Leonardo Bras
2023-09-17 14:58 ` Guo Ren
2023-09-19 5:43 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 12/17] RISC-V: paravirt: pvqspinlock: Add nopvspin kernel parameter guoren
2023-09-15 6:05 ` Leonardo Bras
2023-09-17 15:03 ` Guo Ren
2023-09-19 5:44 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 13/17] RISC-V: paravirt: pvqspinlock: Add SBI implementation guoren
2023-09-15 6:23 ` Leonardo Bras
2023-09-17 15:06 ` Guo Ren
2023-09-19 5:45 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 14/17] RISC-V: paravirt: pvqspinlock: Add kconfig entry guoren
2023-09-15 6:25 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 15/17] RISC-V: paravirt: pvqspinlock: Add trace point for pv_kick/wait guoren
2023-09-15 6:33 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 16/17] RISC-V: paravirt: pvqspinlock: KVM: Add paravirt qspinlock skeleton guoren
2023-09-15 6:46 ` Leonardo Bras
2023-09-10 8:29 ` [PATCH V11 17/17] RISC-V: paravirt: pvqspinlock: KVM: Implement kvm_sbi_ext_pvlock_kick_cpu() guoren
2023-09-15 6:52 ` Leonardo Bras
2023-09-10 8:58 ` [PATCH V11 00/17] riscv: Add Native/Paravirt qspinlock support Conor Dooley
2023-09-10 9:16 ` Guo Ren
2023-09-10 9:20 ` Guo Ren
2023-09-10 9:31 ` Conor Dooley
2023-09-10 9:49 ` Guo Ren
2023-09-10 19:45 ` Conor Dooley
2023-09-11 3:36 ` Guo Ren
2023-09-11 12:52 ` Conor Dooley
2023-09-12 1:33 ` Guo Ren
2023-09-12 8:07 ` Conor Dooley
2023-09-12 10:58 ` Guo Ren
2023-11-06 20:42 ` Leonardo Bras
2023-11-12 4:23 ` Guo Ren
2023-11-13 10:19 ` Leonardo Bras Soares Passos
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZQF49GIZoFceUGYH@redhat.com \
--to=leobras@redhat.com \
--cc=ajones@ventanamicro.com \
--cc=alexghiti@rivosinc.com \
--cc=anup@brainfault.org \
--cc=bjorn@rivosinc.com \
--cc=boqun.feng@gmail.com \
--cc=catalin.marinas@arm.com \
--cc=conor.dooley@microchip.com \
--cc=greentime.hu@sifive.com \
--cc=guoren@kernel.org \
--cc=guoren@linux.alibaba.com \
--cc=jszhang@kernel.org \
--cc=keescook@chromium.org \
--cc=kvm@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-csky@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=longman@redhat.com \
--cc=mingo@redhat.com \
--cc=palmer@rivosinc.com \
--cc=paul.walmsley@sifive.com \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rdunlap@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=virtualization@lists.linux-foundation.org \
--cc=wefu@redhat.com \
--cc=will@kernel.org \
--cc=wuwei2016@iscas.ac.cn \
--cc=xiaoguang.xing@sophgo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).