Remove spin_unlock_wait()

Hi Paul,

Quoted 29 lines · Paul E. McKenney

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 replaces the spin_unlock_wait() calls in nf_conntrack_lock() and nf_conntrack_all_lock() with spin_lock() followed immediately by spin_unlock(). These functions do not appear to be invoked on any fastpaths.

Signed-off-by Paul E. McKenney<[email protected]>

Cc Pablo Neira Ayuso<[email protected]>

Cc Jozsef Kadlecsik<[email protected]>

Cc Florian Westphal<[email protected]>

Cc "David S. Miller"<[email protected]>

Cc <[email protected]>

Cc <[email protected]>

Cc <[email protected]>

Cc Will Deacon<[email protected]>

Cc Peter Zijlstra<[email protected]>

Cc Alan Stern<[email protected]>

Cc Andrea Parri<[email protected]>

Cc Linus Torvalds<[email protected]>

---
 net/netfilter/nf_conntrack_core.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

net/netfilter/nf_conntrack_core.c +0 -0

index e847dbaa0c6b..9f997859d160 100644

net/netfilter/nf_conntrack_core.c void nf_conntrack_lock(spinlock_t *lock) __acquires(lock) · line 100

Quoted 2 lines

net/netfilter/nf_conntrack_core.c +0 -0

@@ -99,15 +99,11 @@ void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
 	spin_lock(lock);
 	while (unlikely(nf_conntrack_locks_all)) {

I think here an ACQUIRE is missing.

Quoted 6 lines

net/netfilter/nf_conntrack_core.c +0 -5

 		spin_unlock(lock);
-
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the
-		 * spin_unlock_wait() loads below, to ensure
-		 * that 'nf_conntrack_locks_all_lock' is indeed held:

net/netfilter/nf_conntrack_core.c void nf_conntrack_lock(spinlock_t *lock) __acquires(lock) · line 108

Quoted 10 lines

net/netfilter/nf_conntrack_core.c +4 -3

-		 */
-		smp_rmb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
-		spin_unlock_wait(&nf_conntrack_locks_all_lock);
+		/* Wait for nf_conntrack_locks_all_lock holder to release ... */
+		spin_lock(&nf_conntrack_locks_all_lock);
+		spin_unlock(&nf_conntrack_locks_all_lock);
 		spin_lock(lock);
+		/* ... and retry. */
 	}
 }

As far as I see, nf_conntrack_locks[] nests inside nf_conntrack_lock_all_lock.
So

   spin_lock(&nf_conntrack_locks_all_lock);
   spin_lock(lock);
   spin_unlock(&nf_conntrack_locks_all_lock);

can replace the retry logic.

Correct? Then what about the attached patch?

--
Manfred

Quoted 1 line

From 453e7a77f3756d939c754031b092cbdfbd149559 Mon Sep 17 00:00:00 2001

From: Manfred Spraul <[email protected]>
Date: Sun, 21 Aug 2016 07:17:55 +0200
Subject: [PATCH] net/netfilter/nf_conntrack_core: Fix net_conntrack_lock()

As we want to remove spin_unlock_wait() and replace it with explicit spin_lock()/spin_unlock() calls, we can use this to simplify the locking.

In addition:
- Reading nf_conntrack_locks_all needs ACQUIRE memory ordering.
- The new code avoids the backwards loop.

Only slightly tested, I did not manage to trigger calls to nf_conntrack_all_lock().

Fixes b16c29191dc8

Signed-off-by Manfred Spraul <[email protected]>

Cc <[email protected]>

Cc Sasha Levin <[email protected]>

Cc Pablo Neira Ayuso <[email protected]>

Cc [email protected]

---
 net/netfilter/nf_conntrack_core.c | 44 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

net/netfilter/nf_conntrack_core.c +24 -20

index e847dba..1193565 100644
@@ -96,19 +96,24 @@ static struct conntrack_gc_work conntrack_gc_work;
 
 void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
 {
+	/* 1) Acquire the lock */
 	spin_lock(lock);
-	while (unlikely(nf_conntrack_locks_all)) {
-		spin_unlock(lock);
 
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the
-		 * spin_unlock_wait() loads below, to ensure
-		 * that 'nf_conntrack_locks_all_lock' is indeed held:
-		 */
-		smp_rmb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
-		spin_unlock_wait(&nf_conntrack_locks_all_lock);
-		spin_lock(lock);
-	}
+	/* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */
+	if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false))
+		return;
+
+	/* fast path failed, unlock */
+	spin_unlock(lock);
+
+	/* Slow path 1) get global lock */
+	spin_lock(&nf_conntrack_locks_all_lock);
+
+	/* Slow path 2) get the lock we want */
+	spin_lock(lock);
+
+	/* Slow path 3) release the global lock */
+	spin_unlock(&nf_conntrack_locks_all_lock);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_lock);
 

@@ -149,18 +154,17 @@ static void nf_conntrack_all_lock(void)
 	int i;
 
 	spin_lock(&nf_conntrack_locks_all_lock);
-	nf_conntrack_locks_all = true;
 
-	/*
-	 * Order the above store of 'nf_conntrack_locks_all' against
-	 * the spin_unlock_wait() loads below, such that if
-	 * nf_conntrack_lock() observes 'nf_conntrack_locks_all'
-	 * we must observe nf_conntrack_locks[] held:
-	 */
-	smp_mb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
+	nf_conntrack_locks_all = true;
 
 	for (i = 0; i < CONNTRACK_LOCKS; i++) {
-		spin_unlock_wait(&nf_conntrack_locks[i]);
+		spin_lock(&nf_conntrack_locks[i]);
+
+		/* This spin_unlock provides the "release" to ensure that
+		 * nf_conntrack_locks_all==true is visible to everyone that
+		 * acquired spin_lock(&nf_conntrack_locks[]).
+		 */
+		spin_unlock(&nf_conntrack_locks[i]);
 	}
 }
 

Signature · 1 line

2.9.4

As we want to remove spin_unlock_wait() and replace it with explicit spin_lock()/spin_unlock() calls, we can use this to simplify the locking.

In addition:
- Reading nf_conntrack_locks_all needs ACQUIRE memory ordering.
- The new code avoids the backwards loop.

Only slightly tested, I did not manage to trigger calls to nf_conntrack_all_lock().

Fixes b16c29191dc8

Signed-off-by Manfred Spraul <[email protected]>

Cc <[email protected]>

Cc Sasha Levin <[email protected]>

Cc Pablo Neira Ayuso <[email protected]>

Cc [email protected]

---
 net/netfilter/nf_conntrack_core.c | 44 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

net/netfilter/nf_conntrack_core.c +1 -4

