Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681942 > unrolled thread

[PATCH v2 0/9] Remove spin_unlock_wait()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 01:40 +0200
    [PATCH v2 9/9] arch: Remove spin_unlock_wait() arch-specific definitions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 01:40 +0200
    RE: [PATCH v2 0/9] Remove spin_unlock_wait() David Laight <David.Laight@ACULAB.COM> - 2017-07-06 16:20 +0200
      Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 17:30 +0200
        Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 18:20 +0200
          Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 18:30 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 18:50 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 19:10 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Alan Stern <stern@rowland.harvard.edu> - 2017-07-06 18:50 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 19:00 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Alan Stern <stern@rowland.harvard.edu> - 2017-07-06 21:40 +0200
      Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 18:10 +0200
        Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 18:30 +0200
          Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 19:00 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Will Deacon <will.deacon@arm.com> - 2017-07-06 19:10 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 19:30 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 19:20 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 10:40 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-07 10:50 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 12:40 +0200
                  Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-07 13:30 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 16:50 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-08 10:50 +0200
                  Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-08 13:50 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() Manfred Spraul <manfred@colorfullife.com> - 2017-07-07 19:50 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-08 10:40 +0200
                  Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-08 13:50 +0200
                    Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-08 14:40 +0200
                      Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-08 16:50 +0200
                      Re: [PATCH v2 0/9] Remove spin_unlock_wait() Alan Stern <stern@rowland.harvard.edu> - 2017-07-08 18:30 +0200
                        Re: [PATCH v2 0/9] Remove spin_unlock_wait() Manfred Spraul <manfred@colorfullife.com> - 2017-07-10 19:30 +0200
        Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 10:10 +0200
          Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 11:40 +0200
    [PATCH v3 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:30 +0200
      [PATCH v3 9/9] arch: Remove spin_unlock_wait() arch-specific definitions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:40 +0200
      [PATCH v3 5/9] exit: Replace spin_unlock_wait() with lock/unlock pair "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:40 +0200
      [PATCH v3 8/9] locking: Remove spin_unlock_wait() generic definitions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:40 +0200

Page 1 of 2  [1] 2  Next page →

#1681942 — [PATCH v2 0/9] Remove spin_unlock_wait()

Hello!

There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair.  This series therefore removes spin_unlock_wait() and changes
its users to instead use a lock/unlock pair.  The commits are as follows,
in three groups:

1-7.	Change uses of spin_unlock_wait() and raw_spin_unlock_wait()
	to instead use a spin_lock/spin_unlock pair.  These may be
	applied in any order, but must be applied before any later
	commits in this series.  The commit logs state why I believe
	that these commits won't noticeably degrade performance.

8.	Remove core-kernel definitions for spin_unlock_wait() and
	raw_spin_unlock_wait().

9.	Remove arch-specific definitions of arch_spin_unlock_wait().

Changes since v1:

o	Disable interrupts where needed, thus avoiding embarrassing
	interrupt-based self-deadlocks.

o	Substitute Manfred's patch for contrack_lock (#1 above).

o	Substitute Oleg's patch for task_work (#2 above).

o	Use more efficient barrier based on Arnd Bergmann feedback.

							Thanx, Paul

------------------------------------------------------------------------

 arch/alpha/include/asm/spinlock.h    |    5 -
 arch/arc/include/asm/spinlock.h      |    5 -
 arch/arm/include/asm/spinlock.h      |   16 ----
 arch/arm64/include/asm/spinlock.h    |   58 +----------------
 arch/blackfin/include/asm/spinlock.h |    5 -
 arch/hexagon/include/asm/spinlock.h  |    5 -
 arch/ia64/include/asm/spinlock.h     |   21 ------
 arch/m32r/include/asm/spinlock.h     |    5 -
 arch/metag/include/asm/spinlock.h    |    5 -
 arch/mips/include/asm/spinlock.h     |   16 ----
 arch/mn10300/include/asm/spinlock.h  |    5 -
 arch/parisc/include/asm/spinlock.h   |    7 --
 arch/powerpc/include/asm/spinlock.h  |   33 ---------
 arch/s390/include/asm/spinlock.h     |    7 --
 arch/sh/include/asm/spinlock-cas.h   |    5 -
 arch/sh/include/asm/spinlock-llsc.h  |    5 -
 arch/sparc/include/asm/spinlock_32.h |    5 -
 arch/sparc/include/asm/spinlock_64.h |    5 -
 arch/tile/include/asm/spinlock_32.h  |    2 
 arch/tile/include/asm/spinlock_64.h  |    2 
 arch/tile/lib/spinlock_32.c          |   23 ------
 arch/tile/lib/spinlock_64.c          |   22 ------
 arch/xtensa/include/asm/spinlock.h   |    5 -
 drivers/ata/libata-eh.c              |    8 --
 include/asm-generic/qspinlock.h      |   14 ----
 include/linux/spinlock.h             |   31 ---------
 include/linux/spinlock_up.h          |    6 -
 ipc/sem.c                            |    3 
 kernel/exit.c                        |    3 
 kernel/locking/qspinlock.c           |  117 -----------------------------------
 kernel/sched/completion.c            |    9 --
 kernel/sched/core.c                  |    5 -
 kernel/task_work.c                   |    8 --
 net/netfilter/nf_conntrack_core.c    |   44 +++++++------
 34 files changed, 43 insertions(+), 472 deletions(-)

[toc] | [next] | [standalone]

#1681944 — [PATCH v2 9/9] arch: Remove spin_unlock_wait() arch-specific definitions

There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair.  This commit therefore removes the underlying arch-specific
arch_spin_unlock_wait() for all architectures providing them.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: <linux-arch@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrea Parri <parri.andrea@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
---
 arch/alpha/include/asm/spinlock.h    |  5 ----
 arch/arc/include/asm/spinlock.h      |  5 ----
 arch/arm/include/asm/spinlock.h      | 16 ----------
 arch/arm64/include/asm/spinlock.h    | 58 ++++--------------------------------
 arch/blackfin/include/asm/spinlock.h |  5 ----
 arch/hexagon/include/asm/spinlock.h  |  5 ----
 arch/ia64/include/asm/spinlock.h     | 21 -------------
 arch/m32r/include/asm/spinlock.h     |  5 ----
 arch/metag/include/asm/spinlock.h    |  5 ----
 arch/mips/include/asm/spinlock.h     | 16 ----------
 arch/mn10300/include/asm/spinlock.h  |  5 ----
 arch/parisc/include/asm/spinlock.h   |  7 -----
 arch/powerpc/include/asm/spinlock.h  | 33 --------------------
 arch/s390/include/asm/spinlock.h     |  7 -----
 arch/sh/include/asm/spinlock-cas.h   |  5 ----
 arch/sh/include/asm/spinlock-llsc.h  |  5 ----
 arch/sparc/include/asm/spinlock_32.h |  5 ----
 arch/sparc/include/asm/spinlock_64.h |  5 ----
 arch/tile/include/asm/spinlock_32.h  |  2 --
 arch/tile/include/asm/spinlock_64.h  |  2 --
 arch/tile/lib/spinlock_32.c          | 23 --------------
 arch/tile/lib/spinlock_64.c          | 22 --------------
 arch/xtensa/include/asm/spinlock.h   |  5 ----
 23 files changed, 5 insertions(+), 262 deletions(-)

diff --git a/arch/alpha/include/asm/spinlock.h b/arch/alpha/include/asm/spinlock.h
index a40b9fc0c6c3..718ac0b64adf 100644
--- a/arch/alpha/include/asm/spinlock.h
+++ b/arch/alpha/include/asm/spinlock.h
@@ -16,11 +16,6 @@
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 #define arch_spin_is_locked(x)	((x)->lock != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 {
         return lock.lock == 0;
diff --git a/arch/arc/include/asm/spinlock.h b/arch/arc/include/asm/spinlock.h
index 233d5ffe6ec7..a325e6a36523 100644
--- a/arch/arc/include/asm/spinlock.h
+++ b/arch/arc/include/asm/spinlock.h
@@ -16,11 +16,6 @@
 #define arch_spin_is_locked(x)	((x)->slock != __ARCH_SPIN_LOCK_UNLOCKED__)
 #define arch_spin_lock_flags(lock, flags)	arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, !VAL);
-}
-
 #ifdef CONFIG_ARC_HAS_LLSC
 
 static inline void arch_spin_lock(arch_spinlock_t *lock)
diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
index 4bec45442072..c030143c18c6 100644
--- a/arch/arm/include/asm/spinlock.h
+++ b/arch/arm/include/asm/spinlock.h
@@ -52,22 +52,6 @@ static inline void dsb_sev(void)
  * memory.
  */
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u16 owner = READ_ONCE(lock->tickets.owner);
-
-	for (;;) {
-		arch_spinlock_t tmp = READ_ONCE(*lock);
-
-		if (tmp.tickets.owner == tmp.tickets.next ||
-		    tmp.tickets.owner != owner)
-			break;
-
-		wfe();
-	}
-	smp_acquire__after_ctrl_dep();
-}
-
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
 static inline void arch_spin_lock(arch_spinlock_t *lock)
diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index cae331d553f8..f445bd7f2b9f 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -26,58 +26,6 @@
  * The memory barriers are implicit with the load-acquire and store-release
  * instructions.
  */
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	unsigned int tmp;
-	arch_spinlock_t lockval;
-	u32 owner;
-
-	/*
-	 * Ensure prior spin_lock operations to other locks have completed
-	 * on this CPU before we test whether "lock" is locked.
-	 */
-	smp_mb();
-	owner = READ_ONCE(lock->owner) << 16;
-
-	asm volatile(
-"	sevl\n"
-"1:	wfe\n"
-"2:	ldaxr	%w0, %2\n"
-	/* Is the lock free? */
-"	eor	%w1, %w0, %w0, ror #16\n"
-"	cbz	%w1, 3f\n"
-	/* Lock taken -- has there been a subsequent unlock->lock transition? */
-"	eor	%w1, %w3, %w0, lsl #16\n"
-"	cbz	%w1, 1b\n"
-	/*
-	 * The owner has been updated, so there was an unlock->lock
-	 * transition that we missed. That means we can rely on the
-	 * store-release of the unlock operation paired with the
-	 * load-acquire of the lock operation to publish any of our
-	 * previous stores to the new lock owner and therefore don't
-	 * need to bother with the writeback below.
-	 */
-"	b	4f\n"
-"3:\n"
-	/*
-	 * Serialise against any concurrent lockers by writing back the
-	 * unlocked lock value
-	 */
-	ARM64_LSE_ATOMIC_INSN(
-	/* LL/SC */
-"	stxr	%w1, %w0, %2\n"
-	__nops(2),
-	/* LSE atomics */
-"	mov	%w1, %w0\n"
-"	cas	%w0, %w0, %2\n"
-"	eor	%w1, %w1, %w0\n")
-	/* Somebody else wrote to the lock, GOTO 10 and reload the value */
-"	cbnz	%w1, 2b\n"
-"4:"
-	: "=&r" (lockval), "=&r" (tmp), "+Q" (*lock)
-	: "r" (owner)
-	: "memory");
-}
 
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
@@ -176,7 +124,11 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 
 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-	smp_mb(); /* See arch_spin_unlock_wait */
+	/*
+	 * Ensure prior spin_lock operations to other locks have completed
+	 * on this CPU before we test whether "lock" is locked.
+	 */
+	smp_mb(); /* ^^^ */
 	return !arch_spin_value_unlocked(READ_ONCE(*lock));
 }
 
diff --git a/arch/blackfin/include/asm/spinlock.h b/arch/blackfin/include/asm/spinlock.h
index c58f4a83ed6f..f6431439d15d 100644
--- a/arch/blackfin/include/asm/spinlock.h
+++ b/arch/blackfin/include/asm/spinlock.h
@@ -48,11 +48,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	__raw_spin_unlock_asm(&lock->lock);
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline int arch_read_can_lock(arch_rwlock_t *rw)
 {
 	return __raw_uncached_fetch_asm(&rw->lock) > 0;
diff --git a/arch/hexagon/include/asm/spinlock.h b/arch/hexagon/include/asm/spinlock.h
index a1c55788c5d6..53a8d5885887 100644
--- a/arch/hexagon/include/asm/spinlock.h
+++ b/arch/hexagon/include/asm/spinlock.h
@@ -179,11 +179,6 @@ static inline unsigned int arch_spin_trylock(arch_spinlock_t *lock)
  */
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 #define arch_spin_is_locked(x) ((x)->lock != 0)
 
 #define arch_read_lock_flags(lock, flags) arch_read_lock(lock)
diff --git a/arch/ia64/include/asm/spinlock.h b/arch/ia64/include/asm/spinlock.h
index ca9e76149a4a..df2c121164b8 100644
--- a/arch/ia64/include/asm/spinlock.h
+++ b/arch/ia64/include/asm/spinlock.h
@@ -76,22 +76,6 @@ static __always_inline void __ticket_spin_unlock(arch_spinlock_t *lock)
 	ACCESS_ONCE(*p) = (tmp + 2) & ~1;
 }
 
