Re: [v2] powerpc: spinlock: Fix spin_unlock_wait()
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2016-06-06 04:52:15
Also in:
lkml
On Fri, 2016-03-06 at 03:49:48 UTC, Boqun Feng wrote:
There is an ordering issue with spin_unlock_wait() on powerpc, because the spin_lock primitive is an ACQUIRE and an ACQUIRE is only ordering the load part of the operation with memory operations following it.
...
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h index 523673d7583c..2ed893662866 100644 --- a/arch/powerpc/include/asm/spinlock.h +++ b/arch/powerpc/include/asm/spinlock.h@@ -162,12 +181,23 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) lock->slock = 0; } -#ifdef CONFIG_PPC64 -extern void arch_spin_unlock_wait(arch_spinlock_t *lock); -#else -#define arch_spin_unlock_wait(lock) \ - do { while (arch_spin_is_locked(lock)) cpu_relax(); } while (0) -#endif +static inline void arch_spin_unlock_wait(arch_spinlock_t *lock) +{ + smp_mb(); + + if (!arch_spin_is_locked_sync(lock)) + goto out; + + while (!arch_spin_value_unlocked(*lock)) { + HMT_low(); + if (SHARED_PROCESSOR) + __spin_yield(lock); + } + HMT_medium(); + +out: + smp_mb(); +}
I think this would actually be easier to follow if it was all just in one routine:
static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
{
arch_spinlock_t lock_val;
smp_mb();
/*
* Atomically load and store back the lock value (unchanged). This
* ensures that our observation of the lock value is ordered with
* respect to other lock operations.
*/
__asm__ __volatile__(
"1: " PPC_LWARX(%0, 0, %2, 1) "\n"
" stwcx. %0, 0, %2\n"
" bne- 1b\n"
: "=&r" (lock_val), "+m" (*lock)
: "r" (lock)
: "cr0", "xer");
if (arch_spin_value_unlocked(lock_val))
goto out;
while (!arch_spin_value_unlocked(*lock)) {
HMT_low();
if (SHARED_PROCESSOR)
__spin_yield(lock);
}
HMT_medium();
out:
smp_mb();
}
Thoughts?
cheers