index e847dba..1193565 100644
@@ -96,19 +96,24 @@ static struct conntrack_gc_work conntrack_gc_work;
 
 void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
 {
+	/* 1) Acquire the lock */
 	spin_lock(lock);
-	while (unlikely(nf_conntrack_locks_all)) {
-		spin_unlock(lock);
 
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the

net/netfilter/nf_conntrack_core.c +20 -9

+
+	/* fast path failed, unlock */
+	spin_unlock(lock);
+
+	/* Slow path 1) get global lock */
+	spin_lock(&nf_conntrack_locks_all_lock);
+
+	/* Slow path 2) get the lock we want */
+	spin_lock(lock);
+
+	/* Slow path 3) release the global lock */
+	spin_unlock(&nf_conntrack_locks_all_lock);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_lock);
 

@@ -149,18 +154,17 @@ static void nf_conntrack_all_lock(void)
 	int i;
 
 	spin_lock(&nf_conntrack_locks_all_lock);
-	nf_conntrack_locks_all = true;
 
-	/*
-	 * Order the above store of 'nf_conntrack_locks_all' against
-	 * the spin_unlock_wait() loads below, such that if
-	 * nf_conntrack_lock() observes 'nf_conntrack_locks_all'
-	 * we must observe nf_conntrack_locks[] held:
-	 */
-	smp_mb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
+	nf_conntrack_locks_all = true;
 
 	for (i = 0; i < CONNTRACK_LOCKS; i++) {
-		spin_unlock_wait(&nf_conntrack_locks[i]);
+		spin_lock(&nf_conntrack_locks[i]);
+
+		/* This spin_unlock provides the "release" to ensure that
+		 * nf_conntrack_locks_all==true is visible to everyone that
+		 * acquired spin_lock(&nf_conntrack_locks[]).
+		 */
+		spin_unlock(&nf_conntrack_locks[i]);
 	}
 }

Quoted 36 lines · Manfred Spraul

As we want to remove spin_unlock_wait() and replace it with explicit spin_lock()/spin_unlock() calls, we can use this to simplify the locking.

In addition:
- Reading nf_conntrack_locks_all needs ACQUIRE memory ordering.
- The new code avoids the backwards loop.

Only slightly tested, I did not manage to trigger calls to nf_conntrack_all_lock().

Fixes b16c29191dc8

Signed-off-by Manfred Spraul <[email protected]>

Cc <[email protected]>

Cc Sasha Levin <[email protected]>

Cc Pablo Neira Ayuso <[email protected]>

Cc [email protected]

---
 net/netfilter/nf_conntrack_core.c | 44 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

net/netfilter/nf_conntrack_core.c +1 -4

index e847dba..1193565 100644
@@ -96,19 +96,24 @@ static struct conntrack_gc_work conntrack_gc_work;
 
 void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
 {
+	/* 1) Acquire the lock */
 	spin_lock(lock);
-	while (unlikely(nf_conntrack_locks_all)) {
-		spin_unlock(lock);
 
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the

net/netfilter/nf_conntrack_core.c static struct conntrack_gc_work conntrack_gc_work; · line 104

Quoted 10 lines

net/netfilter/nf_conntrack_core.c +3 -7

-		 * spin_unlock_wait() loads below, to ensure
-		 * that 'nf_conntrack_locks_all_lock' is indeed held:
-		 */
-		smp_rmb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
-		spin_unlock_wait(&nf_conntrack_locks_all_lock);
-		spin_lock(lock);
-	}
+	/* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */
+	if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false))
+		return;

As far as I can tell, this read does not need to have ACQUIRE semantics.

You need to guarantee that two things can never happen:

(1) We read nf_conntrack_locks_all == false, and this routine's critical section for nf_conntrack_locks[i] runs after the (empty) critical section for that lock in nf_conntrack_all_lock().

(2) We read nf_conntrack_locks_all == true, and this routine's critical section for nf_conntrack_locks_all_lock runs before the critical section in nf_conntrack_all_lock().

In fact, neither one can happen even if smp_load_acquire() is replaced with READ_ONCE(). The reason is simple enough, using this property of spinlocks:

If critical section CS1 runs before critical section CS2 (for the same lock) then: (a) every write coming before CS1's spin_unlock() will be visible to any read coming after CS2's spin_lock(), and (b) no write coming after CS2's spin_lock() will be visible to any read coming before CS1's spin_unlock().

Thus for (1), assuming the critical sections run in the order mentioned above, since nf_conntrack_all_lock() writes to nf_conntrack_locks_all before releasing nf_conntrack_locks[i], and since nf_conntrack_lock() acquires nf_conntrack_locks[i] before reading nf_conntrack_locks_all, by (a) the read will always see the write.

Similarly for (2), since nf_conntrack_all_lock() acquires nf_conntrack_locks_all_lock before writing to nf_conntrack_locks_all, and since nf_conntrack_lock() reads nf_conntrack_locks_all before releasing nf_conntrack_locks_all_lock, by (b) the read cannot see the write.

Quoted 53 lines · Alan Stern

Quoted 36 lines · Manfred Spraul

As we want to remove spin_unlock_wait() and replace it with explicit spin_lock()/spin_unlock() calls, we can use this to simplify the locking.

In addition:
- Reading nf_conntrack_locks_all needs ACQUIRE memory ordering.
- The new code avoids the backwards loop.

Only slightly tested, I did not manage to trigger calls to nf_conntrack_all_lock().

Fixes b16c29191dc8

Signed-off-by Manfred Spraul <[email protected]>

Cc <[email protected]>

Cc Sasha Levin <[email protected]>

Cc Pablo Neira Ayuso <[email protected]>

Cc [email protected]

---
  net/netfilter/nf_conntrack_core.c | 44 +++++++++++++++++++++------------------
  1 file changed, 24 insertions(+), 20 deletions(-)

net/netfilter/nf_conntrack_core.c +1 -4

index e847dba..1193565 100644
@@ -96,19 +96,24 @@ static struct conntrack_gc_work conntrack_gc_work;
 
  void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
  {
+	/* 1) Acquire the lock */
  	spin_lock(lock);
-	while (unlikely(nf_conntrack_locks_all)) {
-		spin_unlock(lock);
 
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the

net/netfilter/nf_conntrack_core.c static struct conntrack_gc_work conntrack_gc_work; · line 104

Quoted 10 lines

net/netfilter/nf_conntrack_core.c +3 -7

-		 * spin_unlock_wait() loads below, to ensure
-		 * that 'nf_conntrack_locks_all_lock' is indeed held:
-		 */
-		smp_rmb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
-		spin_unlock_wait(&nf_conntrack_locks_all_lock);
-		spin_lock(lock);
-	}
+	/* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */
+	if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false))
+		return;

As far as I can tell, this read does not need to have ACQUIRE semantics.

You need to guarantee that two things can never happen:

Quoted 8 lines

(1) We read nf_conntrack_locks_all == false, and this routine's critical section for nf_conntrack_locks[i] runs after the (empty) critical section for that lock in nf_conntrack_all_lock().

(2) We read nf_conntrack_locks_all == true, and this routine's critical section for nf_conntrack_locks_all_lock runs before the critical section in nf_conntrack_all_lock().

Quoted 9 lines

In fact, neither one can happen even if smp_load_acquire() is replaced with READ_ONCE(). The reason is simple enough, using this property of spinlocks:

If critical section CS1 runs before critical section CS2 (for the same lock) then: (a) every write coming before CS1's spin_unlock() will be visible to any read coming after CS2's spin_lock(), and (b) no write coming after CS2's spin_lock() will be visible to any read coming before CS1's spin_unlock().

Quoted 11 lines

Thus for (1), assuming the critical sections run in the order mentioned above, since nf_conntrack_all_lock() writes to nf_conntrack_locks_all before releasing nf_conntrack_locks[i], and since nf_conntrack_lock() acquires nf_conntrack_locks[i] before reading nf_conntrack_locks_all, by (a) the read will always see the write.

Similarly for (2), since nf_conntrack_all_lock() acquires nf_conntrack_locks_all_lock before writing to nf_conntrack_locks_all, and since nf_conntrack_lock() reads nf_conntrack_locks_all before releasing nf_conntrack_locks_all_lock, by (b) the read cannot see the write.