-static __always_inline void __ticket_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	int	*p = (int *)&lock->lock, ticket;
-
-	ia64_invala();
-
-	for (;;) {
-		asm volatile ("ld4.c.nc %0=[%1]" : "=r"(ticket) : "r"(p) : "memory");
-		if (!(((ticket >> TICKET_SHIFT) ^ ticket) & TICKET_MASK))
-			return;
-		cpu_relax();
-	}
-
-	smp_acquire__after_ctrl_dep();
-}
-
 static inline int __ticket_spin_is_locked(arch_spinlock_t *lock)
 {
 	long tmp = ACCESS_ONCE(lock->lock);
@@ -143,11 +127,6 @@ static __always_inline void arch_spin_lock_flags(arch_spinlock_t *lock,
 	arch_spin_lock(lock);
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	__ticket_spin_unlock_wait(lock);
-}
-
 #define arch_read_can_lock(rw)		(*(volatile int *)(rw) >= 0)
 #define arch_write_can_lock(rw)	(*(volatile int *)(rw) == 0)
 
diff --git a/arch/m32r/include/asm/spinlock.h b/arch/m32r/include/asm/spinlock.h
index 323c7fc953cd..a56825592b90 100644
--- a/arch/m32r/include/asm/spinlock.h
+++ b/arch/m32r/include/asm/spinlock.h
@@ -30,11 +30,6 @@
 #define arch_spin_is_locked(x)		(*(volatile int *)(&(x)->slock) <= 0)
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, VAL > 0);
-}
-
 /**
  * arch_spin_trylock - Try spin lock and return a result
  * @lock: Pointer to the lock variable
diff --git a/arch/metag/include/asm/spinlock.h b/arch/metag/include/asm/spinlock.h
index c0c7a22be1ae..ddf7fe5708a6 100644
--- a/arch/metag/include/asm/spinlock.h
+++ b/arch/metag/include/asm/spinlock.h
@@ -15,11 +15,6 @@
  * locked.
  */
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
 #define	arch_read_lock_flags(lock, flags) arch_read_lock(lock)
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index a8df44d60607..81b4945031ee 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -50,22 +50,6 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u16 owner = READ_ONCE(lock->h.serving_now);
-	smp_rmb();
-	for (;;) {
-		arch_spinlock_t tmp = READ_ONCE(*lock);
-
-		if (tmp.h.serving_now == tmp.h.ticket ||
-		    tmp.h.serving_now != owner)
-			break;
-
-		cpu_relax();
-	}
-	smp_acquire__after_ctrl_dep();
-}
-
 static inline int arch_spin_is_contended(arch_spinlock_t *lock)
 {
 	u32 counters = ACCESS_ONCE(lock->lock);
diff --git a/arch/mn10300/include/asm/spinlock.h b/arch/mn10300/include/asm/spinlock.h
index 9c7b8f7942d8..fe413b41df6c 100644
--- a/arch/mn10300/include/asm/spinlock.h
+++ b/arch/mn10300/include/asm/spinlock.h
@@ -26,11 +26,6 @@
 
 #define arch_spin_is_locked(x)	(*(volatile signed char *)(&(x)->slock) != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, !VAL);
-}
-
 static inline void arch_spin_unlock(arch_spinlock_t *lock)
 {
 	asm volatile(
diff --git a/arch/parisc/include/asm/spinlock.h b/arch/parisc/include/asm/spinlock.h
index e32936cd7f10..55bfe4affca3 100644
--- a/arch/parisc/include/asm/spinlock.h
+++ b/arch/parisc/include/asm/spinlock.h
@@ -14,13 +14,6 @@ static inline int arch_spin_is_locked(arch_spinlock_t *x)
 
 #define arch_spin_lock(lock) arch_spin_lock_flags(lock, 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *x)
-{
-	volatile unsigned int *a = __ldcw_align(x);
-
-	smp_cond_load_acquire(a, VAL);
-}
-
 static inline void arch_spin_lock_flags(arch_spinlock_t *x,
 					 unsigned long flags)
 {
diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
index 8c1b913de6d7..d256e448ea49 100644
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -170,39 +170,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	lock->slock = 0;
 }
 
-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, 0) "\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 (lock->slock) {
-		HMT_low();
-		if (SHARED_PROCESSOR)
-			__spin_yield(lock);
-	}
-	HMT_medium();
-
-out:
-	smp_mb();
-}
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
diff --git a/arch/s390/include/asm/spinlock.h b/arch/s390/include/asm/spinlock.h
index f7838ecd83c6..217ee5210c32 100644
--- a/arch/s390/include/asm/spinlock.h
+++ b/arch/s390/include/asm/spinlock.h
@@ -98,13 +98,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lp)
 		: "cc", "memory");
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	while (arch_spin_is_locked(lock))
-		arch_spin_relax(lock);
-	smp_acquire__after_ctrl_dep();
-}
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
diff --git a/arch/sh/include/asm/spinlock-cas.h b/arch/sh/include/asm/spinlock-cas.h
index c46e8cc7b515..5ed7dbbd94ff 100644
--- a/arch/sh/include/asm/spinlock-cas.h
+++ b/arch/sh/include/asm/spinlock-cas.h
@@ -29,11 +29,6 @@ static inline unsigned __sl_cas(volatile unsigned *p, unsigned old, unsigned new
 #define arch_spin_is_locked(x)		((x)->lock <= 0)
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, VAL > 0);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	while (!__sl_cas(&lock->lock, 1, 0));
diff --git a/arch/sh/include/asm/spinlock-llsc.h b/arch/sh/include/asm/spinlock-llsc.h
index cec78143fa83..f77263aae760 100644
--- a/arch/sh/include/asm/spinlock-llsc.h
+++ b/arch/sh/include/asm/spinlock-llsc.h
@@ -21,11 +21,6 @@
 #define arch_spin_is_locked(x)		((x)->lock <= 0)
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, VAL > 0);
-}
-
 /*
  * Simple spin lock operations.  There are two variants, one clears IRQ's
  * on the local processor, one does not.
diff --git a/arch/sparc/include/asm/spinlock_32.h b/arch/sparc/include/asm/spinlock_32.h
index 8011e79f59c9..67345b2dc408 100644
--- a/arch/sparc/include/asm/spinlock_32.h
+++ b/arch/sparc/include/asm/spinlock_32.h
@@ -14,11 +14,6 @@
 
 #define arch_spin_is_locked(lock) (*((volatile unsigned char *)(lock)) != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	__asm__ __volatile__(
diff --git a/arch/sparc/include/asm/spinlock_64.h b/arch/sparc/include/asm/spinlock_64.h
index 07c9f2e9bf57..923d57f9b79d 100644
--- a/arch/sparc/include/asm/spinlock_64.h
+++ b/arch/sparc/include/asm/spinlock_64.h
@@ -26,11 +26,6 @@
 
 #define arch_spin_is_locked(lp)	((lp)->lock != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	unsigned long tmp;
diff --git a/arch/tile/include/asm/spinlock_32.h b/arch/tile/include/asm/spinlock_32.h
index b14b1ba5bf9c..cba8ba9b8da6 100644
--- a/arch/tile/include/asm/spinlock_32.h
+++ b/arch/tile/include/asm/spinlock_32.h
@@ -64,8 +64,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	lock->current_ticket = old_ticket + TICKET_QUANTUM;
 }
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock);
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
diff --git a/arch/tile/include/asm/spinlock_64.h b/arch/tile/include/asm/spinlock_64.h
index b9718fb4e74a..9a2c2d605752 100644
--- a/arch/tile/include/asm/spinlock_64.h
+++ b/arch/tile/include/asm/spinlock_64.h
@@ -58,8 +58,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	__insn_fetchadd4(&lock->lock, 1U << __ARCH_SPIN_CURRENT_SHIFT);
 }
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock);
-
 void arch_spin_lock_slow(arch_spinlock_t *lock, u32 val);
 
 /* Grab the "next" ticket number and bump it atomically.
diff --git a/arch/tile/lib/spinlock_32.c b/arch/tile/lib/spinlock_32.c
index 076c6cc43113..db9333f2447c 100644
--- a/arch/tile/lib/spinlock_32.c
+++ b/arch/tile/lib/spinlock_32.c
@@ -62,29 +62,6 @@ int arch_spin_trylock(arch_spinlock_t *lock)
 }
 EXPORT_SYMBOL(arch_spin_trylock);
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u32 iterations = 0;
-	int curr = READ_ONCE(lock->current_ticket);
-	int next = READ_ONCE(lock->next_ticket);
-
-	/* Return immediately if unlocked. */
-	if (next == curr)
-		return;
-
-	/* Wait until the current locker has released the lock. */
-	do {
-		delay_backoff(iterations++);
-	} while (READ_ONCE(lock->current_ticket) == curr);
-
-	/*
-	 * The TILE architecture doesn't do read speculation; therefore
-	 * a control dependency guarantees a LOAD->{LOAD,STORE} order.
-	 */
-	barrier();
-}
-EXPORT_SYMBOL(arch_spin_unlock_wait);
-
 /*
  * The low byte is always reserved to be the marker for a "tns" operation
  * since the low bit is set to "1" by a tns.  The next seven bits are
diff --git a/arch/tile/lib/spinlock_64.c b/arch/tile/lib/spinlock_64.c
index a4b5b2cbce93..de414c22892f 100644
--- a/arch/tile/lib/spinlock_64.c
+++ b/arch/tile/lib/spinlock_64.c
@@ -62,28 +62,6 @@ int arch_spin_trylock(arch_spinlock_t *lock)
 }
 EXPORT_SYMBOL(arch_spin_trylock);
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u32 iterations = 0;
-	u32 val = READ_ONCE(lock->lock);
-	u32 curr = arch_spin_current(val);
-
-	/* Return immediately if unlocked. */
-	if (arch_spin_next(val) == curr)
-		return;
-
-	/* Wait until the current locker has released the lock. */
-	do {
-		delay_backoff(iterations++);
-	} while (arch_spin_current(READ_ONCE(lock->lock)) == curr);
-
-	/*
-	 * The TILE architecture doesn't do read speculation; therefore
-	 * a control dependency guarantees a LOAD->{LOAD,STORE} order.
-	 */
-	barrier();
-}
-EXPORT_SYMBOL(arch_spin_unlock_wait);
 
 /*
  * If the read lock fails due to a writer, we retry periodically
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index a36221cf6363..3bb49681ee24 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -33,11 +33,6 @@
 
 #define arch_spin_is_locked(x) ((x)->slock != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, !VAL);
-}
-
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
 static inline void arch_spin_lock(arch_spinlock_t *lock)
-- 
2.5.2

[toc] | [prev] | [next] | [standalone]

#1682465

From: Paul E. McKenney
> Sent: 06 July 2017 00:30
> There is no agreed-upon definition of spin_unlock_wait()'s semantics,
> and it appears that all callers could do just as well with a lock/unlock
> pair.  This series therefore removes spin_unlock_wait() and changes
> its users to instead use a lock/unlock pair.  The commits are as follows,
> in three groups:
> 
> 1-7.	Change uses of spin_unlock_wait() and raw_spin_unlock_wait()
> 	to instead use a spin_lock/spin_unlock pair.  These may be
> 	applied in any order, but must be applied before any later
> 	commits in this series.  The commit logs state why I believe
> 	that these commits won't noticeably degrade performance.

I can't help feeling that it might be better to have a spin_lock_sync()
call that is equivalent to a spin_lock/spin_unlock pair.
The default implementation being an inline function that does exactly that.
This would let an architecture implement a more efficient version.

It might even be that this is the defined semantics of spin_unlock_wait().

Note that it can only be useful to do a spin_lock/unlock pair if it is
impossible for another code path to try to acquire the lock.
(Or, at least, the code can't care if the lock is acquired just after.)
So if it can de determined that the lock isn't held (a READ_ONCE()
might be enough) the lock itself need not be acquired (with the
associated slow bus cycles).

	David

[toc] | [prev] | [next] | [standalone]

#1682511

On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> From: Paul E. McKenney
> > Sent: 06 July 2017 00:30
> > There is no agreed-upon definition of spin_unlock_wait()'s semantics,
> > and it appears that all callers could do just as well with a lock/unlock
> > pair.  This series therefore removes spin_unlock_wait() and changes
> > its users to instead use a lock/unlock pair.  The commits are as follows,
> > in three groups:
> > 
> > 1-7.	Change uses of spin_unlock_wait() and raw_spin_unlock_wait()
> > 	to instead use a spin_lock/spin_unlock pair.  These may be
> > 	applied in any order, but must be applied before any later
> > 	commits in this series.  The commit logs state why I believe
> > 	that these commits won't noticeably degrade performance.
> 
> I can't help feeling that it might be better to have a spin_lock_sync()
> call that is equivalent to a spin_lock/spin_unlock pair.
> The default implementation being an inline function that does exactly that.
> This would let an architecture implement a more efficient version.
> 
> It might even be that this is the defined semantics of spin_unlock_wait().

That was in fact my first proposal, see the comment header in current
mainline for spin_unlock_wait() in include/linux/spinlock.h.  But Linus
quite rightly pointed out that if spin_unlock_wait() was to be defined in
this way, we should get rid of spin_unlock_wait() entirely, especially
given that there are not very many calls to spin_unlock_wait() and
also given that none of them are particularly performance critical.
Hence the current patch set, which does just that.

> Note that it can only be useful to do a spin_lock/unlock pair if it is
> impossible for another code path to try to acquire the lock.
> (Or, at least, the code can't care if the lock is acquired just after.)

Indeed!  As Oleg Nesterov pointed out, a spin_lock()/spin_unlock() pair
is sort of like synchronize_rcu() for a given lock, where that lock's
critical sections play the role of RCU read-side critical sections.
So anything before the pair is visible to later critical sections, and
anything in prior critical sections is visible to anything after the pair.
But again, as Linus pointed out, if we are going to have these semantics,
just do spin_lock() immediately followed by spin_unlock().

> So if it can de determined that the lock isn't held (a READ_ONCE()
> might be enough) the lock itself need not be acquired (with the
> associated slow bus cycles).

If you want the full semantics of a spin_lock()/spin_unlock() pair,
you need a full memory barrier before the READ_ONCE(), even on x86.
Without that memory barrier, you don't get the effect of the release
implicit in spin_unlock().

For weaker architectures, such as PowerPC and ARM, a READ_ONCE()
does -not- suffice, not at all, even with smp_mb() before and after.
I encourage you to take a look at arch_spin_unlock_wait() in arm64
and powerpc if youi are interested.  There were also some lengthy LKML
threads discussing this about 18 months ago that could be illuminating.

And yes, there are architecture-specific optimizations for an
empty spin_lock()/spin_unlock() critical section, and the current
arch_spin_unlock_wait() implementations show some of these optimizations.
But I expect that performance benefits would need to be demonstrated at
the system level.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1682558

On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> And yes, there are architecture-specific optimizations for an
> empty spin_lock()/spin_unlock() critical section, and the current
> arch_spin_unlock_wait() implementations show some of these optimizations.
> But I expect that performance benefits would need to be demonstrated at
> the system level.

I do in fact contended there are any optimizations for the exact
lock+unlock semantics.

The current spin_unlock_wait() is weaker. Most notably it will not (with
exception of ARM64/PPC for other reasons) cause waits on other CPUs.

[toc] | [prev] | [next] | [standalone]

#1682565

On Thu, Jul 06, 2017 at 06:10:47PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> > And yes, there are architecture-specific optimizations for an
> > empty spin_lock()/spin_unlock() critical section, and the current
> > arch_spin_unlock_wait() implementations show some of these optimizations.
> > But I expect that performance benefits would need to be demonstrated at
> > the system level.
> 
> I do in fact contended there are any optimizations for the exact
> lock+unlock semantics.

You lost me on this one.

> The current spin_unlock_wait() is weaker. Most notably it will not (with
> exception of ARM64/PPC for other reasons) cause waits on other CPUs.

Agreed, weaker semantics allow more optimizations.  So use cases needing
only the weaker semantics should more readily show performance benefits.
But either way, we need compelling use cases, and I do not believe that
any of the existing spin_unlock_wait() calls are compelling.  Perhaps I
am confused, but I am not seeing it for any of them.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1682582

On Thu, Jul 06, 2017 at 09:24:12AM -0700, Paul E. McKenney wrote:
> On Thu, Jul 06, 2017 at 06:10:47PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> > > And yes, there are architecture-specific optimizations for an
> > > empty spin_lock()/spin_unlock() critical section, and the current
> > > arch_spin_unlock_wait() implementations show some of these optimizations.
> > > But I expect that performance benefits would need to be demonstrated at
> > > the system level.
> > 
> > I do in fact contended there are any optimizations for the exact
> > lock+unlock semantics.
> 
> You lost me on this one.

For the exact semantics you'd have to fully participate in the fairness
protocol. You have to in fact acquire the lock in order to have the
other contending CPUs wait (otherwise my earlier case 3 will fail).

At that point I'm not sure there is much actual code you can leave out.

What actual optimization is there left at that point?

[toc] | [prev] | [next] | [standalone]

#1682595

On Thu, Jul 06, 2017 at 06:41:34PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 06, 2017 at 09:24:12AM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 06, 2017 at 06:10:47PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> > > > And yes, there are architecture-specific optimizations for an
> > > > empty spin_lock()/spin_unlock() critical section, and the current
> > > > arch_spin_unlock_wait() implementations show some of these optimizations.
> > > > But I expect that performance benefits would need to be demonstrated at
> > > > the system level.
> > > 
> > > I do in fact contended there are any optimizations for the exact
> > > lock+unlock semantics.
> > 
> > You lost me on this one.
> 
> For the exact semantics you'd have to fully participate in the fairness
> protocol. You have to in fact acquire the lock in order to have the
> other contending CPUs wait (otherwise my earlier case 3 will fail).
> 
> At that point I'm not sure there is much actual code you can leave out.
> 
> What actual optimization is there left at that point?

Got it.  It was just that I was having a hard time parsing your sentence.
You were contending that there are no optimizations for all implementations
for the full semantics.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1682583

On Thu, 6 Jul 2017, Paul E. McKenney wrote:

> On Thu, Jul 06, 2017 at 06:10:47PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> > > And yes, there are architecture-specific optimizations for an
> > > empty spin_lock()/spin_unlock() critical section, and the current
> > > arch_spin_unlock_wait() implementations show some of these optimizations.
> > > But I expect that performance benefits would need to be demonstrated at
> > > the system level.
> > 
> > I do in fact contended there are any optimizations for the exact
> > lock+unlock semantics.
> 
> You lost me on this one.
> 
> > The current spin_unlock_wait() is weaker. Most notably it will not (with
> > exception of ARM64/PPC for other reasons) cause waits on other CPUs.
> 
> Agreed, weaker semantics allow more optimizations.  So use cases needing
> only the weaker semantics should more readily show performance benefits.
> But either way, we need compelling use cases, and I do not believe that
> any of the existing spin_unlock_wait() calls are compelling.  Perhaps I
> am confused, but I am not seeing it for any of them.

If somebody really wants the full spin_unlock_wait semantics and
doesn't want to interfere with other CPUs, wouldn't synchronize_sched()
or something similar do the job?  It wouldn't be as efficient as
lock+unlock, but it also wouldn't affect other CPUs.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1682591

On Thu, Jul 06, 2017 at 12:49:12PM -0400, Alan Stern wrote:
> On Thu, 6 Jul 2017, Paul E. McKenney wrote:
> 
> > On Thu, Jul 06, 2017 at 06:10:47PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> > > > And yes, there are architecture-specific optimizations for an
> > > > empty spin_lock()/spin_unlock() critical section, and the current
> > > > arch_spin_unlock_wait() implementations show some of these optimizations.
> > > > But I expect that performance benefits would need to be demonstrated at
> > > > the system level.
> > > 
> > > I do in fact contended there are any optimizations for the exact
> > > lock+unlock semantics.
> > 
> > You lost me on this one.
> > 
> > > The current spin_unlock_wait() is weaker. Most notably it will not (with
> > > exception of ARM64/PPC for other reasons) cause waits on other CPUs.
> > 
> > Agreed, weaker semantics allow more optimizations.  So use cases needing
> > only the weaker semantics should more readily show performance benefits.
> > But either way, we need compelling use cases, and I do not believe that
> > any of the existing spin_unlock_wait() calls are compelling.  Perhaps I
> > am confused, but I am not seeing it for any of them.
> 
> If somebody really wants the full spin_unlock_wait semantics and
> doesn't want to interfere with other CPUs, wouldn't synchronize_sched()
> or something similar do the job?  It wouldn't be as efficient as
> lock+unlock, but it also wouldn't affect other CPUs.

So please don't do that. That'll create massive pain for RT. Also I
don't think it works. The whole point was that spin_unlock_wait() is
_cheaper_ than lock()+unlock(). If it gets to be more expensive there is
absolutely no point in using it.

[toc] | [prev] | [next] | [standalone]

#1682671

On Thu, 6 Jul 2017, Peter Zijlstra wrote:

> On Thu, Jul 06, 2017 at 12:49:12PM -0400, Alan Stern wrote:
> > On Thu, 6 Jul 2017, Paul E. McKenney wrote:
> > 
> > > On Thu, Jul 06, 2017 at 06:10:47PM +0200, Peter Zijlstra wrote:
> > > > On Thu, Jul 06, 2017 at 08:21:10AM -0700, Paul E. McKenney wrote:
> > > > > And yes, there are architecture-specific optimizations for an
> > > > > empty spin_lock()/spin_unlock() critical section, and the current
> > > > > arch_spin_unlock_wait() implementations show some of these optimizations.
> > > > > But I expect that performance benefits would need to be demonstrated at
> > > > > the system level.
> > > > 
> > > > I do in fact contended there are any optimizations for the exact
> > > > lock+unlock semantics.
> > > 
> > > You lost me on this one.
> > > 
> > > > The current spin_unlock_wait() is weaker. Most notably it will not (with
> > > > exception of ARM64/PPC for other reasons) cause waits on other CPUs.
> > > 
> > > Agreed, weaker semantics allow more optimizations.  So use cases needing
> > > only the weaker semantics should more readily show performance benefits.
> > > But either way, we need compelling use cases, and I do not believe that
> > > any of the existing spin_unlock_wait() calls are compelling.  Perhaps I
> > > am confused, but I am not seeing it for any of them.
> > 
> > If somebody really wants the full spin_unlock_wait semantics and
> > doesn't want to interfere with other CPUs, wouldn't synchronize_sched()
> > or something similar do the job?  It wouldn't be as efficient as
> > lock+unlock, but it also wouldn't affect other CPUs.
> 
> So please don't do that. That'll create massive pain for RT. Also I
> don't think it works. The whole point was that spin_unlock_wait() is
> _cheaper_ than lock()+unlock(). If it gets to be more expensive there is
> absolutely no point in using it.

Of course; that is obvious.

I was making a rhetorical point: You should not try to justify
spin_unlock_wait() on the basis that it doesn't cause waits on other
CPUs.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1682542

On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> From: Paul E. McKenney
> > Sent: 06 July 2017 00:30
> > There is no agreed-upon definition of spin_unlock_wait()'s semantics,
> > and it appears that all callers could do just as well with a lock/unlock
> > pair.  This series therefore removes spin_unlock_wait() and changes
> > its users to instead use a lock/unlock pair.  The commits are as follows,
> > in three groups:
> > 
> > 1-7.	Change uses of spin_unlock_wait() and raw_spin_unlock_wait()
> > 	to instead use a spin_lock/spin_unlock pair.  These may be
> > 	applied in any order, but must be applied before any later
> > 	commits in this series.  The commit logs state why I believe
> > 	that these commits won't noticeably degrade performance.
> 
> I can't help feeling that it might be better to have a spin_lock_sync()
> call that is equivalent to a spin_lock/spin_unlock pair.
> The default implementation being an inline function that does exactly that.
> This would let an architecture implement a more efficient version.

So that has the IRQ inversion issue found earlier in this patch-set. Not
actually doing the acquire though breaks other guarantees. See later.

> It might even be that this is the defined semantics of spin_unlock_wait().

As is, spin_unlock_wait() is somewhat ill defined. IIRC it grew from an
optimization by Oleg and subsequently got used elsewhere. And it being
the subtle bugger it is, there were bugs.

But part of the problem with that definition is fairness. For fair locks,
spin_lock()+spin_unlock() partakes in the fairness protocol. Many of the
things that would make spin_lock_sync() cheaper preclude it doing that.

(with the exception of ticket locks, those could actually do this).

But I think we can argue we don't in fact want that, all we really need
is to ensure the completion of the _current_ lock. But then you've
violated that equivalent thing.

As is, this is all a bit up in the air -- some of the ticket lock
variants are fair or minimal, the qspinlock on is prone to starvation
(although I think I can in fact fix that for qspinlock).

> Note that it can only be useful to do a spin_lock/unlock pair if it is
> impossible for another code path to try to acquire the lock.
> (Or, at least, the code can't care if the lock is acquired just after.)
> So if it can de determined that the lock isn't held (a READ_ONCE()
> might be enough) the lock itself need not be acquired (with the
> associated slow bus cycles).

Now look at the ARM64/PPC implementations that do explicit stores.

READ_ONCE() can only ever be sufficient on strongly ordered
architectures, but given many performance critical architectures are in
fact weakly ordered, you've opened up the exact can of worms we want to
get rid of the thing for.


So given:

(1)

  spin_lock()
  spin_unlock()

does not in fact provide memory ordering. Any prior/subsequent
load/stores can leak into the section and cross there.

What the thing does do however is serialize against other critical
sections in that:

(2)

  CPU0			CPU1

			spin_lock()
  spin_lock()
			X = 5
			spin_unlock()
  spin_unlock()
  r = X;

we must have r == 5 (due to address dependency on the lock; CPU1 does
the store to X, then a store-release to the lock. CPU0 then does a
load-acquire on the lock and that fully orders the subsequent load of X
to the prior store of X).


The other way around is more difficult though:

(3)

  CPU0			CPU1

  X=5
  spin_lock()
			spin_lock()
  spin_unlock()
			r = X;
			spin_unlock()

Where the above will in fact observe r == 5, this will be very difficult
to achieve with anything that will not let CPU1 wait. Which was the
entire premise of the original optimization by Oleg.

One of the later fixes we did to spin_unlock_wait() is to give it
ACQUIRE semantics to deal with (2) but even that got massively tricky,
see for example the ARM64 / PPC implementations.



In short, I doubt there is any real optimization possible if you want to
retain exact lock+unlock semantics.

Now on the one hand I feel like Oleg that it would be a shame to loose
the optimization, OTOH this thing is really really tricky to use,
and has lead to a number of bugs already.

[toc] | [prev] | [next] | [standalone]

#1682562

On Thu, Jul 06, 2017 at 06:05:55PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> > From: Paul E. McKenney

[ . . . ]

> Now on the one hand I feel like Oleg that it would be a shame to loose
> the optimization, OTOH this thing is really really tricky to use,
> and has lead to a number of bugs already.

I do agree, it is a bit sad to see these optimizations go.  So, should
this make mainline, I will be tagging the commits that spin_unlock_wait()
so that they can be easily reverted should someone come up with good
semantics and a compelling use case with compelling performance benefits.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1682590

On Thu, Jul 06, 2017 at 09:20:24AM -0700, Paul E. McKenney wrote:
> On Thu, Jul 06, 2017 at 06:05:55PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> > > From: Paul E. McKenney
> 
> [ . . . ]
> 
> > Now on the one hand I feel like Oleg that it would be a shame to loose
> > the optimization, OTOH this thing is really really tricky to use,
> > and has lead to a number of bugs already.
> 
> I do agree, it is a bit sad to see these optimizations go.  So, should
> this make mainline, I will be tagging the commits that spin_unlock_wait()
> so that they can be easily reverted should someone come up with good
> semantics and a compelling use case with compelling performance benefits.

Ha!, but what would constitute 'good semantics' ?

The current thing is something along the lines of:

  "Waits for the currently observed critical section
   to complete with ACQUIRE ordering such that it will observe
   whatever state was left by said critical section."

With the 'obvious' benefit of limited interference on those actually
wanting to acquire the lock, and a shorter wait time on our side too,
since we only need to wait for completion of the current section, and
not for however many contender are before us.

Not sure I have an actual (micro) benchmark that shows a difference
though.



Is this all good enough to retain the thing, I dunno. Like I said, I'm
conflicted on the whole thing. On the one hand its a nice optimization,
on the other hand I don't want to have to keep fixing these bugs.

[toc] | [prev] | [next] | [standalone]

#1682597

On Thu, Jul 06, 2017 at 06:50:36PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 06, 2017 at 09:20:24AM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 06, 2017 at 06:05:55PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> > > > From: Paul E. McKenney
> > 
> > [ . . . ]
> > 
> > > Now on the one hand I feel like Oleg that it would be a shame to loose
> > > the optimization, OTOH this thing is really really tricky to use,
> > > and has lead to a number of bugs already.
> > 
> > I do agree, it is a bit sad to see these optimizations go.  So, should
> > this make mainline, I will be tagging the commits that spin_unlock_wait()
> > so that they can be easily reverted should someone come up with good
> > semantics and a compelling use case with compelling performance benefits.
> 
> Ha!, but what would constitute 'good semantics' ?
> 
> The current thing is something along the lines of:
> 
>   "Waits for the currently observed critical section
>    to complete with ACQUIRE ordering such that it will observe
>    whatever state was left by said critical section."
> 
> With the 'obvious' benefit of limited interference on those actually
> wanting to acquire the lock, and a shorter wait time on our side too,
> since we only need to wait for completion of the current section, and
> not for however many contender are before us.
> 
> Not sure I have an actual (micro) benchmark that shows a difference
> though.
> 
> 
> 
> Is this all good enough to retain the thing, I dunno. Like I said, I'm
> conflicted on the whole thing. On the one hand its a nice optimization,
> on the other hand I don't want to have to keep fixing these bugs.

As I've said, I'd be keen to see us drop this and bring it back if/when we
get a compelling use-case along with performance numbers. At that point,
we'd be in a better position to define the semantics anyway, knowing what
exactly is expected by the use-case.

Will

[toc] | [prev] | [next] | [standalone]

#1682607

On Thu, Jul 06, 2017 at 06:08:50PM +0100, Will Deacon wrote:
> On Thu, Jul 06, 2017 at 06:50:36PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 06, 2017 at 09:20:24AM -0700, Paul E. McKenney wrote:
> > > On Thu, Jul 06, 2017 at 06:05:55PM +0200, Peter Zijlstra wrote:
> > > > On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> > > > > From: Paul E. McKenney
> > > 
> > > [ . . . ]
> > > 
> > > > Now on the one hand I feel like Oleg that it would be a shame to loose
> > > > the optimization, OTOH this thing is really really tricky to use,
> > > > and has lead to a number of bugs already.
> > > 
> > > I do agree, it is a bit sad to see these optimizations go.  So, should
> > > this make mainline, I will be tagging the commits that spin_unlock_wait()
> > > so that they can be easily reverted should someone come up with good
> > > semantics and a compelling use case with compelling performance benefits.
> > 
> > Ha!, but what would constitute 'good semantics' ?
> > 
> > The current thing is something along the lines of:
> > 
> >   "Waits for the currently observed critical section
> >    to complete with ACQUIRE ordering such that it will observe
> >    whatever state was left by said critical section."
> > 
> > With the 'obvious' benefit of limited interference on those actually
> > wanting to acquire the lock, and a shorter wait time on our side too,
> > since we only need to wait for completion of the current section, and
> > not for however many contender are before us.
> > 
> > Not sure I have an actual (micro) benchmark that shows a difference
> > though.
> > 
> > 
> > 
> > Is this all good enough to retain the thing, I dunno. Like I said, I'm
> > conflicted on the whole thing. On the one hand its a nice optimization,
> > on the other hand I don't want to have to keep fixing these bugs.
> 
> As I've said, I'd be keen to see us drop this and bring it back if/when we
> get a compelling use-case along with performance numbers. At that point,
> we'd be in a better position to define the semantics anyway, knowing what
> exactly is expected by the use-case.

Hear, hear!!!  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1682604

On Thu, Jul 06, 2017 at 06:50:36PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 06, 2017 at 09:20:24AM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 06, 2017 at 06:05:55PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> > > > From: Paul E. McKenney
> > 
> > [ . . . ]
> > 
> > > Now on the one hand I feel like Oleg that it would be a shame to loose
> > > the optimization, OTOH this thing is really really tricky to use,
> > > and has lead to a number of bugs already.
> > 
> > I do agree, it is a bit sad to see these optimizations go.  So, should
> > this make mainline, I will be tagging the commits that spin_unlock_wait()
> > so that they can be easily reverted should someone come up with good
> > semantics and a compelling use case with compelling performance benefits.
> 
> Ha!, but what would constitute 'good semantics' ?

At this point, it beats the heck out of me!  ;-)

> The current thing is something along the lines of:
> 
>   "Waits for the currently observed critical section
>    to complete with ACQUIRE ordering such that it will observe
>    whatever state was left by said critical section."
> 
> With the 'obvious' benefit of limited interference on those actually
> wanting to acquire the lock, and a shorter wait time on our side too,
> since we only need to wait for completion of the current section, and
> not for however many contender are before us.
> 
> Not sure I have an actual (micro) benchmark that shows a difference
> though.
> 
> 
> 
> Is this all good enough to retain the thing, I dunno. Like I said, I'm
> conflicted on the whole thing. On the one hand its a nice optimization,
> on the other hand I don't want to have to keep fixing these bugs.

Yeah, if I had seen a compelling use case...  Oleg's task_work case was
closest, but given that it involved a task-local lock that shouldn't
be all -that- heavily contended, it is hard to see there being all that
much difference.

But maybe I am missing something here?  Wouldn't be the first time...

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1683023

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jul 06, 2017 at 09:20:24AM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 06, 2017 at 06:05:55PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 06, 2017 at 02:12:24PM +0000, David Laight wrote:
> > > > From: Paul E. McKenney
> > 
> > [ . . . ]
> > 
> > > Now on the one hand I feel like Oleg that it would be a shame to loose
> > > the optimization, OTOH this thing is really really tricky to use,
> > > and has lead to a number of bugs already.
> > 
> > I do agree, it is a bit sad to see these optimizations go.  So, should
> > this make mainline, I will be tagging the commits that spin_unlock_wait()
> > so that they can be easily reverted should someone come up with good
> > semantics and a compelling use case with compelling performance benefits.
> 
> Ha!, but what would constitute 'good semantics' ?
> 
> The current thing is something along the lines of:
> 
>   "Waits for the currently observed critical section
>    to complete with ACQUIRE ordering such that it will observe
>    whatever state was left by said critical section."
> 
> With the 'obvious' benefit of limited interference on those actually
> wanting to acquire the lock, and a shorter wait time on our side too,
> since we only need to wait for completion of the current section, and
> not for however many contender are before us.

There's another, probably just as significant advantage: queued_spin_unlock_wait() 
is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On 
any bigger system this should make a very measurable difference - if 
spin_unlock_wait() is ever used in a performance critical code path.

> Not sure I have an actual (micro) benchmark that shows a difference
> though.

It should be pretty obvious from pretty much any profile, the actual lock+unlock 
sequence that modifies the lock cache line is essentially a global cacheline 
bounce.

> Is this all good enough to retain the thing, I dunno. Like I said, I'm 
> conflicted on the whole thing. On the one hand its a nice optimization, on the 
> other hand I don't want to have to keep fixing these bugs.

So on one hand it's _obvious_ that spin_unlock_wait() is both faster on the local 
_and_ the remote CPUs for any sort of use case where performance matters - I don't 
even understand how that can be argued otherwise.

The real question, does any use-case (we care about) exist.

Here's a quick list of all the use cases:

 net/netfilter/nf_conntrack_core.c:

   - This is I believe the 'original', historic spin_unlock_wait() usecase that
     still exists in the kernel. spin_unlock_wait() is only used in a rare case, 
     when the netfilter hash is resized via nf_conntrack_hash_resize() - which is 
     a very heavy operation to begin with. It will no doubt get slower with the 
     proposed changes, but it probably does not matter. A networking person 
     Acked-by would be nice though.

 drivers/ata/libata-eh.c:

   - Locking of the ATA port in ata_scsi_cmd_error_handler(), presumably this can
     race with IRQs and ioctls() on other CPUs. Very likely not performance 
     sensitive in any fashion, on IO errors things stop for many seconds anyway.

 ipc/sem.c:

   - A rare race condition branch in the SysV IPC semaphore freeing code in 
     exit_sem() - where even the main code flow is not performance sensitive, 
     because typical database workloads get their semaphore arrays during startup 
     and don't ever do heavy runtime allocation/freeing of them.

 kernel/sched/completion.c:

   - completion_done(). This is actually a (comparatively) rarely used completion 
     API call - almost all the upstream usecases are in drivers, plus two in 
     filesystems - neither usecase seems in a performance critical hot path. 
     Completions typically involve scheduling and context switching, so in the 
     worst case the proposed change adds overhead to a scheduling slow path.

So I'd argue that unless there's some surprising performance aspect of a 
completion_done() user, the proposed changes should not cause any performance 
trouble.

In fact I'd argue that any future high performance spin_unlock_wait() user is 
probably better off open coding the unlock-wait poll loop (and possibly thinking 
hard about eliminating it altogether). If such patterns pop up in the kernel we 
can think about consolidating them into a single read-only primitive again.

I.e. I think the proposed changes are doing no harm, and the unavailability of a 
generic primitive does not hinder future optimizations either in any significant 
fashion.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683034

On Fri, Jul 07, 2017 at 10:31:28AM +0200, Ingo Molnar wrote:
> Here's a quick list of all the use cases:
> 
>  net/netfilter/nf_conntrack_core.c:
> 
>    - This is I believe the 'original', historic spin_unlock_wait() usecase that
>      still exists in the kernel. spin_unlock_wait() is only used in a rare case, 
>      when the netfilter hash is resized via nf_conntrack_hash_resize() - which is 
>      a very heavy operation to begin with. It will no doubt get slower with the 
>      proposed changes, but it probably does not matter. A networking person 
>      Acked-by would be nice though.
> 
>  drivers/ata/libata-eh.c:
> 
>    - Locking of the ATA port in ata_scsi_cmd_error_handler(), presumably this can
>      race with IRQs and ioctls() on other CPUs. Very likely not performance 
>      sensitive in any fashion, on IO errors things stop for many seconds anyway.
> 
>  ipc/sem.c:
> 
>    - A rare race condition branch in the SysV IPC semaphore freeing code in 
>      exit_sem() - where even the main code flow is not performance sensitive, 
>      because typical database workloads get their semaphore arrays during startup 
>      and don't ever do heavy runtime allocation/freeing of them.
> 
>  kernel/sched/completion.c:
> 
>    - completion_done(). This is actually a (comparatively) rarely used completion 
>      API call - almost all the upstream usecases are in drivers, plus two in 
>      filesystems - neither usecase seems in a performance critical hot path. 
>      Completions typically involve scheduling and context switching, so in the 
>      worst case the proposed change adds overhead to a scheduling slow path.
> 

You missed the one in do_exit(), which I thought was the original one.

[toc] | [prev] | [next] | [standalone]

#1683114

* Peter Zijlstra <peterz@infradead.org> wrote:

> You missed the one in do_exit(), which I thought was the original one.

Indeed, it's raw_spin_unlock_wait() which my git grep pattern missed.

But it's not the original spin_unlock_wait(): the pi_lock and priority inheritance 
is a newfangled invention that Linus (rightfully) resisted for years.

The original spin_unlock_wait() was for the global scheduler_lock, long gone.

Here's the full history of the original spin_unlock_wait() usecase in the 
do_exit() path, for the historically interested:

 [1997/04] v2.1.36:

      the spin_unlock_wait() primitive gets introduced as part of release()

 [1998/08] v2.1.114:

      the release() usecase gets converted to an open coded spin_lock()+unlock() 
      poll loop over scheduler_lock

 [1999/05] v2.3.11pre3:

      open coded loop is changed over to poll p->has_cpu

 [1999/07] v2.3.12pre6:

      ->has_cpu loop poll loop is converted to a spin_lock()+unlock() 
      poll loop over runqueue_lock

 [2000/06] 2.4.0-test6pre4:

      combined open coded p->has_cpu poll loop is added back, in addition to the
      lock()+unlock() loop

 [2000/11] 2.4.0-test12pre4:

      lock+unlock loop is changed from scheduler_lock to task_lock

 [2001/11] v2.4.14.9:

       ->has_cpu gets renamed to ->cpus_runnable

 [2001/12] v2.5.1.10:

       poll loop is factored out from exit()'s release() function 
       to the scheduler's new wait_task_inactive() function

 ...

 [2017/07] v4.12:

      wait_task_inactive() is still alive and kicking. Its poll loop has
      increased in complexity, but it still does not use spin_unlock_wait()

So it was always a mess, and we relatively early flipped from the clever 
spin_unlock_wait() implementation to an open coded lock+unlock poll loop.

TL;DR: The original do_exit() usecase is gone, it does not use spin_unlock_wait(),
       since 1998.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web