From: Guo Ren <redacted>
Current riscv is still using baby spinlock implementation. It'll cause
fairness and cache line bouncing problems. Many people are involved
and pay the efforts to improve it:
- The first version of patch was made in 2019.1:
https://lore.kernel.org/linux-riscv/20190211043829.30096-1-michaeljclark@mac.com/#r
- The second version was made in 2020.11:
https://lore.kernel.org/linux-riscv/1606225437-22948-2-git-send-email-guoren@kernel.org/
- A good discussion at Platform HSC.2021-03-08:
https://drive.google.com/drive/folders/1ooqdnIsYx7XKor5O1XTtM6D1CHp4hc0p
- A good discussion on V4 in mailling list:
https://lore.kernel.org/linux-riscv/1616868399-82848-1-git-send-email-guoren@kernel.org/T/#t
- Openrisc's maintainer want to implement arch_cmpxchg infrastructure.
https://lore.kernel.org/linux-riscv/1616868399-82848-1-git-send-email-guoren@kernel.org/T/#m11b712fb6a4fda043811b1f4c3d61446951ed65a
Hope your comments and Tested-by or Co-developed-by or Reviewed-by ...
Let's kick the qspinlock into riscv right now (Also for the
architecture which hasn't xchg16 atomic instruction.)
Change V6:
- Add ticket-lock for riscv, default is qspinlock
- Keep ticket-lock for csky, default is ticketlock
- Using smp_cond_load for riscv ticket-lock
- Optimize csky ticketlock with smp_cond_load, store_release
- Add PPC_LBARX_LWARX for powerpc
Change V5:
- Fixup #endif comment typo by Waiman
- Remove cmpxchg coding convention patches which will get into a
separate patchset later by Arnd's advice
- Try to involve more architectures in the discussion
Change V4:
- Remove custom sub-word xchg implementation
- Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32 in locking/qspinlock
Change V3:
- Coding convention by Peter Zijlstra's advices
Change V2:
- Coding convention in cmpxchg.h
- Re-implement short xchg
- Remove char & cmpxchg implementations
Guo Ren (8):
locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
riscv: locks: Introduce ticket-based spinlock implementation
csky: locks: Optimize coding convention
csky: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
openrisc: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
sparc: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
xtensa: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
powerpc/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
Michael Clark (1):
riscv: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
arch/csky/Kconfig | 8 ++
arch/csky/include/asm/Kbuild | 2 +
arch/csky/include/asm/spinlock.h | 15 +--
arch/csky/include/asm/spinlock_types.h | 4 +
arch/openrisc/Kconfig | 1 +
arch/powerpc/Kconfig | 1 +
arch/riscv/Kconfig | 8 ++
arch/riscv/include/asm/Kbuild | 3 +
arch/riscv/include/asm/spinlock.h | 158 +++++++++---------------
arch/riscv/include/asm/spinlock_types.h | 26 ++--
arch/sparc/Kconfig | 1 +
arch/xtensa/Kconfig | 1 +
kernel/Kconfig.locks | 3 +
kernel/locking/qspinlock.c | 46 +++----
14 files changed, 142 insertions(+), 135 deletions(-)
--
2.17.1
From: Guo Ren <redacted>
Some architectures don't have sub-word swap atomic instruction,
they only have the full word's one.
The sub-word swap only improve the performance when:
NR_CPUS < 16K
* 0- 7: locked byte
* 8: pending
* 9-15: not used
* 16-17: tail index
* 18-31: tail cpu (+1)
The 9-15 bits are wasted to use xchg16 in xchg_tail.
Please let architecture select xchg16/xchg32 to implement
xchg_tail.
Signed-off-by: Guo Ren <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Anup Patel <anup@brainfault.org>
---
kernel/Kconfig.locks | 3 +++
kernel/locking/qspinlock.c | 46 +++++++++++++++++++++-----------------
2 files changed, 28 insertions(+), 21 deletions(-)
From: Michael Clark <redacted>
Update the RISC-V port to use the generic qspinlock and qrwlock.
This patch requires support for xchg_xtail for full-word which
are added by a previous patch:
Guo added select ARCH_USE_QUEUED_SPINLOCKS_XCHG32 in Kconfig
Guo fixed up compile error which made by below include sequence:
+#include <asm/qrwlock.h>
+#include <asm/qspinlock.h>
Signed-off-by: Michael Clark <redacted>
Co-developed-by: Guo Ren <redacted>
Tested-by: Guo Ren <redacted>
Signed-off-by: Guo Ren <redacted>
Link: https://lore.kernel.org/linux-riscv/20190211043829.30096-3-michaeljclark@mac.com/
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Palmer Dabbelt <redacted>
---
arch/riscv/Kconfig | 3 +
arch/riscv/include/asm/Kbuild | 3 +
arch/riscv/include/asm/spinlock.h | 126 +-----------------------
arch/riscv/include/asm/spinlock_types.h | 15 +--
4 files changed, 11 insertions(+), 136 deletions(-)
From: Guo Ren <redacted>
- Using smp_cond_load_acquire in arch_spin_lock by Peter's
advice.
- Using __smp_acquire_fence in arch_spin_trylock
- Using smp_store_release in arch_spin_unlock
All above are just coding conventions and won't affect the
function.
TODO in smp_cond_load_acquire for architecture:
- current csky only has:
lr.w val, <p0>
sc.w <p0>. val2
(Any other stores to p0 will let sc.w failed)
- But smp_cond_load_acquire need:
lr.w val, <p0>
wfe
(Any stores to p0 will send the event to let wfe retired)
Signed-off-by: Guo Ren <redacted>
Link: https://lore.kernel.org/linux-riscv/CAAhSdy1JHLUFwu7RuCaQ+RUWRBks2KsDva7EpRt8--4ZfofSUQ@mail.gmail.com/T/#m13adac285b7f51f4f879a5d6b65753ecb1a7524e
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/csky/include/asm/spinlock.h | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
From: Guo Ren <redacted>
This patch introduces a ticket lock implementation for riscv, along the
same lines as the implementation for arch/arm & arch/csky.
We still use qspinlock as default.
Signed-off-by: Guo Ren <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/riscv/Kconfig | 7 ++-
arch/riscv/include/asm/spinlock.h | 84 +++++++++++++++++++++++++
arch/riscv/include/asm/spinlock_types.h | 17 +++++
3 files changed, 107 insertions(+), 1 deletion(-)
From: Guo Ren <redacted>
We don't have native hw xchg16 instruction, so let qspinlock
generic code to deal with it.
Using the full-word atomic xchg instructions implement xchg16 has
the semantic risk for atomic operations.
This patch cancels the dependency of on qspinlock generic code on
architecture's xchg16.
Signed-off-by: Guo Ren <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Rob Gardner <redacted>
---
arch/sparc/Kconfig | 1 +
1 file changed, 1 insertion(+)
From: Guo Ren <redacted>
Update the C-SKY port to use the generic qspinlock and qrwlock.
C-SKY only support ldex.w/stex.w with word(double word) size &
align access. So it must select XCHG32 to let qspinlock only use
word atomic xchg_tail.
Default is still ticket lock.
Signed-off-by: Guo Ren <redacted>
Cc: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/csky/Kconfig | 8 ++++++++
arch/csky/include/asm/Kbuild | 2 ++
arch/csky/include/asm/spinlock.h | 4 ++++
arch/csky/include/asm/spinlock_types.h | 4 ++++
4 files changed, 18 insertions(+)
From: Guo Ren <redacted>
We don't have native hw xchg16 instruction, so let qspinlock
generic code to deal with it.
Using the full-word atomic xchg instructions implement xchg16 has
the semantic risk for atomic operations.
This patch cancels the dependency of on qspinlock generic code on
architecture's xchg16.
Signed-off-by: Guo Ren <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Cc: Stafford Horne <shorne@gmail.com>
Cc: openrisc@lists.librecores.org
---
arch/openrisc/Kconfig | 1 +
1 file changed, 1 insertion(+)
From: Guo Ren <redacted>
We don't have native hw xchg16 instruction, so let qspinlock
generic code to deal with it.
Using the full-word atomic xchg instructions implement xchg16 has
the semantic risk for atomic operations.
This patch cancels the dependency of on qspinlock generic code on
architecture's xchg16.
Signed-off-by: Guo Ren <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Chris Zankel <chris@zankel.net>
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/Kconfig | 1 +
1 file changed, 1 insertion(+)
From: Guo Ren <redacted>
We don't have native hw xchg16 instruction, so let qspinlock
generic code to deal with it.
Using the full-word atomic xchg instructions implement xchg16 has
the semantic risk for atomic operations.
This patch cancels the dependency of on qspinlock generic code on
architecture's xchg16.
Also no need when PPC_LBARX_LWARX is enabled, see the link below.
Signed-off-by: Guo Ren <redacted>
Link: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20201107032328.2454582-1-npiggin@gmail.com/
Cc: Christophe Leroy <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
---
arch/powerpc/Kconfig | 1 +
1 file changed, 1 insertion(+)
On Wed, Mar 31, 2021 at 10:32 PM [off-list ref] wrote:
quoted hunk
From: Guo Ren <redacted>
This patch introduces a ticket lock implementation for riscv, along the
same lines as the implementation for arch/arm & arch/csky.
We still use qspinlock as default.
Signed-off-by: Guo Ren <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/riscv/Kconfig | 7 ++-
arch/riscv/include/asm/spinlock.h | 84 +++++++++++++++++++++++++
arch/riscv/include/asm/spinlock_types.h | 17 +++++
3 files changed, 107 insertions(+), 1 deletion(-)
On Wed, Mar 31, 2021 at 02:30:37PM +0000, guoren@kernel.org wrote:
From: Guo Ren <redacted>
We don't have native hw xchg16 instruction, so let qspinlock
generic code to deal with it.
Using the full-word atomic xchg instructions implement xchg16 has
the semantic risk for atomic operations.
This patch cancels the dependency of on qspinlock generic code on
architecture's xchg16.
Signed-off-by: Guo Ren <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Cc: Stafford Horne <shorne@gmail.com>
Cc: openrisc@lists.librecores.org
Hi,
On Wed, Mar 31, 2021 at 02:30:32PM +0000, guoren@kernel.org wrote:
From: Guo Ren <redacted>
Some architectures don't have sub-word swap atomic instruction,
they only have the full word's one.
The sub-word swap only improve the performance when:
NR_CPUS < 16K
* 0- 7: locked byte
* 8: pending
* 9-15: not used
* 16-17: tail index
* 18-31: tail cpu (+1)
The 9-15 bits are wasted to use xchg16 in xchg_tail.
Please let architecture select xchg16/xchg32 to implement
xchg_tail.
If the architecture doesn't have sub-word swap atomic, won't it generate
the same/similar code no matter which version xchg_tail() is used? That
is even CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, xchg_tail() acts
similar to an xchg16() implemented by cmpxchg(), which means we still
don't have forward progress guarantee. So this configuration doesn't
solve the problem.
I think it's OK to introduce this config and don't provide xchg16() for
risc-v. But I don't see the point of converting other architectures to
use it.
Regards,
Boqun
On Wed, Apr 07, 2021 at 12:51:56AM +0800, Boqun Feng wrote:
Hi,
On Wed, Mar 31, 2021 at 02:30:32PM +0000, guoren@kernel.org wrote:
quoted
From: Guo Ren <redacted>
Some architectures don't have sub-word swap atomic instruction,
they only have the full word's one.
The sub-word swap only improve the performance when:
NR_CPUS < 16K
* 0- 7: locked byte
* 8: pending
* 9-15: not used
* 16-17: tail index
* 18-31: tail cpu (+1)
The 9-15 bits are wasted to use xchg16 in xchg_tail.
Please let architecture select xchg16/xchg32 to implement
xchg_tail.
If the architecture doesn't have sub-word swap atomic, won't it generate
the same/similar code no matter which version xchg_tail() is used? That
is even CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, xchg_tail() acts
similar to an xchg16() implemented by cmpxchg(), which means we still
don't have forward progress guarantee. So this configuration doesn't
solve the problem.
I think it's OK to introduce this config and don't provide xchg16() for
risc-v. But I don't see the point of converting other architectures to
use it.
Hello,
For OpenRISC I did ack the patch to convert to
CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y. But I think you are right, the
generic code in xchg_tail and the xchg16 emulation code in produced by OpenRISC
using xchg32 would produce very similar code. I have not compared instructions,
but it does seem like duplicate functionality.
Why doesn't RISC-V add the xchg16 emulation code similar to OpenRISC? For
OpenRISC we added xchg16 and xchg8 emulation code to enable qspinlocks. So
one thought is with CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, can we remove our
xchg16/xchg8 emulation code?
-Stafford
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-04-07 09:48:38
On Wed, Apr 07, 2021 at 08:52:08AM +0900, Stafford Horne wrote:
Why doesn't RISC-V add the xchg16 emulation code similar to OpenRISC? For
OpenRISC we added xchg16 and xchg8 emulation code to enable qspinlocks. So
one thought is with CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, can we remove our
xchg16/xchg8 emulation code?
CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32 is guaranteed crap.
All the architectures that have wanted it are RISC style LL/SC archs,
and for them a cmpxchg loop is a daft thing to do, since it reduces the
chance of it behaving sanely.
Why would we provide something that's known to be suboptimal? If an
architecture chooses to not care about determinism and or fwd progress,
then that's their choice. But not one, I feel, we should encourage.
On Wed, Apr 07, 2021 at 11:47:49AM +0200, Peter Zijlstra wrote:
On Wed, Apr 07, 2021 at 08:52:08AM +0900, Stafford Horne wrote:
quoted
Why doesn't RISC-V add the xchg16 emulation code similar to OpenRISC? For
OpenRISC we added xchg16 and xchg8 emulation code to enable qspinlocks. So
one thought is with CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, can we remove our
xchg16/xchg8 emulation code?
CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32 is guaranteed crap.
All the architectures that have wanted it are RISC style LL/SC archs,
and for them a cmpxchg loop is a daft thing to do, since it reduces the
chance of it behaving sanely.
Why would we provide something that's known to be suboptimal? If an
architecture chooses to not care about determinism and or fwd progress,
then that's their choice. But not one, I feel, we should encourage.
Thanks, this is the response I was hoping my comment would provoke.
So not enabling CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32 for architectures
unless they really want it should be the way.
-Stafford
From: Waiman Long <longman@redhat.com> Date: 2021-04-08 19:01:30
On 4/6/21 7:52 PM, Stafford Horne wrote:
For OpenRISC I did ack the patch to convert to
CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y. But I think you are right, the
generic code in xchg_tail and the xchg16 emulation code in produced by OpenRISC
using xchg32 would produce very similar code. I have not compared instructions,
but it does seem like duplicate functionality.
Why doesn't RISC-V add the xchg16 emulation code similar to OpenRISC? For
OpenRISC we added xchg16 and xchg8 emulation code to enable qspinlocks. So
one thought is with CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32=y, can we remove our
xchg16/xchg8 emulation code?
For the record, the latest qspinlock code doesn't use xchg8 anymore. It
still need xchg16, though.
Cheers,
Longman
On Wed, Mar 31, 2021 at 10:32 PM [off-list ref] wrote:
quoted hunk
From: Guo Ren <redacted>
- Using smp_cond_load_acquire in arch_spin_lock by Peter's
advice.
- Using __smp_acquire_fence in arch_spin_trylock
- Using smp_store_release in arch_spin_unlock
All above are just coding conventions and won't affect the
function.
TODO in smp_cond_load_acquire for architecture:
- current csky only has:
lr.w val, <p0>
sc.w <p0>. val2
(Any other stores to p0 will let sc.w failed)
- But smp_cond_load_acquire need:
lr.w val, <p0>
wfe
(Any stores to p0 will send the event to let wfe retired)
Signed-off-by: Guo Ren <redacted>
Link: https://lore.kernel.org/linux-riscv/CAAhSdy1JHLUFwu7RuCaQ+RUWRBks2KsDva7EpRt8--4ZfofSUQ@mail.gmail.com/T/#m13adac285b7f51f4f879a5d6b65753ecb1a7524e
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/csky/include/asm/spinlock.h | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
On Wed, Mar 31, 2021 at 10:32 PM [off-list ref] wrote:
quoted hunk
From: Guo Ren <redacted>
This patch introduces a ticket lock implementation for riscv, along the
same lines as the implementation for arch/arm & arch/csky.
We still use qspinlock as default.
Signed-off-by: Guo Ren <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/riscv/Kconfig | 7 ++-
arch/riscv/include/asm/spinlock.h | 84 +++++++++++++++++++++++++
arch/riscv/include/asm/spinlock_types.h | 17 +++++
3 files changed, 107 insertions(+), 1 deletion(-)
On Mon, Apr 12, 2021 at 12:02 AM Guo Ren [off-list ref] wrote:
quoted hunk
On Wed, Mar 31, 2021 at 10:32 PM [off-list ref] wrote:
quoted
From: Guo Ren <redacted>
This patch introduces a ticket lock implementation for riscv, along the
same lines as the implementation for arch/arm & arch/csky.
We still use qspinlock as default.
Signed-off-by: Guo Ren <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/riscv/Kconfig | 7 ++-
arch/riscv/include/asm/spinlock.h | 84 +++++++++++++++++++++++++
arch/riscv/include/asm/spinlock_types.h | 17 +++++
3 files changed, 107 insertions(+), 1 deletion(-)
eh... plus __smp_acquire_fence:
if (lockval.owner != lockval.tickets.next)
smp_cond_load_acquire(&lock->tickets.owner,
VAL == lockval.tickets.next);
else
__smp_acquire_fence();