And the Linux kernel memory model (https://lwn.net/Articles/718628/
and https://lwn.net/Articles/720550/) agrees with Alan. Here is a litmus test, which emulates spin_lock() with xchg_acquire() and spin_unlock() with smp_store_release():

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

C C-ManfredSpraul-L1G1xchgnr.litmus

(* Expected result: Never. *)

{
}

P0(int *nfcla, spinlock_t *gbl, int *gbl_held, spinlock_t *lcl, int *lcl_held)

{
	/* Acquire local lock. */
	r10 = xchg_acquire(lcl, 1);
	r1 = READ_ONCE(*nfcla);
	if (r1) {
		smp_store_release(lcl, 0);
		r11 = xchg_acquire(gbl, 1);
		r12 = xchg_acquire(lcl, 1);
		smp_store_release(gbl, 0);
	}
	r2 = READ_ONCE(*gbl_held);
	WRITE_ONCE(*lcl_held, 1);
	WRITE_ONCE(*lcl_held, 0);
	smp_store_release(lcl, 0);
}

P1(int *nfcla, spinlock_t *gbl, int *gbl_held, spinlock_t *lcl, int *lcl_held)

{
	/* Acquire global lock. */
	r10 = xchg_acquire(gbl, 1);
	WRITE_ONCE(*nfcla, 1);
	r11 = xchg_acquire(lcl, 1);
	smp_store_release(lcl, 0);
	r2 = READ_ONCE(*lcl_held);
	WRITE_ONCE(*gbl_held, 1);
	WRITE_ONCE(*gbl_held, 0);

	smp_store_release(gbl, 0);
}

exists
((0:r2=1 \/ 1:r2=1) /\ 0:r10=0 /\ 0:r11=0 /\ 0:r12=0 /\ 1:r10=0 /\ 1:r11=0)

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

The memory model says that the forbidden state does not happen:

Manfred, any objections to my changing your patch as Alan suggests?

Quoted 8 lines

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */
+	if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false))
+		return;

As far as I can tell, this read does not need to have ACQUIRE semantics.

You need to guarantee that two things can never happen:

Quoted 8 lines

Quoted 8 lines

(1) We read nf_conntrack_locks_all == false, and this routine's critical section for nf_conntrack_locks[i] runs after the (empty) critical section for that lock in nf_conntrack_all_lock().

(2) We read nf_conntrack_locks_all == true, and this routine's critical section for nf_conntrack_locks_all_lock runs before the critical section in nf_conntrack_all_lock().

I was looking at nf_conntrack_all_unlock:
There is a smp_store_release() - which memory barrier does this pair with?

nf_conntrack_all_unlock()

     <arbitrary writes>
     smp_store_release(a, false)
     spin_unlock(b);

nf_conntrack_lock()

     spin_lock(c);
     xx=read_once(a)
     if (xx==false)
         return
     <arbitrary read>

I tried to pair the memory barriers:
nf_conntrack_all_unlock() contains a smp_store_release(). What does that pair with?

(Alternatively, you could make nf_conntrack_all_unlock() do a lock+unlock on all the locks in the array, just like nf_conntrack_all_lock(). But of course, that would be a lot less efficient.)

From b549e0281b66124b62aa94543f91b0e616abaf52 Mon Sep 17 00:00:00 2001

Quoted 79 lines · Alan Stern

Quoted 36 lines · Manfred Spraul

As we want to remove spin_unlock_wait() and replace it with explicit spin_lock()/spin_unlock() calls, we can use this to simplify the locking.

In addition:
- Reading nf_conntrack_locks_all needs ACQUIRE memory ordering.
- The new code avoids the backwards loop.

Only slightly tested, I did not manage to trigger calls to nf_conntrack_all_lock().

Fixes b16c29191dc8

Signed-off-by Manfred Spraul <[email protected]>

Cc <[email protected]>

Cc Sasha Levin <[email protected]>

Cc Pablo Neira Ayuso <[email protected]>

Cc [email protected]

---
 net/netfilter/nf_conntrack_core.c | 44 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

net/netfilter/nf_conntrack_core.c +1 -4

index e847dba..1193565 100644
@@ -96,19 +96,24 @@ static struct conntrack_gc_work conntrack_gc_work;
 
 void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
 {
+	/* 1) Acquire the lock */
 	spin_lock(lock);
-	while (unlikely(nf_conntrack_locks_all)) {
-		spin_unlock(lock);
 
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the

net/netfilter/nf_conntrack_core.c static struct conntrack_gc_work conntrack_gc_work; · line 104

Quoted 10 lines

net/netfilter/nf_conntrack_core.c +3 -7

-		 * spin_unlock_wait() loads below, to ensure
-		 * that 'nf_conntrack_locks_all_lock' is indeed held:
-		 */
-		smp_rmb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
-		spin_unlock_wait(&nf_conntrack_locks_all_lock);
-		spin_lock(lock);
-	}
+	/* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */
+	if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false))
+		return;

As far as I can tell, this read does not need to have ACQUIRE semantics.

You need to guarantee that two things can never happen:

(1) We read nf_conntrack_locks_all == false, and this routine's critical section for nf_conntrack_locks[i] runs after the (empty) critical section for that lock in nf_conntrack_all_lock().

(2) We read nf_conntrack_locks_all == true, and this routine's critical section for nf_conntrack_locks_all_lock runs before the critical section in nf_conntrack_all_lock().

In fact, neither one can happen even if smp_load_acquire() is replaced with READ_ONCE(). The reason is simple enough, using this property of spinlocks:

If critical section CS1 runs before critical section CS2 (for the same lock) then: (a) every write coming before CS1's spin_unlock() will be visible to any read coming after CS2's spin_lock(), and (b) no write coming after CS2's spin_lock() will be visible to any read coming before CS1's spin_unlock().

Thus for (1), assuming the critical sections run in the order mentioned above, since nf_conntrack_all_lock() writes to nf_conntrack_locks_all before releasing nf_conntrack_locks[i], and since nf_conntrack_lock() acquires nf_conntrack_locks[i] before reading nf_conntrack_locks_all, by (a) the read will always see the write.

Quoted 5 lines

Similarly for (2), since nf_conntrack_all_lock() acquires nf_conntrack_locks_all_lock before writing to nf_conntrack_locks_all, and since nf_conntrack_lock() reads nf_conntrack_locks_all before releasing nf_conntrack_locks_all_lock, by (b) the read cannot see the write.

