Re: [OpenRISC] [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
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
Re: [OpenRISC] [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
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: > > 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
Re: [OpenRISC] [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
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.
Re: [OpenRISC] [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Apr 07, 2021 at 12:51:56AM +0800, Boqun Feng wrote: > Hi, > > On Wed, Mar 31, 2021 at 02:30:32PM +, guo...@kernel.org wrote: > > From: Guo Ren > > > > 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
Re: [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
Hi,
On Wed, Mar 31, 2021 at 02:30:32PM +, guo...@kernel.org wrote:
> From: Guo Ren
>
> 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
> Signed-off-by: Guo Ren
> Cc: Peter Zijlstra
> Cc: Will Deacon
> Cc: Ingo Molnar
> Cc: Waiman Long
> Cc: Arnd Bergmann
> Cc: Anup Patel
> ---
> kernel/Kconfig.locks | 3 +++
> kernel/locking/qspinlock.c | 46 +-
> 2 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
> index 3de8fd11873b..d02f1261f73f 100644
> --- a/kernel/Kconfig.locks
> +++ b/kernel/Kconfig.locks
> @@ -239,6 +239,9 @@ config LOCK_SPIN_ON_OWNER
> config ARCH_USE_QUEUED_SPINLOCKS
> bool
>
> +config ARCH_USE_QUEUED_SPINLOCKS_XCHG32
> + bool
> +
> config QUEUED_SPINLOCKS
> def_bool y if ARCH_USE_QUEUED_SPINLOCKS
> depends on SMP
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index cbff6ba53d56..4bfaa969bd15 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -163,26 +163,6 @@ static __always_inline void
> clear_pending_set_locked(struct qspinlock *lock)
> WRITE_ONCE(lock->locked_pending, _Q_LOCKED_VAL);
> }
>
> -/*
> - * xchg_tail - Put in the new queue tail code word & retrieve previous one
> - * @lock : Pointer to queued spinlock structure
> - * @tail : The new queue tail code word
> - * Return: The previous queue tail code word
> - *
> - * xchg(lock, tail), which heads an address dependency
> - *
> - * p,*,* -> n,*,* ; prev = xchg(lock, node)
> - */
> -static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> -{
> - /*
> - * We can use relaxed semantics since the caller ensures that the
> - * MCS node is properly initialized before updating the tail.
> - */
> - return (u32)xchg_relaxed(>tail,
> - tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
> -}
> -
> #else /* _Q_PENDING_BITS == 8 */
>
> /**
> @@ -206,6 +186,30 @@ static __always_inline void
> clear_pending_set_locked(struct qspinlock *lock)
> {
> atomic_add(-_Q_PENDING_VAL + _Q_LOCKED_VAL, >val);
> }
> +#endif /* _Q_PENDING_BITS == 8 */
> +
> +#if _Q_PENDING_BITS == 8 && !defined(CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32)
> +/*
> + * xchg_tail - Put in the new queue tail code word & retrieve previous one
> + * @lock : Pointer to queued spinlock structure
> + * @tail : The new queue tail code word
> + * Return: The previous queue tail code word
> + *
> + * xchg(lock, tail), which heads an address dependency
> + *
> + * p,*,* -> n,*,* ; prev = xchg(lock, node)
> + */
> +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> +{
> + /*
> + * We can use relaxed semantics since the caller ensures that the
> + * MCS node is properly initialized before updating the tail.
> + */
> + return (u32)xchg_relaxed(>tail,
> + tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
> +}
> +
> +#else
>
> /**
> * xchg_tail - Put in the new queue tail code word & retrieve previous one
> @@ -236,7 +240,7 @@ static __always_inline u32 xchg_tail(struct qspinlock
> *lock, u32 tail)
> }
> return old;
> }
> -#endif /* _Q_PENDING_BITS == 8 */
> +#endif
>
> /**
> * queued_fetch_set_pending_acquire - fetch the whole lock value and set
> pending
> --
> 2.17.1
>
[PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
From: Guo Ren
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
Cc: Peter Zijlstra
Cc: Will Deacon
Cc: Ingo Molnar
Cc: Waiman Long
Cc: Arnd Bergmann
Cc: Anup Patel
---
kernel/Kconfig.locks | 3 +++
kernel/locking/qspinlock.c | 46 +-
2 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 3de8fd11873b..d02f1261f73f 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -239,6 +239,9 @@ config LOCK_SPIN_ON_OWNER
config ARCH_USE_QUEUED_SPINLOCKS
bool
+config ARCH_USE_QUEUED_SPINLOCKS_XCHG32
+ bool
+
config QUEUED_SPINLOCKS
def_bool y if ARCH_USE_QUEUED_SPINLOCKS
depends on SMP
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index cbff6ba53d56..4bfaa969bd15 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -163,26 +163,6 @@ static __always_inline void
clear_pending_set_locked(struct qspinlock *lock)
WRITE_ONCE(lock->locked_pending, _Q_LOCKED_VAL);
}
-/*
- * xchg_tail - Put in the new queue tail code word & retrieve previous one
- * @lock : Pointer to queued spinlock structure
- * @tail : The new queue tail code word
- * Return: The previous queue tail code word
- *
- * xchg(lock, tail), which heads an address dependency
- *
- * p,*,* -> n,*,* ; prev = xchg(lock, node)
- */
-static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
-{
- /*
-* We can use relaxed semantics since the caller ensures that the
-* MCS node is properly initialized before updating the tail.
-*/
- return (u32)xchg_relaxed(>tail,
-tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
-}
-
#else /* _Q_PENDING_BITS == 8 */
/**
@@ -206,6 +186,30 @@ static __always_inline void
clear_pending_set_locked(struct qspinlock *lock)
{
atomic_add(-_Q_PENDING_VAL + _Q_LOCKED_VAL, >val);
}
+#endif /* _Q_PENDING_BITS == 8 */
+
+#if _Q_PENDING_BITS == 8 && !defined(CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32)
+/*
+ * xchg_tail - Put in the new queue tail code word & retrieve previous one
+ * @lock : Pointer to queued spinlock structure
+ * @tail : The new queue tail code word
+ * Return: The previous queue tail code word
+ *
+ * xchg(lock, tail), which heads an address dependency
+ *
+ * p,*,* -> n,*,* ; prev = xchg(lock, node)
+ */
+static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
+{
+ /*
+* We can use relaxed semantics since the caller ensures that the
+* MCS node is properly initialized before updating the tail.
+*/
+ return (u32)xchg_relaxed(>tail,
+tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
+}
+
+#else
/**
* xchg_tail - Put in the new queue tail code word & retrieve previous one
@@ -236,7 +240,7 @@ static __always_inline u32 xchg_tail(struct qspinlock
*lock, u32 tail)
}
return old;
}
-#endif /* _Q_PENDING_BITS == 8 */
+#endif
/**
* queued_fetch_set_pending_acquire - fetch the whole lock value and set
pending
--
2.17.1
