Commit 3032f0c9 authored by Vineet Gupta's avatar Vineet Gupta

ARCv2: spinlock: remove the extra smp_mb before lock, after unlock

 - ARCv2 LLSC spinlocks have smp_mb() both before and after the LLSC
   instructions, which is not required per lkmm ACQ/REL semantics.
   smp_mb() is only needed _after_ lock and _before_ unlock.
   So remove the extra barriers.
   The reason they were there was mainly historical. At the time of
   initial SMP Linux bringup on HS38 cores, I was too conservative,
   given the fluidity of both hw and sw. The last attempt to ditch the
   extra barrier showed some hackbench regression which is apparently
   not the case now (atleast for LLSC case, read on...)

 - EX based spinlocks (!CONFIG_ARC_HAS_LLSC) still needs the extra
   smp_mb(), not due to lkmm, but due to some hardware shenanigans.
   W/o that, hackbench triggers RCU stall splat so extra DMB is retained
   !LLSC based systems are not realistic Linux sstem anyways so they can
   afford to be a nit suboptimal ;-)

   | [ARCLinux]# for i in (seq 1 1 5) ; do hackbench; done
   | Running with 10 groups 400 process
   | INFO: task hackbench:158 blocked for more than 10 seconds.
   |       Not tainted 4.20.0-00005-g96b18288a88e-dirty #117
   | "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
   | hackbench       D    0   158    135 0x00000000
   |
   | Stack Trace:
   | watchdog: BUG: soft lockup - CPU#3 stuck for 59s! [hackbench:469]
   | Modules linked in:
   | Path: (null)
   | CPU: 3 PID: 469 Comm: hackbench Not tainted 4.20.0-00005-g96b18288a88e-dirty
   |
   | [ECR   ]: 0x00000000 => Check Programmer's Manual
   | [EFA   ]: 0x00000000
   | [BLINK ]: do_exit+0x4a6/0x7d0
   | [ERET  ]: _raw_write_unlock_irq+0x44/0x5c

 - And while at it, remove the extar smp_mb() from EX based
   arch_read_trylock() since the spin lock there guarantees a full
   barrier anyways

 - For LLSC case, hackbench threads improves with this patch (HAPS @ 50MHz)

   ---- before ----
   |
   | [ARCLinux]# for i in 1 2 3 4 5; do hackbench 10 thread; done
   | Running with 10 groups 400 threads
   | Time: 16.253
   | Time: 16.445
   | Time: 16.590
   | Time: 16.721
   | Time: 16.544

   ---- after ----
   |
   | [ARCLinux]# for i in 1 2 3 4 5; do hackbench 10 thread; done
   | Running with 10 groups 400 threads
   | Time: 15.638
   | Time: 15.730
   | Time: 15.870
   | Time: 15.842
   | Time: 15.729