And the Linux kernel memory model (https://lwn.net/Articles/718628/
and https://lwn.net/Articles/720550/) agrees with Alan. Here is a litmus test, which emulates spin_lock() with xchg_acquire() and spin_unlock() with smp_store_release():

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

C C-ManfredSpraul-L1G1xchgnr.litmus

(* Expected result: Never. *)

{
}

P0(int *nfcla, spinlock_t *gbl, int *gbl_held, spinlock_t *lcl, int *lcl_held)

{
	/* Acquire local lock. */
	r10 = xchg_acquire(lcl, 1);
	r1 = READ_ONCE(*nfcla);
	if (r1) {
		smp_store_release(lcl, 0);
		r11 = xchg_acquire(gbl, 1);
		r12 = xchg_acquire(lcl, 1);
		smp_store_release(gbl, 0);
	}
	r2 = READ_ONCE(*gbl_held);
	WRITE_ONCE(*lcl_held, 1);
	WRITE_ONCE(*lcl_held, 0);
	smp_store_release(lcl, 0);
}

P1(int *nfcla, spinlock_t *gbl, int *gbl_held, spinlock_t *lcl, int *lcl_held)

{
	/* Acquire global lock. */
	r10 = xchg_acquire(gbl, 1);
	WRITE_ONCE(*nfcla, 1);
	r11 = xchg_acquire(lcl, 1);
	smp_store_release(lcl, 0);
	r2 = READ_ONCE(*lcl_held);
	WRITE_ONCE(*gbl_held, 1);

	WRITE_ONCE(*gbl_held, 0);
	smp_store_release(gbl, 0);
}

exists
((0:r2=1 \/ 1:r2=1) /\ 0:r10=0 /\ 0:r11=0 /\ 0:r12=0 /\ 1:r10=0 /\ 1:r11=0)

Quoted 118 lines · Paul E. McKenney

Quoted 79 lines · Alan Stern

Quoted 36 lines · Manfred Spraul

As we want to remove spin_unlock_wait() and replace it with explicit spin_lock()/spin_unlock() calls, we can use this to simplify the locking.

In addition:
- Reading nf_conntrack_locks_all needs ACQUIRE memory ordering.
- The new code avoids the backwards loop.

Only slightly tested, I did not manage to trigger calls to nf_conntrack_all_lock().

Fixes b16c29191dc8

Signed-off-by Manfred Spraul <[email protected]>

Cc <[email protected]>

Cc Sasha Levin <[email protected]>

Cc Pablo Neira Ayuso <[email protected]>

Cc [email protected]

---
 net/netfilter/nf_conntrack_core.c | 44 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

net/netfilter/nf_conntrack_core.c +1 -4

index e847dba..1193565 100644
@@ -96,19 +96,24 @@ static struct conntrack_gc_work conntrack_gc_work;
 
 void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
 {
+	/* 1) Acquire the lock */
 	spin_lock(lock);
-	while (unlikely(nf_conntrack_locks_all)) {
-		spin_unlock(lock);
 
-		/*
-		 * Order the 'nf_conntrack_locks_all' load vs. the

net/netfilter/nf_conntrack_core.c static struct conntrack_gc_work conntrack_gc_work; · line 104

Quoted 10 lines

net/netfilter/nf_conntrack_core.c +3 -7

-		 * spin_unlock_wait() loads below, to ensure
-		 * that 'nf_conntrack_locks_all_lock' is indeed held:
-		 */
-		smp_rmb(); /* spin_lock(&nf_conntrack_locks_all_lock) */
-		spin_unlock_wait(&nf_conntrack_locks_all_lock);
-		spin_lock(lock);
-	}
+	/* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */
+	if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false))
+		return;

As far as I can tell, this read does not need to have ACQUIRE semantics.

You need to guarantee that two things can never happen:

(1) We read nf_conntrack_locks_all == false, and this routine's critical section for nf_conntrack_locks[i] runs after the (empty) critical section for that lock in nf_conntrack_all_lock().

(2) We read nf_conntrack_locks_all == true, and this routine's critical section for nf_conntrack_locks_all_lock runs before the critical section in nf_conntrack_all_lock().

In fact, neither one can happen even if smp_load_acquire() is replaced with READ_ONCE(). The reason is simple enough, using this property of spinlocks:

If critical section CS1 runs before critical section CS2 (for the same lock) then: (a) every write coming before CS1's spin_unlock() will be visible to any read coming after CS2's spin_lock(), and (b) no write coming after CS2's spin_lock() will be visible to any read coming before CS1's spin_unlock().

Thus for (1), assuming the critical sections run in the order mentioned above, since nf_conntrack_all_lock() writes to nf_conntrack_locks_all before releasing nf_conntrack_locks[i], and since nf_conntrack_lock() acquires nf_conntrack_locks[i] before reading nf_conntrack_locks_all, by (a) the read will always see the write.

Quoted 5 lines

Similarly for (2), since nf_conntrack_all_lock() acquires nf_conntrack_locks_all_lock before writing to nf_conntrack_locks_all, and since nf_conntrack_lock() reads nf_conntrack_locks_all before releasing nf_conntrack_locks_all_lock, by (b) the read cannot see the write.

And the Linux kernel memory model (https://lwn.net/Articles/718628/
and https://lwn.net/Articles/720550/) agrees with Alan. Here is a litmus test, which emulates spin_lock() with xchg_acquire() and spin_unlock() with smp_store_release():

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

C C-ManfredSpraul-L1G1xchgnr.litmus

(* Expected result: Never. *)

{
}

P0(int *nfcla, spinlock_t *gbl, int *gbl_held, spinlock_t *lcl, int *lcl_held)

{
	/* Acquire local lock. */
	r10 = xchg_acquire(lcl, 1);
	r1 = READ_ONCE(*nfcla);
	if (r1) {
		smp_store_release(lcl, 0);
		r11 = xchg_acquire(gbl, 1);
		r12 = xchg_acquire(lcl, 1);
		smp_store_release(gbl, 0);
	}
	r2 = READ_ONCE(*gbl_held);
	WRITE_ONCE(*lcl_held, 1);
	WRITE_ONCE(*lcl_held, 0);
	smp_store_release(lcl, 0);
}

P1(int *nfcla, spinlock_t *gbl, int *gbl_held, spinlock_t *lcl, int *lcl_held)

Quoted 8 lines

{
	/* Acquire global lock. */
	r10 = xchg_acquire(gbl, 1);
	WRITE_ONCE(*nfcla, 1);
	r11 = xchg_acquire(lcl, 1);
	smp_store_release(lcl, 0);
	r2 = READ_ONCE(*lcl_held);
	WRITE_ONCE(*gbl_held, 1);

This litmus test is incomplete, because it omits the assignment setting nf_conntrack_locks_all back to false when the global lock is released. You should insert

	smp_store_release(*nfcla, 0);

right here.

Quoted 4 lines

	WRITE_ONCE(*gbl_held, 0);
	smp_store_release(gbl, 0);
}

Quoted 2 lines

exists
((0:r2=1 \/ 1:r2=1) /\ 0:r10=0 /\ 0:r11=0 /\ 0:r12=0 /\ 1:r10=0 /\ 1:r11=0)

With that addition, the litmus test fails unless the read of nfcla in P0 is an smp_load_acquire. So Manfred's patch should not be changed.

kernel/task_work.c void task_work_run(void) · line 113

Quoted 10 lines · Paul E. McKenney

kernel/task_work.c +2 -1

@@ -109,7 +109,8 @@ void task_work_run(void)
 		 * the first entry == work, cmpxchg(task_works) should
 		 * fail, but it can play with *work and other entries.
 		 */
-		raw_spin_unlock_wait(&task->pi_lock);
+		raw_spin_lock(&task->pi_lock);
+		raw_spin_unlock(&task->pi_lock);

Well, bit the you need spin_lock_irq(). And this is one of the reasons why I personally think unlock_wait have some sense...

kernel/task_work.c void task_work_run(void) · line 115

Quoted 2 lines

kernel/task_work.c +2 -0

+		raw_spin_lock_irq(&task->pi_lock);
+		raw_spin_unlock_irq(&task->pi_lock);

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+		raw_spin_lock_irq(&task->pi_lock);
+		raw_spin_unlock_irq(&task->pi_lock);

I agree that the spin_unlock_wait() implementations would avoid the deadlock with an acquisition from an interrupt handler, while also avoiding the need to momentarily disable interrupts. The ->pi_lock is a per-task lock, so I am assuming (perhaps naively) that contention is not a problem. So is the overhead of interrupt disabling likely to be noticeable here?

Quoted 10 lines · Paul E. McKenney

Quoted 3 lines

kernel/task_work.c void task_work_run(void) · line 105

Quoted 3 lines

kernel/task_work.c +2 -0

+		raw_spin_lock_irq(&task->pi_lock);
+		raw_spin_unlock_irq(&task->pi_lock);

I agree that the spin_unlock_wait() implementations would avoid the deadlock with an acquisition from an interrupt handler, while also avoiding the need to momentarily disable interrupts. The ->pi_lock is a per-task lock, so I am assuming (perhaps naively) that contention is not a problem. So is the overhead of interrupt disabling likely to be noticeable here?

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree, it has some problems, but still...

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

If not, we should probably change this code more:

kernel/task_work.c +2 -6

@@ -96,20 +96,16 @@ void task_work_run(void)
 		 * work->func() can do task_work_add(), do not set
 		 * work_exited unless the list is empty.
 		 */
+		raw_spin_lock_irq(&task->pi_lock);
 		do {
 			work = READ_ONCE(task->task_works);
 			head = !work && (task->flags & PF_EXITING) ?
 				&work_exited : NULL;
 		} while (cmpxchg(&task->task_works, work, head) != work);
+		raw_spin_unlock_irq(&task->pi_lock);
 
 		if (!work)
 			break;
-		/*
-		 * Synchronize with task_work_cancel(). It can't remove
-		 * the first entry == work, cmpxchg(task_works) should
-		 * fail, but it can play with *work and other entries.
-		 */
-		raw_spin_unlock_wait(&task->pi_lock);
 
 		do {
 			next = work->next;

performance-wise this is almost the same, and if we do not really care about overhead we can simplify the code: this way it is obvious that we can't race with task_work_cancel().

Oleg.

Quoted 12 lines · Oleg Nesterov

Quoted 10 lines · Paul E. McKenney

Quoted 3 lines

kernel/task_work.c void task_work_run(void) · line 105

Quoted 3 lines

kernel/task_work.c +2 -0

+		raw_spin_lock_irq(&task->pi_lock);
+		raw_spin_unlock_irq(&task->pi_lock);

I agree that the spin_unlock_wait() implementations would avoid the deadlock with an acquisition from an interrupt handler, while also avoiding the need to momentarily disable interrupts. The ->pi_lock is a per-task lock, so I am assuming (perhaps naively) that contention is not a problem. So is the overhead of interrupt disabling likely to be noticeable here?

Quoted 4 lines

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree, it has some problems, but still...

Quoted 4 lines

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

If not, we should probably change this code more:

This looks -much- better than my patch! May I have your Signed-off-by?

Thanx, Paul

Quoted 32 lines

kernel/task_work.c +2 -6

@@ -96,20 +96,16 @@ void task_work_run(void)
 		 * work->func() can do task_work_add(), do not set
 		 * work_exited unless the list is empty.
 		 */
+		raw_spin_lock_irq(&task->pi_lock);
 		do {
 			work = READ_ONCE(task->task_works);
 			head = !work && (task->flags & PF_EXITING) ?
 				&work_exited : NULL;
 		} while (cmpxchg(&task->task_works, work, head) != work);
+		raw_spin_unlock_irq(&task->pi_lock);
 
 		if (!work)
 			break;
-		/*
-		 * Synchronize with task_work_cancel(). It can't remove
-		 * the first entry == work, cmpxchg(task_works) should
-		 * fail, but it can play with *work and other entries.
-		 */
-		raw_spin_unlock_wait(&task->pi_lock);
 
 		do {
 			next = work->next;

performance-wise this is almost the same, and if we do not really care about overhead we can simplify the code: this way it is obvious that we can't race with task_work_cancel().

Oleg.

Quoted 4 lines · Oleg Nesterov

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree,

Quoted 6 lines

it has some problems, but still...

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

If not, we should probably change this code more:

This looks -much- better than my patch! May I have your Signed-off-by?

Quoted 6 lines · Paul E. McKenney

Quoted 4 lines · Oleg Nesterov

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree,

                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

if it was not clear, I tried to say "why do we want to _remove_ unlock_wait".

Quoted 7 lines

Quoted 6 lines

it has some problems, but still...

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

If not, we should probably change this code more:

Quoted 1 line

This looks -much- better than my patch! May I have your Signed-off-by?

Only if you promise to replace all RCU flavors with a single simple implementation based on rwlock ;)

Seriously, of course I won't argue, and it seems that nobody except me likes this primitive, but to me spin_unlock_wait() looks like synchronize_rcu(() and sometimes it makes sense.

Including this particular case. task_work_run() is going to flush/destroy the ->task_works list, so it needs to wait until all currently executing "readers" (task_work_cancel()'s which have started before ->task_works was updated) have completed.

Quoted 23 lines · Oleg Nesterov

Quoted 6 lines · Paul E. McKenney

Quoted 4 lines · Oleg Nesterov

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree,

                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

if it was not clear, I tried to say "why do we want to _remove_ unlock_wait".

Quoted 8 lines

Quoted 6 lines

it has some problems, but still...

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

If not, we should probably change this code more:

This looks -much- better than my patch! May I have your Signed-off-by?

Only if you promise to replace all RCU flavors with a single simple implementation based on rwlock ;)

Quoted 3 lines

Seriously, of course I won't argue, and it seems that nobody except me likes this primitive, but to me spin_unlock_wait() looks like synchronize_rcu(() and sometimes it makes sense.

If it looks like synchronize_rcu(), why not actually use synchronize_rcu()?

Alan Stern

Quoted 4 lines

Including this particular case. task_work_run() is going to flush/destroy the ->task_works list, so it needs to wait until all currently executing "readers" (task_work_cancel()'s which have started before ->task_works was updated) have completed.

Quoted 6 lines · Paul E. McKenney

Quoted 4 lines · Oleg Nesterov

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree,

                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

if it was not clear, I tried to say "why do we want to _remove_ unlock_wait".

Quoted 5 lines

Quoted 5 lines

it has some problems, but still...

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

Quoted 3 lines

Quoted 1 line

If not, we should probably change this code more:

This looks -much- better than my patch! May I have your Signed-off-by?

Only if you promise to replace all RCU flavors with a single simple implementation based on rwlock ;)

Seriously, of course I won't argue, and it seems that nobody except me likes this primitive, but to me spin_unlock_wait() looks like synchronize_rcu(() and sometimes it makes sense.

Including this particular case. task_work_run() is going to flush/destroy the ->task_works list, so it needs to wait until all currently executing "readers" (task_work_cancel()'s which have started before ->task_works was updated) have completed.

Quoted 16 lines · Oleg Nesterov

Quoted 6 lines · Paul E. McKenney

Quoted 4 lines · Oleg Nesterov

I do not think the overhead will be noticeable in this particular case.

But I am not sure I understand why do we want to unlock_wait. Yes I agree,

                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

if it was not clear, I tried to say "why do we want to _remove_ unlock_wait".

Quoted 5 lines

Quoted 5 lines

it has some problems, but still...

The code above looks strange for me. If we are going to repeat this pattern the perhaps we should add a helper for lock+unlock and name it unlock_wait2 ;)

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

If not, we should probably change this code more:

This looks -much- better than my patch! May I have your Signed-off-by?

Only if you promise to replace all RCU flavors with a single simple implementation based on rwlock ;)

;-) ;-) ;-)

Here you go:

https://github.com/pramalhe/ConcurrencyFreaks/blob/master/papers/poormanurcu-2015.pdf

Quoted 3 lines

Seriously, of course I won't argue, and it seems that nobody except me likes this primitive, but to me spin_unlock_wait() looks like synchronize_rcu(() and sometimes it makes sense.

Well, that analogy was what led me to propose that its semantics be defined as spin_lock() immediately followed by spin_unlock(). But that didn't go over well.

Quoted 4 lines

Including this particular case. task_work_run() is going to flush/destroy the ->task_works list, so it needs to wait until all currently executing "readers" (task_work_cancel()'s which have started before ->task_works was updated) have completed.

Understood!

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 replaces the spin_unlock_wait() call in do_task_dead() with spin_lock() followed immediately by spin_unlock(). This should be safe from a performance perspective because the lock is this tasks ->pi_lock, and this is called only after the task exits.

kernel/sched/core.c +0 -0

index e91138fcde86..6dea3d9728c8 100644

Quoted 7 lines · Paul E. McKenney

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 replaces the spin_unlock_wait() call in do_task_dead() with spin_lock() followed immediately by spin_unlock(). This should be safe from a performance perspective because the lock is this tasks ->pi_lock, and this is called only after the task exits.

kernel/sched/core.c void __noreturn do_task_dead(void) · line 3465

Quoted 11 lines

kernel/sched/core.c +2 -1

index e91138fcde86..6dea3d9728c8 100644
@@ -3461,7 +3461,8 @@ void __noreturn do_task_dead(void)
         * is held by try_to_wake_up()
         */
        smp_mb();
-       raw_spin_unlock_wait(&current->pi_lock);
+       raw_spin_lock(&current->pi_lock);
+       raw_spin_unlock(&current->pi_lock);

Does the raw_spin_lock()/raw_spin_unlock() imply an smp_mb() or stronger? Maybe it would be clearer to remove the extra barrier if so.

kernel/sched/core.c void __noreturn do_task_dead(void) · line 3465

Quoted 7 lines

kernel/sched/core.c +3 -2

         * is held by try_to_wake_up()
         */
-       smp_mb();
-       raw_spin_unlock_wait(&current->pi_lock);
+       smp_mb__before_spinlock();
+       raw_spin_lock(&current->pi_lock);
+       raw_spin_unlock(&current->pi_lock);

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 replaces the spin_unlock_wait() call in exit_sem() with spin_lock() followed immediately by spin_unlock(). This should be safe from a performance perspective because exit_sem() is rarely invoked in production.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Manfred Spraul <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
  ipc/sem.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

ipc/sem.c +2 -1

index 947dc2348271..e88d0749a929 100644
@@ -2096,7 +2096,8 @@ void exit_sem(struct task_struct *tsk)
  			 * possibility where we exit while freeary() didn't
  			 * finish unlocking sem_undo_list.
  			 */
-			spin_unlock_wait(&ulp->lock);
+			spin_lock(&ulp->lock);
+			spin_unlock(&ulp->lock);
  			rcu_read_unlock();
  			break;
  		}

Quoted 7 lines · Paul E. McKenney

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 replaces the spin_unlock_wait() call in exit_sem() with spin_lock() followed immediately by spin_unlock(). This should be safe from a performance perspective because exit_sem() is rarely invoked in production.

Quoted 9 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Manfred Spraul <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

Acked-by Manfred Spraul <[email protected]>

Quoted 18 lines

---
 ipc/sem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

ipc/sem.c +2 -1

index 947dc2348271..e88d0749a929 100644
@@ -2096,7 +2096,8 @@ void exit_sem(struct task_struct *tsk)
 			 * possibility where we exit while freeary() didn't
 			 * finish unlocking sem_undo_list.
 			 */
-			spin_unlock_wait(&ulp->lock);
+			spin_lock(&ulp->lock);
+			spin_unlock(&ulp->lock);
 			rcu_read_unlock();
 			break;
 		}

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 spin_unlock_wait() and related definitions from core code.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 include/asm-generic/qspinlock.h |  14 -----
 include/linux/spinlock.h        |  31 -----------
 include/linux/spinlock_up.h     |   6 ---
 kernel/locking/qspinlock.c      | 117 ----------------------------------------
 4 files changed, 168 deletions(-)

kernel/locking/qspinlock.c +0 -25

index b2caec7315af..64a9051e4c2c 100644
@@ -267,123 +267,6 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
 #define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
 #endif
 
-/*
- * Various notes on spin_is_locked() and spin_unlock_wait(), which are
- * 'interesting' functions:
- *
- * PROBLEM: some architectures have an interesting issue with atomic ACQUIRE
- * operations in that the ACQUIRE applies to the LOAD _not_ the STORE (ARM64,
- * PPC). Also qspinlock has a similar issue per construction, the setting of
- * the locked byte can be unordered acquiring the lock proper.
- *
- * This gets to be 'interesting' in the following cases, where the /should/s
- * end up false because of this issue.
- *
- *
- * CASE 1:
- *
- * So the spin_is_locked() correctness issue comes from something like:
- *
- *   CPU0				CPU1
- *
- *   global_lock();			local_lock(i)
- *     spin_lock(&G)			  spin_lock(&L[i])
- *     for (i)				  if (!spin_is_locked(&G)) {
- *       spin_unlock_wait(&L[i]);	    smp_acquire__after_ctrl_dep();
- *					    return;
- *					  }

Quoted 19 lines · Paul E. McKenney

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 spin_unlock_wait() and related definitions from core code.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 include/asm-generic/qspinlock.h |  14 -----
 include/linux/spinlock.h        |  31 -----------
 include/linux/spinlock_up.h     |   6 ---
 kernel/locking/qspinlock.c      | 117 ----------------------------------------
 4 files changed, 168 deletions(-)

[...]

Quoted 38 lines

kernel/locking/qspinlock.c +0 -30

index b2caec7315af..64a9051e4c2c 100644
@@ -267,123 +267,6 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
 #define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
 #endif
 
-/*
- * Various notes on spin_is_locked() and spin_unlock_wait(), which are
- * 'interesting' functions:
- *
- * PROBLEM: some architectures have an interesting issue with atomic ACQUIRE
- * operations in that the ACQUIRE applies to the LOAD _not_ the STORE (ARM64,
- * PPC). Also qspinlock has a similar issue per construction, the setting of
- * the locked byte can be unordered acquiring the lock proper.
- *
- * This gets to be 'interesting' in the following cases, where the /should/s
- * end up false because of this issue.
- *
- *
- * CASE 1:
- *
- * So the spin_is_locked() correctness issue comes from something like:
- *
- *   CPU0				CPU1
- *
- *   global_lock();			local_lock(i)
- *     spin_lock(&G)			  spin_lock(&L[i])
- *     for (i)				  if (!spin_is_locked(&G)) {
- *       spin_unlock_wait(&L[i]);	    smp_acquire__after_ctrl_dep();
- *					    return;
- *					  }
- *					  // deal with fail
- *
- * Where it is important CPU1 sees G locked or CPU0 sees L[i] locked such
- * that there is exclusion between the two critical sections.
- *

kernel/locking/qspinlock.c static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock, · line 304

Quoted 5 lines

kernel/locking/qspinlock.c +0 -5

- * The load from spin_is_locked(&G) /should/ be constrained by the ACQUIRE from
- * spin_lock(&L[i]), and similarly the load(s) from spin_unlock_wait(&L[i])
- * /should/ be constrained by the ACQUIRE from spin_lock(&G).
- *
- * Similarly, later stuff is constrained by the ACQUIRE from CTRL+RMB.

Might be worth keeping this comment about spin_is_locked, since we're not removing that guy just yet!

Quoted 61 lines · Will Deacon

Quoted 19 lines · Paul E. McKenney

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 spin_unlock_wait() and related definitions from core code.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 include/asm-generic/qspinlock.h |  14 -----
 include/linux/spinlock.h        |  31 -----------
 include/linux/spinlock_up.h     |   6 ---
 kernel/locking/qspinlock.c      | 117 ----------------------------------------
 4 files changed, 168 deletions(-)

[...]

Quoted 38 lines

kernel/locking/qspinlock.c +0 -30

index b2caec7315af..64a9051e4c2c 100644
@@ -267,123 +267,6 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
 #define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
 #endif
 
-/*
- * Various notes on spin_is_locked() and spin_unlock_wait(), which are
- * 'interesting' functions:
- *
- * PROBLEM: some architectures have an interesting issue with atomic ACQUIRE
- * operations in that the ACQUIRE applies to the LOAD _not_ the STORE (ARM64,
- * PPC). Also qspinlock has a similar issue per construction, the setting of
- * the locked byte can be unordered acquiring the lock proper.
- *
- * This gets to be 'interesting' in the following cases, where the /should/s
- * end up false because of this issue.
- *
- *
- * CASE 1:
- *
- * So the spin_is_locked() correctness issue comes from something like:
- *
- *   CPU0				CPU1
- *
- *   global_lock();			local_lock(i)
- *     spin_lock(&G)			  spin_lock(&L[i])
- *     for (i)				  if (!spin_is_locked(&G)) {
- *       spin_unlock_wait(&L[i]);	    smp_acquire__after_ctrl_dep();
- *					    return;
- *					  }
- *					  // deal with fail
- *
- * Where it is important CPU1 sees G locked or CPU0 sees L[i] locked such
- * that there is exclusion between the two critical sections.
- *

Quoted 8 lines

kernel/locking/qspinlock.c static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock, · line 304

Quoted 5 lines

kernel/locking/qspinlock.c +0 -5

- * The load from spin_is_locked(&G) /should/ be constrained by the ACQUIRE from
- * spin_lock(&L[i]), and similarly the load(s) from spin_unlock_wait(&L[i])
- * /should/ be constrained by the ACQUIRE from spin_lock(&G).
- *
- * Similarly, later stuff is constrained by the ACQUIRE from CTRL+RMB.

Might be worth keeping this comment about spin_is_locked, since we're not removing that guy just yet!

Ah, all the examples had spin_unlock_wait() in them. So what I need to do is to create a spin_unlock_wait()-free example to illustrate the text starting with "The load from spin_is_locked(", correct?

I also need to check all uses of spin_is_locked(). There might no longer be any that rely on any particular ordering...

Quoted 71 lines · Paul E. McKenney

Quoted 61 lines · Will Deacon

Quoted 19 lines · Paul E. McKenney

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 spin_unlock_wait() and related definitions from core code.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 include/asm-generic/qspinlock.h |  14 -----
 include/linux/spinlock.h        |  31 -----------
 include/linux/spinlock_up.h     |   6 ---
 kernel/locking/qspinlock.c      | 117 ----------------------------------------
 4 files changed, 168 deletions(-)

[...]

Quoted 38 lines

kernel/locking/qspinlock.c +0 -30

index b2caec7315af..64a9051e4c2c 100644
@@ -267,123 +267,6 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
 #define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
 #endif
 
-/*
- * Various notes on spin_is_locked() and spin_unlock_wait(), which are
- * 'interesting' functions:
- *
- * PROBLEM: some architectures have an interesting issue with atomic ACQUIRE
- * operations in that the ACQUIRE applies to the LOAD _not_ the STORE (ARM64,
- * PPC). Also qspinlock has a similar issue per construction, the setting of
- * the locked byte can be unordered acquiring the lock proper.
- *
- * This gets to be 'interesting' in the following cases, where the /should/s
- * end up false because of this issue.
- *
- *
- * CASE 1:
- *
- * So the spin_is_locked() correctness issue comes from something like:
- *
- *   CPU0				CPU1
- *
- *   global_lock();			local_lock(i)
- *     spin_lock(&G)			  spin_lock(&L[i])
- *     for (i)				  if (!spin_is_locked(&G)) {
- *       spin_unlock_wait(&L[i]);	    smp_acquire__after_ctrl_dep();
- *					    return;
- *					  }
- *					  // deal with fail
- *
- * Where it is important CPU1 sees G locked or CPU0 sees L[i] locked such
- * that there is exclusion between the two critical sections.
- *

Quoted 8 lines

kernel/locking/qspinlock.c static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock, · line 304

Quoted 5 lines

kernel/locking/qspinlock.c +0 -5

- * The load from spin_is_locked(&G) /should/ be constrained by the ACQUIRE from
- * spin_lock(&L[i]), and similarly the load(s) from spin_unlock_wait(&L[i])
- * /should/ be constrained by the ACQUIRE from spin_lock(&G).
- *
- * Similarly, later stuff is constrained by the ACQUIRE from CTRL+RMB.

Might be worth keeping this comment about spin_is_locked, since we're not removing that guy just yet!

Quoted 3 lines

Ah, all the examples had spin_unlock_wait() in them. So what I need to do is to create a spin_unlock_wait()-free example to illustrate the text starting with "The load from spin_is_locked(", correct?

Yeah, I think so.

Quoted 2 lines

I also need to check all uses of spin_is_locked(). There might no longer be any that rely on any particular ordering...

Right. I think we're looking for the "insane case" as per 38b850a73034 (which was apparently used by ipc/sem.c at the time, but no longer).

There's a usage in kernel/debug/debug_core.c, but it doesn't fill me with joy.

Quoted 9 lines · Will Deacon

Quoted 2 lines · Paul E. McKenney

I also need to check all uses of spin_is_locked(). There might no longer be any that rely on any particular ordering...

Right. I think we're looking for the "insane case" as per 38b850a73034 (which was apparently used by ipc/sem.c at the time, but no longer).

There's a usage in kernel/debug/debug_core.c, but it doesn't fill me with joy.

That is indeed an interesting one... But my first round will be what semantics the implementations seem to provide:

Acquire courtesy of TSO: s390, sparc, x86.
Acquire: ia64 (in reality fully ordered).
Control dependency: alpha, arc, arm, blackfin, hexagon, m32r, mn10300, tile,
xtensa.
Control dependency plus leading full barrier: arm64, powerpc.
UP-only: c6x, cris, frv, h8300, m68k, microblaze nios2, openrisc, um, unicore32.

Special cases:
metag: Acquire if !CONFIG_METAG_SMP_WRITE_REORDERING.
Otherwise control dependency?
mips: Control dependency, acquire if CONFIG_CPU_CAVIUM_OCTEON.
parisc: Acquire courtesy of TSO, but why barrier in smp_load_acquire?
sh: Acquire if one of SH4A, SH5, or J2, otherwise acquire? UP-only?

Are these correct, or am I missing something with any of them?

Quoted 28 lines · Paul E. McKenney

Quoted 9 lines · Will Deacon

Quoted 2 lines · Paul E. McKenney

I also need to check all uses of spin_is_locked(). There might no longer be any that rely on any particular ordering...

Right. I think we're looking for the "insane case" as per 38b850a73034 (which was apparently used by ipc/sem.c at the time, but no longer).

There's a usage in kernel/debug/debug_core.c, but it doesn't fill me with joy.

That is indeed an interesting one... But my first round will be what semantics the implementations seem to provide:

Acquire courtesy of TSO: s390, sparc, x86.
Acquire: ia64 (in reality fully ordered).
Control dependency: alpha, arc, arm, blackfin, hexagon, m32r, mn10300, tile,
xtensa.
Control dependency plus leading full barrier: arm64, powerpc.
UP-only: c6x, cris, frv, h8300, m68k, microblaze nios2, openrisc, um, unicore32.

Special cases:
metag: Acquire if !CONFIG_METAG_SMP_WRITE_REORDERING.
Otherwise control dependency?
mips: Control dependency, acquire if CONFIG_CPU_CAVIUM_OCTEON.
parisc: Acquire courtesy of TSO, but why barrier in smp_load_acquire?
sh: Acquire if one of SH4A, SH5, or J2, otherwise acquire? UP-only?

Quoted 1 line

Are these correct, or am I missing something with any of them?

That looks about right but, at least on ARM, I think we have to consider the semantics of spin_is_locked with respect to the other spin_* functions, rather than in isolation.

For example, ARM only has a control dependency, but spin_lock has a trailing smp_mb() and spin_unlock has both leading and trailing smp_mb().

Agreed, and my next step is to look at spin_lock() followed by spin_is_locked(), not necessarily the same lock.

Quoted 3 lines · Paul E. McKenney

Agreed, and my next step is to look at spin_lock() followed by spin_is_locked(), not necessarily the same lock.

Hmm. Most (all?) "spin_is_locked()" really should be about the same thread that took the lock (ie it's about asserts and lock debugging).

The optimistic ABBA avoidance pattern for spinlocks *should* be

    spin_lock(inner)
    ...
    if (!try_lock(outer)) {
           spin_unlock(inner);
           .. do them in the right order ..

so I don't think spin_is_locked() should have any memory barriers.

In fact, the core function for spin_is_locked() is arguably arch_spin_value_unlocked() which doesn't even do the access itself.

Quoted 17 lines · Linus Torvalds

Quoted 3 lines · Paul E. McKenney

Agreed, and my next step is to look at spin_lock() followed by spin_is_locked(), not necessarily the same lock.

Hmm. Most (all?) "spin_is_locked()" really should be about the same thread that took the lock (ie it's about asserts and lock debugging).

The optimistic ABBA avoidance pattern for spinlocks *should* be

    spin_lock(inner)
    ...
    if (!try_lock(outer)) {
           spin_unlock(inner);
           .. do them in the right order ..

Quoted 4 lines

so I don't think spin_is_locked() should have any memory barriers.

In fact, the core function for spin_is_locked() is arguably arch_spin_value_unlocked() which doesn't even do the access itself.

Yeah, but there's some spaced-out stuff going on in kgdb_cpu_enter where it looks to me like raw_spin_is_locked is used for synchronization. My eyes are hurting looking at it, though.

That certainly is one interesting function, isn't it? I wonder what happens if you replace the raw_spin_is_locked() calls with an unlock under a trylock check? ;-)

Quoted 4 lines · Paul E. McKenney

That certainly is one interesting function, isn't it? I wonder what happens if you replace the raw_spin_is_locked() calls with an unlock under a trylock check? ;-)

Deadlock due to interrupts again?

Didn't your spin_unlock_wait() patches teach you anything? Checking state is fundamentally different from taking the lock. Even a trylock.

I guess you could try with the irqsave versions. But no, we're not doing that.

Quoted 8 lines · Linus Torvalds

Quoted 4 lines · Paul E. McKenney

That certainly is one interesting function, isn't it? I wonder what happens if you replace the raw_spin_is_locked() calls with an unlock under a trylock check? ;-)

Deadlock due to interrupts again?

Unless I am missing something subtle, the kgdb_cpu_enter() function in question has a local_irq_save() over the "interesting" portion of its workings, so interrupt-handler self-deadlock should not happen.

Quoted 2 lines

Didn't your spin_unlock_wait() patches teach you anything? Checking state is fundamentally different from taking the lock. Even a trylock.

That was an embarrassing bug, no two ways about it. :-/

Quoted 1 line

I guess you could try with the irqsave versions. But no, we're not doing that.

Again, no need in this case.

But I agree with Will's assessment of this function...

The raw_spin_is_locked() looks to be asking if -any- CPU holds the dbg_slave_lock, and the answer could of course change immediately on return from raw_spin_is_locked(). Perhaps the theory is that if other CPU holds the lock, this CPU is supposed to be subjected to kgdb_roundup_cpus(). Except that the CPU that held dbg_slave_lock might be just about to release that lock. Odd.

Seems like there should be a get_online_cpus() somewhere, but maybe that constraint is to be manually enforced.

Quoted 3 lines · Paul E. McKenney

Agreed, and my next step is to look at spin_lock() followed by spin_is_locked(), not necessarily the same lock.

Hmm. Most (all?) "spin_is_locked()" really should be about the same thread that took the lock (ie it's about asserts and lock debugging).

The optimistic ABBA avoidance pattern for spinlocks *should* be

    spin_lock(inner)
    ...
    if (!try_lock(outer)) {
           spin_unlock(inner);
           .. do them in the right order ..

so I don't think spin_is_locked() should have any memory barriers.

In fact, the core function for spin_is_locked() is arguably arch_spin_value_unlocked() which doesn't even do the access itself.

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().

Signed-off-by Paul E. McKenney <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 arch/arm64/include/asm/spinlock.h | 58 ++++-----------------------------------
 1 file changed, 5 insertions(+), 53 deletions(-)

Quoted 13 lines · Paul E. McKenney

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().

Signed-off-by Paul E. McKenney <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

Quoted 3 lines

---
 arch/arm64/include/asm/spinlock.h | 58 ++++-----------------------------------
 1 file changed, 5 insertions(+), 53 deletions(-)

I'm going to miss this code.

Acked-by Will Deacon <[email protected]>

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().

Signed-off-by Paul E. McKenney <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 arch/powerpc/include/asm/spinlock.h | 33 ---------------------------------
 1 file changed, 33 deletions(-)

arch/powerpc/include/asm/spinlock.h +0 -33

index 8c1b913de6d7..d256e448ea49 100644
@@ -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.

Signature · 1 line

2.5.2

Quoted 5 lines · Paul E. McKenney

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().

Quoted 10 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Linus Torvalds <[email protected]>

Acked-by Boqun Feng <[email protected]>

Regards,
Boqun

Quoted 51 lines

---
 arch/powerpc/include/asm/spinlock.h | 33 ---------------------------------
 1 file changed, 33 deletions(-)

arch/powerpc/include/asm/spinlock.h +0 -33

index 8c1b913de6d7..d256e448ea49 100644
@@ -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.

Signature · 1 line

2.5.2

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().

Quoted 4 lines · Paul E. McKenney

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().

Please don't make this one commit fopr every architecture.

Once something gets removed, it gets removed. There's no point in "remove it from architecture X". If there are no more users, we're done with it, and making it be 25 patches with the same commit message instead of just one doesn't help anybody.

Quoted 7 lines · Linus Torvalds

Quoted 4 lines · Paul E. McKenney

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().

Quoted 6 lines

Please don't make this one commit fopr every architecture.

Once something gets removed, it gets removed. There's no point in "remove it from architecture X". If there are no more users, we're done with it, and making it be 25 patches with the same commit message instead of just one doesn't help anybody.

Apologies, I will merge them.