Message ID | 20220728063120.2867508-2-npiggin@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc: alternate queued spinlock implementation | expand |
On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: <snip> > -#define queued_spin_lock queued_spin_lock > > -static inline void queued_spin_unlock(struct qspinlock *lock) > +static __always_inline int queued_spin_trylock(struct qspinlock *lock) > { > - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) > - smp_store_release(&lock->locked, 0); > - else > - __pv_queued_spin_unlock(lock); > + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) > + return 1; > + return 0; optional style nit: return (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0);
Le 10/08/2022 à 03:52, Jordan NIethe a écrit : > On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: > <snip> >> -#define queued_spin_lock queued_spin_lock >> >> -static inline void queued_spin_unlock(struct qspinlock *lock) >> +static __always_inline int queued_spin_trylock(struct qspinlock *lock) >> { >> - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) >> - smp_store_release(&lock->locked, 0); >> - else >> - __pv_queued_spin_unlock(lock); >> + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) >> + return 1; >> + return 0; > > optional style nit: return (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0); > The parenthesis are pointless, and ! is usually prefered to == 0, something like that: return !atomic_cmpxchg_acquire(&lock->val, 0, 1);
On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: <snip> > -#define queued_spin_lock queued_spin_lock > > -static inline void queued_spin_unlock(struct qspinlock *lock) > +static __always_inline int queued_spin_trylock(struct qspinlock *lock) > { > - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) > - smp_store_release(&lock->locked, 0); > - else > - __pv_queued_spin_unlock(lock); > + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) > + return 1; > + return 0; optional style nit: return (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0); [resend as utf-8, not utf-7]
Le 10/11/2022 à 01:35, Jordan Niethe a écrit : > On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: > <snip> >> -#define queued_spin_lock queued_spin_lock >> >> -static inline void queued_spin_unlock(struct qspinlock *lock) >> +static __always_inline int queued_spin_trylock(struct qspinlock *lock) >> { >> - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) >> - smp_store_release(&lock->locked, 0); >> - else >> - __pv_queued_spin_unlock(lock); >> + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) >> + return 1; >> + return 0; > > optional style nit: return (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0); No parenthesis. No == 0 Should be : return !atomic_cmpxchg_acquire(&lock->val, 0, 1); > > [resend as utf-8, not utf-7] >
On Thu Nov 10, 2022 at 10:35 AM AEST, Jordan Niethe wrote: > On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: > <snip> > > -#define queued_spin_lock queued_spin_lock > > > > -static inline void queued_spin_unlock(struct qspinlock *lock) > > +static __always_inline int queued_spin_trylock(struct qspinlock *lock) > > { > > - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) > > - smp_store_release(&lock->locked, 0); > > - else > > - __pv_queued_spin_unlock(lock); > > + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) > > + return 1; > > + return 0; > > optional style nit: return (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0); > > [resend as utf-8, not utf-7] Thanks for the thorough review, apologies again it took me so long to get back to. I'm not completely sold on this. I guess it's already side-effects in a control flow statement though... Maybe I will change it, not sure. Thanks, Nick
On Thu Nov 10, 2022 at 4:37 PM AEST, Christophe Leroy wrote: > > > Le 10/11/2022 à 01:35, Jordan Niethe a écrit : > > On Thu, 2022-07-28 at 16:31 +1000, Nicholas Piggin wrote: > > <snip> > >> -#define queued_spin_lock queued_spin_lock > >> > >> -static inline void queued_spin_unlock(struct qspinlock *lock) > >> +static __always_inline int queued_spin_trylock(struct qspinlock *lock) > >> { > >> - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) > >> - smp_store_release(&lock->locked, 0); > >> - else > >> - __pv_queued_spin_unlock(lock); > >> + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) > >> + return 1; > >> + return 0; > > > > optional style nit: return (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0); > > No parenthesis. > No == 0 > > Should be : > > return !atomic_cmpxchg_acquire(&lock->val, 0, 1); In this case I prefer the == 0 because we're testing against the 0 old parameter being passed in. This is the recognisable cmpxchg pattern. The other style of cmpxchg returns true if it succeeded, so it's less clear we're not using that version if using !. Thanks, Nick
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 7aa12e88c580..4838e6c96b20 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -154,7 +154,6 @@ config PPC select ARCH_USE_CMPXCHG_LOCKREF if PPC64 select ARCH_USE_MEMTEST select ARCH_USE_QUEUED_RWLOCKS if PPC_QUEUED_SPINLOCKS - select ARCH_USE_QUEUED_SPINLOCKS if PPC_QUEUED_SPINLOCKS select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select ARCH_WANT_IPC_PARSE_VERSION select ARCH_WANT_IRQS_OFF_ACTIVATE_MM diff --git a/arch/powerpc/include/asm/qspinlock.h b/arch/powerpc/include/asm/qspinlock.h index 39c1c7f80579..cb2b4f91e976 100644 --- a/arch/powerpc/include/asm/qspinlock.h +++ b/arch/powerpc/include/asm/qspinlock.h @@ -2,66 +2,56 @@ #ifndef _ASM_POWERPC_QSPINLOCK_H #define _ASM_POWERPC_QSPINLOCK_H -#include <asm-generic/qspinlock_types.h> -#include <asm/paravirt.h> +#include <linux/atomic.h> +#include <linux/compiler.h> +#include <asm/qspinlock_types.h> -#define _Q_PENDING_LOOPS (1 << 9) /* not tuned */ - -void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); -void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); -void __pv_queued_spin_unlock(struct qspinlock *lock); - -static __always_inline void queued_spin_lock(struct qspinlock *lock) +static __always_inline int queued_spin_is_locked(struct qspinlock *lock) { - u32 val = 0; + return atomic_read(&lock->val); +} - if (likely(arch_atomic_try_cmpxchg_lock(&lock->val, &val, _Q_LOCKED_VAL))) - return; +static __always_inline int queued_spin_value_unlocked(struct qspinlock lock) +{ + return !atomic_read(&lock.val); +} - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) - queued_spin_lock_slowpath(lock, val); - else - __pv_queued_spin_lock_slowpath(lock, val); +static __always_inline int queued_spin_is_contended(struct qspinlock *lock) +{ + return 0; } -#define queued_spin_lock queued_spin_lock -static inline void queued_spin_unlock(struct qspinlock *lock) +static __always_inline int queued_spin_trylock(struct qspinlock *lock) { - if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) || !is_shared_processor()) - smp_store_release(&lock->locked, 0); - else - __pv_queued_spin_unlock(lock); + if (atomic_cmpxchg_acquire(&lock->val, 0, 1) == 0) + return 1; + return 0; } -#define queued_spin_unlock queued_spin_unlock -#ifdef CONFIG_PARAVIRT_SPINLOCKS -#define SPIN_THRESHOLD (1<<15) /* not tuned */ +void queued_spin_lock_slowpath(struct qspinlock *lock); -static __always_inline void pv_wait(u8 *ptr, u8 val) +static __always_inline void queued_spin_lock(struct qspinlock *lock) { - if (*ptr != val) - return; - yield_to_any(); - /* - * We could pass in a CPU here if waiting in the queue and yield to - * the previous CPU in the queue. - */ + if (!queued_spin_trylock(lock)) + queued_spin_lock_slowpath(lock); } -static __always_inline void pv_kick(int cpu) +static inline void queued_spin_unlock(struct qspinlock *lock) { - prod_cpu(cpu); + atomic_set_release(&lock->val, 0); } -#endif +#define arch_spin_is_locked(l) queued_spin_is_locked(l) +#define arch_spin_is_contended(l) queued_spin_is_contended(l) +#define arch_spin_value_unlocked(l) queued_spin_value_unlocked(l) +#define arch_spin_lock(l) queued_spin_lock(l) +#define arch_spin_trylock(l) queued_spin_trylock(l) +#define arch_spin_unlock(l) queued_spin_unlock(l) -/* - * Queued spinlocks rely heavily on smp_cond_load_relaxed() to busy-wait, - * which was found to have performance problems if implemented with - * the preferred spin_begin()/spin_end() SMT priority pattern. Use the - * generic version instead. - */ - -#include <asm-generic/qspinlock.h> +#ifdef CONFIG_PARAVIRT_SPINLOCKS +void pv_spinlocks_init(void); +#else +static inline void pv_spinlocks_init(void) { } +#endif #endif /* _ASM_POWERPC_QSPINLOCK_H */ diff --git a/arch/powerpc/include/asm/qspinlock_types.h b/arch/powerpc/include/asm/qspinlock_types.h new file mode 100644 index 000000000000..59606bc0c774 --- /dev/null +++ b/arch/powerpc/include/asm/qspinlock_types.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef _ASM_POWERPC_QSPINLOCK_TYPES_H +#define _ASM_POWERPC_QSPINLOCK_TYPES_H + +#include <linux/types.h> + +typedef struct qspinlock { + atomic_t val; +} arch_spinlock_t; + +#define __ARCH_SPIN_LOCK_UNLOCKED { .val = ATOMIC_INIT(0) } + +#endif /* _ASM_POWERPC_QSPINLOCK_TYPES_H */ diff --git a/arch/powerpc/include/asm/spinlock_types.h b/arch/powerpc/include/asm/spinlock_types.h index d5f8a74ed2e8..40b01446cf75 100644 --- a/arch/powerpc/include/asm/spinlock_types.h +++ b/arch/powerpc/include/asm/spinlock_types.h @@ -7,7 +7,7 @@ #endif #ifdef CONFIG_PPC_QUEUED_SPINLOCKS -#include <asm-generic/qspinlock_types.h> +#include <asm/qspinlock_types.h> #include <asm-generic/qrwlock_types.h> #else #include <asm/simple_spinlock_types.h> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile index 8560c912186d..b895cbf6a709 100644 --- a/arch/powerpc/lib/Makefile +++ b/arch/powerpc/lib/Makefile @@ -52,7 +52,9 @@ obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \ obj64-y += copypage_64.o copyuser_64.o mem_64.o hweight_64.o \ memcpy_64.o copy_mc_64.o -ifndef CONFIG_PPC_QUEUED_SPINLOCKS +ifdef CONFIG_PPC_QUEUED_SPINLOCKS +obj64-$(CONFIG_SMP) += qspinlock.o +else obj64-$(CONFIG_SMP) += locks.o endif diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c new file mode 100644 index 000000000000..8dbce99a373c --- /dev/null +++ b/arch/powerpc/lib/qspinlock.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include <linux/export.h> +#include <linux/processor.h> +#include <asm/qspinlock.h> + +void queued_spin_lock_slowpath(struct qspinlock *lock) +{ + while (!queued_spin_trylock(lock)) + cpu_relax(); +} +EXPORT_SYMBOL(queued_spin_lock_slowpath); + +#ifdef CONFIG_PARAVIRT_SPINLOCKS +void pv_spinlocks_init(void) +{ +} +#endif +