Acked-by: default avatarPeter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: default avatarVineet Gupta <vgupta@synopsys.com>
parent 6dd356d8
...@@ -21,8 +21,6 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) ...@@ -21,8 +21,6 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
{ {
unsigned int val; unsigned int val;
smp_mb();
__asm__ __volatile__( __asm__ __volatile__(
"1: llock %[val], [%[slock]] \n" "1: llock %[val], [%[slock]] \n"
" breq %[val], %[LOCKED], 1b \n" /* spin while LOCKED */ " breq %[val], %[LOCKED], 1b \n" /* spin while LOCKED */
...@@ -34,6 +32,14 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) ...@@ -34,6 +32,14 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
[LOCKED] "r" (__ARCH_SPIN_LOCK_LOCKED__) [LOCKED] "r" (__ARCH_SPIN_LOCK_LOCKED__)
: "memory", "cc"); : "memory", "cc");
/*
* ACQUIRE barrier to ensure load/store after taking the lock
* don't "bleed-up" out of the critical section (leak-in is allowed)
* http://www.spinics.net/lists/kernel/msg2010409.html
*
* ARCv2 only has load-load, store-store and all-all barrier
* thus need the full all-all barrier
*/
smp_mb(); smp_mb();
} }
...@@ -42,8 +48,6 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock) ...@@ -42,8 +48,6 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
{ {
unsigned int val, got_it = 0; unsigned int val, got_it = 0;
smp_mb();
__asm__ __volatile__( __asm__ __volatile__(
"1: llock %[val], [%[slock]] \n" "1: llock %[val], [%[slock]] \n"
" breq %[val], %[LOCKED], 4f \n" /* already LOCKED, just bail */ " breq %[val], %[LOCKED], 4f \n" /* already LOCKED, just bail */
...@@ -67,9 +71,7 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) ...@@ -67,9 +71,7 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
{ {
smp_mb(); smp_mb();
lock->slock = __ARCH_SPIN_LOCK_UNLOCKED__; WRITE_ONCE(lock->slock, __ARCH_SPIN_LOCK_UNLOCKED__);
smp_mb();
} }
/* /*
...@@ -81,8 +83,6 @@ static inline void arch_read_lock(arch_rwlock_t *rw) ...@@ -81,8 +83,6 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
{ {
unsigned int val; unsigned int val;
smp_mb();
/* /*
* zero means writer holds the lock exclusively, deny Reader. * zero means writer holds the lock exclusively, deny Reader.
* Otherwise grant lock to first/subseq reader * Otherwise grant lock to first/subseq reader
...@@ -113,8 +113,6 @@ static inline int arch_read_trylock(arch_rwlock_t *rw) ...@@ -113,8 +113,6 @@ static inline int arch_read_trylock(arch_rwlock_t *rw)
{ {
unsigned int val, got_it = 0; unsigned int val, got_it = 0;
smp_mb();
__asm__ __volatile__( __asm__ __volatile__(
"1: llock %[val], [%[rwlock]] \n" "1: llock %[val], [%[rwlock]] \n"
" brls %[val], %[WR_LOCKED], 4f\n" /* <= 0: already write locked, bail */ " brls %[val], %[WR_LOCKED], 4f\n" /* <= 0: already write locked, bail */
...@@ -140,8 +138,6 @@ static inline void arch_write_lock(arch_rwlock_t *rw) ...@@ -140,8 +138,6 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
{ {
unsigned int val; unsigned int val;
smp_mb();
/* /*
* If reader(s) hold lock (lock < __ARCH_RW_LOCK_UNLOCKED__), * If reader(s) hold lock (lock < __ARCH_RW_LOCK_UNLOCKED__),
* deny writer. Otherwise if unlocked grant to writer * deny writer. Otherwise if unlocked grant to writer
...@@ -175,8 +171,6 @@ static inline int arch_write_trylock(arch_rwlock_t *rw) ...@@ -175,8 +171,6 @@ static inline int arch_write_trylock(arch_rwlock_t *rw)
{ {
unsigned int val, got_it = 0; unsigned int val, got_it = 0;
smp_mb();
__asm__ __volatile__( __asm__ __volatile__(
"1: llock %[val], [%[rwlock]] \n" "1: llock %[val], [%[rwlock]] \n"
" brne %[val], %[UNLOCKED], 4f \n" /* !UNLOCKED, bail */ " brne %[val], %[UNLOCKED], 4f \n" /* !UNLOCKED, bail */
...@@ -217,17 +211,13 @@ static inline void arch_read_unlock(arch_rwlock_t *rw) ...@@ -217,17 +211,13 @@ static inline void arch_read_unlock(arch_rwlock_t *rw)
: [val] "=&r" (val) : [val] "=&r" (val)
: [rwlock] "r" (&(rw->counter)) : [rwlock] "r" (&(rw->counter))
: "memory", "cc"); : "memory", "cc");
smp_mb();
} }
static inline void arch_write_unlock(arch_rwlock_t *rw) static inline void arch_write_unlock(arch_rwlock_t *rw)
{ {
smp_mb(); smp_mb();
rw->counter = __ARCH_RW_LOCK_UNLOCKED__; WRITE_ONCE(rw->counter, __ARCH_RW_LOCK_UNLOCKED__);
smp_mb();
} }
#else /* !CONFIG_ARC_HAS_LLSC */ #else /* !CONFIG_ARC_HAS_LLSC */
...@@ -237,10 +227,9 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) ...@@ -237,10 +227,9 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
unsigned int val = __ARCH_SPIN_LOCK_LOCKED__; unsigned int val = __ARCH_SPIN_LOCK_LOCKED__;
/* /*
* This smp_mb() is technically superfluous, we only need the one * Per lkmm, smp_mb() is only required after _lock (and before_unlock)
* after the lock for providing the ACQUIRE semantics. * for ACQ and REL semantics respectively. However EX based spinlocks
* However doing the "right" thing was regressing hackbench * need the extra smp_mb to workaround a hardware quirk.
* so keeping this, pending further investigation
*/ */
smp_mb(); smp_mb();
...@@ -257,14 +246,6 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) ...@@ -257,14 +246,6 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
#endif #endif
: "memory"); : "memory");
/*
* ACQUIRE barrier to ensure load/store after taking the lock
* don't "bleed-up" out of the critical section (leak-in is allowed)
* http://www.spinics.net/lists/kernel/msg2010409.html
*
* ARCv2 only has load-load, store-store and all-all barrier
* thus need the full all-all barrier
*/
smp_mb(); smp_mb();
} }
...@@ -309,8 +290,7 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) ...@@ -309,8 +290,7 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
: "memory"); : "memory");
/* /*
* superfluous, but keeping for now - see pairing version in * see pairing version/comment in arch_spin_lock above
* arch_spin_lock above
*/ */
smp_mb(); smp_mb();
} }
...@@ -344,7 +324,6 @@ static inline int arch_read_trylock(arch_rwlock_t *rw) ...@@ -344,7 +324,6 @@ static inline int arch_read_trylock(arch_rwlock_t *rw)
arch_spin_unlock(&(rw->lock_mutex)); arch_spin_unlock(&(rw->lock_mutex));
local_irq_restore(flags); local_irq_restore(flags);
smp_mb();
return ret; return ret;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment