net/rds: own the fastpath locks across connection teardown

Hi all,

This is v2 of the follow-up set to "net/rds: Bug fix ports, part 2"
[1] (v1 of this set is at [2]). During review of part 2, the later half of that series needed more work than a respin, so it was split off into this set together with the companion fixes identified along the way.

RDS connection teardown quiesces the transmit and receive-refill fast paths by waiting for the RDS_IN_XMIT/RDS_RECV_REFILL bits to be sampled clear. Sampling a bit clear is not owning it: the fast path can re-take its bit right after the wait returns and then run concurrently with the transport shutdown and the send-state reset. Oracle UEK closed this by making teardown acquire the bits as locks ("rds: Make sure transmit path and connection tear-down does not run concurrently"); patches 4 and 5 do the same for the two rds_send_path_reset() call sites upstream. These two are effectively v3 of patches 4 and 3 of "net/rds: Bug fix ports, part 2" [1].

Making teardown block on the bits as locks promotes three latent ordering bugs from rare to load-bearing, so they are fixed first:

Patch 1: release_in_xmit() checks waitqueue_active() after clear_bit_unlock(), which does not order that read; the wake-up of the (now uninterruptible, untimed) teardown wait can be lost. Use wq_has_sleeper().

Patch 2: rds_conn_path_reset() wipes the whole cp_flags word with a plain store. Once teardown owns bits in that word across the reset, a blanket store would end lock ownership early - and it already races atomic RMWs on the same word today. Clear the bits the reset is responsible for individually, as Oracle UEK also does.

Patch 3: rds_tcp_reset_callbacks() stores RDS_CONN_RESETTING unconditionally, which can overwrite the RDS_CONN_ERROR or RDS_CONN_DISCONNECTING of a shutdown already in progress on the same path and send that shutdown through an extra drop cycle. Once the accept path can park for the duration of a teardown (patch 5) that window widens, so make the transition conditional first, as Oracle UEK does.

With those in place, patch 4 converts rds_tcp_reset_callbacks() from waiting on RDS_IN_XMIT to acquiring it, holding it across the socket swap and rds_send_path_reset(), and patch 5 has rds_conn_shutdown() hold both bit locks across the transport shutdown and path reset. The order matters: with the accept path owning the lock first, no intermediate commit leaves it resuming on a socket pointer that a lock-holding teardown has already released.

[PATCH net-next 1/5] net/rds: use wq_has_sleeper() in release_in_xmit()
Restore full barrier before wake-up checks in release_in_xmit()

[PATCH net-next 2/5] net/rds: clear cp_flags bits individually in rds_conn_path_reset()
Partial port of commit d04896037223 ("net/rds: Preserve essential connection state flags")
https://github.com/oracle/linux-uek/commit/d04896037223

[PATCH net-next 3/5] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown
Port of commit 72c176a1d9ac ("net/rds: Don't force state RDS_CONN_RESETTING")
https://github.com/oracle/linux-uek/commit/72c176a1d9ac

[PATCH net-next 4/5] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
Extend the port in patch 5 to the second rds_send_path_reset() call site

[PATCH net-next 5/5] net/rds: acquire the fastpath locks in rds_conn_shutdown()
Port of commit 2b8aaa4f163b ("rds: Make sure transmit path and connection tear-down does not run concurrently")
https://github.com/oracle/linux-uek/commit/2b8aaa4f163b

Changes since v1 [2]:
- New patch 3, porting the UEK fix for the accept-vs-shutdown state race; the RESETTING store is now a conditional transition.
- Patches 4 and 5 (formerly 4 and 3) reordered so that rds_tcp_reset_callbacks() owns RDS_IN_XMIT before rds_conn_shutdown() does; no intermediate commit is a bisect hazard.
- Patch 2 gains a Fixes tag; patch 4 refreshes the stale block comment above rds_tcp_reset_callbacks(); patch 5's changelog describes the accept-side waiter parking on krdsd for the duration of a TCP teardown and why that is preferable to the race.

Questions and comments appreciated!

Thanks,
Allison

[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/

Allison Henderson (3):
net/rds: use wq_has_sleeper() in release_in_xmit() net/rds: clear cp_flags bits individually in rds_conn_path_reset() net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()

Gerd Rausch (1):
net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent
shutdown

Håkon Bugge (1):
net/rds: acquire the fastpath locks in rds_conn_shutdown()

 net/rds/connection.c | 32 +++++++++++++++++--
 net/rds/send.c       | 12 +++++--
 net/rds/tcp.c        | 76 +++++++++++++++++++++++++++++---------------
 3 files changed, 88 insertions(+), 32 deletions(-)

base-commit: 3da8c3c8b8fa99505624b65ef590482f48e766b6

2.25.1

release_in_xmit() clears RDS_IN_XMIT with clear_bit_unlock() and then checks waitqueue_active() to decide whether anyone needs waking. clear_bit_unlock() is only a release operation: it orders the critical section before the bit clear, but does not order the subsequent plain load of the wait queue head after it. The waiter side does the mirror image - it adds itself to the wait queue and then tests the bit. That is the classic store-buffering pattern: the releasing CPU can read the wait queue as empty while the waiting CPU still reads the bit as set, so the sleeper is never woken.

The waiters are rds_conn_shutdown() and rds_tcp_reset_callbacks(), both in uninterruptible wait_event() with no timeout. A lost wake-up strands the shutdown worker on its single-threaded workqueue until some other sender releases the bit again - and on a connection that is being torn down precisely because it failed, there may never be another sender.

The barrier used to be there: release_in_xmit() did clear_bit() followed by smp_mb__after_atomic() until commit 1422f28826d2 ("rds:
introduce acquire/release ordering in acquire/release_in_xmit()") folded both into clear_bit_unlock(), which strengthened the lock hand-off but silently dropped the full barrier the wake-up check depends on. The refill counterpart, release_refill() in net/rds/ib_recv.c, still carries its smp_mb__after_atomic() for exactly this reason.

Use wq_has_sleeper(), which is waitqueue_active() preceded by the required full barrier.

---
v2: unchanged.
v1: https://lore.kernel.org/netdev/[email protected]/

 net/rds/send.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 15a1b97f13e7..8aad185e4b1a 100644
@@ -114,8 +114,13 @@ static void release_in_xmit(struct rds_conn_path *cp)
 	 * hot path and finding waiters is very rare.  We don't want to walk
 	 * the system-wide hashed waitqueue buckets in the fast path only to
 	 * almost never find waiters.
+	 *
+	 * wq_has_sleeper() supplies the full barrier that orders the wait
+	 * queue read after the bit clear; clear_bit_unlock() alone is only
+	 * a release and would let this check read a stale empty queue,
+	 * losing the wake-up.
 	 */
-	if (waitqueue_active(&cp->cp_waitq))
+	if (wq_has_sleeper(&cp->cp_waitq))
 		wake_up_all(&cp->cp_waitq);
 }
 

2.25.1

rds_conn_path_reset() wipes the whole flag word with a plain cp->cp_flags = 0 store. Every other accessor of that word uses atomic bitops, and some of them can run concurrently with the reset:
RDS_LL_SEND_FULL is set from rds_send_xmit() and cleared from the transport completion paths, neither of which holds anything that excludes the shutdown worker. A plain store racing an atomic read-modify-write on the same word is a data race, and whichever side loses has its update silently discarded.

Clear the two bits the reset is actually responsible for instead. RDS_IN_XMIT and RDS_RECV_REFILL need no store at all here: the caller, rds_conn_shutdown(), waits for both to be clear before calling the transport shutdown and this reset.

This also gives every bit in cp_flags a single well-defined writer discipline, which the following patches rely on when they turn RDS_IN_XMIT and RDS_RECV_REFILL into bit locks held across the
teardown: a blanket store mid-teardown would destroy lock ownership that an atomic clear preserves.

Oracle UEK carries the same conversion ("net/rds: Preserve essential connection state flags"), motivated by its asynchronous shutdown state machine, whose progress and destroy flags must survive the reset. UEK's variant also clears RDS_IN_XMIT and RDS_RECV_REFILL because there the reset runs as the final step of a teardown that owns both bits, making those clears its unlock; upstream the unlock stays in rds_conn_shutdown(), which needs release semantics and a wake-up that a plain clear inside the reset would not provide.

Based on Oracle UEK commit "net/rds: Preserve essential connection state flags" by Gerd Rausch.

---
v2: add Fixes tag.
v1: https://lore.kernel.org/netdev/[email protected]/

 net/rds/connection.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 7c8ab8e973e1..ddd7e2291eea 100644
@@ -120,7 +120,15 @@ static void rds_conn_path_reset(struct rds_conn_path *cp)
 
 	rds_stats_inc(s_conn_reset);
 	rds_send_path_reset(cp);
-	cp->cp_flags = 0;
+
+	/* Clear the bits the reset is responsible for individually: a
+	 * blanket cp_flags = 0 is a plain store that can clobber a
+	 * concurrent atomic read-modify-write on the same word.
+	 * RDS_IN_XMIT and RDS_RECV_REFILL are already clear here - the
+	 * caller waited for both before tearing the transport down.
+	 */
+	clear_bit(RDS_LL_SEND_FULL, &cp->cp_flags);
+	clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
 
 	/* Do not clear next_rx_seq here, else we cannot distinguish
 	 * retransmitted packets from new packets, and will hand all

2.25.1

From: Gerd Rausch <[email protected]>

rds_tcp_reset_callbacks() resolves a duelling SYN by storing RDS_CONN_RESETTING into cp_state unconditionally. Nothing serializes that store against the shutdown path: rds_tcp_accept_one() checks for RDS_CONN_CONNECTING or RDS_CONN_ERROR under t_conn_path_lock, but neither rds_conn_path_drop(), which forces RDS_CONN_ERROR, nor rds_conn_shutdown(), which moves the path to RDS_CONN_DISCONNECTING under cp_cm_lock, takes that lock. The store can therefore land on top of a shutdown that is already in progress, or that gets queued right after the accept-side check.

When it does, the shutdown worker's final DISCONNECTING -> DOWN transition fails and the path goes through rds_conn_path_error() and a second drop/shutdown cycle instead of a clean reconnect, tearing down the socket the accept path has just installed. Before commit ad22d24be635 ("net/rds: No shortcut out of RDS_CONN_ERROR") a path found in RDS_CONN_RESETTING even made rds_conn_shutdown() bail out altogether.

Make the transition conditional: move CONNECTING -> RESETTING (or stay in RESETTING from an earlier duel), and drop the path in any other state so that the pending shutdown wins. In that case the accept path still installs the new socket, rds_connect_path_complete() then fails its RESETTING -> UP transition and drops it: the raced socket ends up torn down as it does today, but without disturbing the shutdown's state machine along the way.

Based on Oracle UEK commit "net/rds: Don't force state RDS_CONN_RESETTING" by Gerd Rausch.

[achender: port to net-next: use the two-argument
rds_conn_path_transition()/rds_conn_path_drop() and rewrite the changelog for the upstream shutdown path]

---
v2: new in this version, port of an Oracle UEK fix for the accept-vs-shutdown state race that the following patches would otherwise widen.

 net/rds/tcp.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index b263634ac750..d4877de68168 100644
@@ -150,9 +150,19 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	 * end up deadlocking with tcp_sendmsg(), and the RDS_IN_XMIT
 	 * would not get set. As a result, we set c_state to
 	 * RDS_CONN_RESETTTING, to ensure that rds_tcp_state_change
-	 * cannot mark rds_conn_path_up() in the window before lock_sock()
+	 * cannot mark rds_conn_path_up() in the window before lock_sock().
+	 *
+	 * Only make that transition if the path is still connecting
+	 * (or already resetting from an earlier duel), so that a
+	 * concurrent shutdown's RDS_CONN_ERROR/RDS_CONN_DISCONNECTING
+	 * is not overwritten; a path in any other state is dropped
+	 * and its pending shutdown wins.
 	 */
-	atomic_set(&cp->cp_state, RDS_CONN_RESETTING);
+	if (!rds_conn_path_transition(cp, RDS_CONN_CONNECTING,
+				      RDS_CONN_RESETTING) &&
+	    !rds_conn_path_transition(cp, RDS_CONN_RESETTING,
+				      RDS_CONN_RESETTING))
+		rds_conn_path_drop(cp, 0);
 	wait_event(cp->cp_waitq, !test_bit(RDS_IN_XMIT, &cp->cp_flags));
 	/* reset receive side state for rds_tcp_data_recv() for osock  */
 	cancel_delayed_work_sync(&cp->cp_send_w);

2.25.1

rds_tcp_reset_callbacks() quiesces the transmit path by setting the path state to RDS_CONN_RESETTING and then waiting for RDS_IN_XMIT to be sampled clear before swapping the underlying socket and calling rds_send_path_reset().

Sampling the bit clear is not the same as owning it: rds_send_xmit() can re-acquire RDS_IN_XMIT right after the wait_event() returns. Its state recheck after taking the lock is a store-buffering pattern (the resetter writes the state and reads the bit, the sender writes the bit and reads the state) and acquire_in_xmit() is only an acquire operation, so on weakly ordered architectures both sides can miss each other's write and the transmit path then runs concurrently with rds_send_path_reset() rewriting cp_xmit_* state - which is exactly what the comment above rds_send_path_reset() tells its callers to prevent.

Take the lock instead, hold it across the socket swap and rds_send_path_reset(), and release it with a wake-up at the end. The lock-ordering constraint documented above the wait still holds: the lock is acquired before lock_sock(), so a sender inside tcp_sendmsg() can never be waited on while we hold the socket lock.

Two details of the old code go away with the same change:

- t_sock is now read only after the lock is acquired. The old code cached it before waiting; the teardown in rds_conn_shutdown() releases that socket and clears t_sock, so a pointer cached before the wait can be stale by the time the accept path resumes. Reading it under RDS_IN_XMIT is what makes the exclusion complete once the teardown owns the same lock, which the next patch arranges.

- The old !osock early path called rds_send_path_reset() with no serialization at all. It now runs under the lock like the normal path. The conditional RDS_CONN_RESETTING transition of the previous patch happens before the socket check either way: a path found without a socket is either still connecting (its reconnect worker blocked on t_conn_path_lock) and legitimately goes RESETTING -> UP on the new socket, or it has been torn down meanwhile and is dropped.

The in-function comment describing the old wait-based quiesce is rewritten to describe the lock-based one, and the stale block comment above the function (which still described a return value and an incomplete list of t_sock writers) is refreshed.

---
Previously patch 4/4 of v1 and of "net/rds: Bug fix ports, part 2".
v2: reorder ahead of the rds_conn_shutdown() patch so that no intermediate commit leaves rds_tcp_reset_callbacks() resuming on a
socket pointer cached before the teardown owned the lock; refresh the stale block comment above the function; scope the in-function comment
to what the lock covers; note the interaction with the conditional RDS_CONN_RESETTING transition of the previous patch.
v1: https://lore.kernel.org/netdev/[email protected]/
part 2 v1: https://lore.kernel.org/netdev/[email protected]/

 net/rds/tcp.c | 64 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 25 deletions(-)

index d4877de68168..1ff9e846dfc7 100644
@@ -115,42 +115,42 @@ void rds_tcp_restore_callbacks(struct socket *sock,
 }
 
 /*
- * rds_tcp_reset_callbacks() switches the to the new sock and
- * returns the existing tc->t_sock.
+ * rds_tcp_reset_callbacks() switches a path to a new socket and
+ * releases the old one it finds in tc->t_sock, resolving a duelling
+ * SYN.
  *
- * The only functions that set tc->t_sock are rds_tcp_set_callbacks
- * and rds_tcp_reset_callbacks.  Send and receive trust that
- * it is set.  The absence of RDS_CONN_UP bit protects those paths
- * from being called while it isn't set.
+ * tc->t_sock is set by rds_tcp_set_callbacks() and cleared by
+ * rds_tcp_restore_callbacks(), from rds_tcp_conn_path_shutdown() and
+ * from here.  Send and receive trust that it is set: the absence of
+ * RDS_CONN_UP protects those paths from being called while it isn't,
+ * and the swap done here runs under RDS_IN_XMIT so that it cannot
+ * interleave with a sender already inside rds_send_xmit().
  */
 void rds_tcp_reset_callbacks(struct socket *sock,
 			     struct rds_conn_path *cp)
 {
 	struct rds_tcp_connection *tc = cp->cp_transport_data;
-	struct socket *osock = tc->t_sock;
-
-	if (!osock)
-		goto newsock;
+	struct socket *osock;
 
 	/* Need to resolve a duelling SYN between peers.
 	 * We have an outstanding SYN to this peer, which may
 	 * potentially have transitioned to the RDS_CONN_UP state,
 	 * so we must quiesce any send threads before resetting
-	 * cp_transport_data. We quiesce these threads by setting
-	 * cp_state to something other than RDS_CONN_UP, and then
-	 * waiting for any existing threads in rds_send_xmit to
-	 * complete release_in_xmit(). (Subsequent threads entering
-	 * rds_send_xmit() will bail on !rds_conn_up().
+	 * cp_transport_data.  Setting cp_state to something other
+	 * than RDS_CONN_UP stops new senders, and owning RDS_IN_XMIT
+	 * excludes any thread already inside rds_send_xmit() for the
+	 * whole socket swap and the rds_send_path_reset() below.
 	 *
-	 * However an incoming syn-ack at this point would end up
-	 * marking the conn as RDS_CONN_UP, and would again permit
-	 * rds_send_xmi() threads through, so ideally we would
-	 * synchronize on RDS_CONN_UP after lock_sock(), but cannot
-	 * do that: waiting on !RDS_IN_XMIT after lock_sock() may
-	 * end up deadlocking with tcp_sendmsg(), and the RDS_IN_XMIT
-	 * would not get set. As a result, we set c_state to
-	 * RDS_CONN_RESETTTING, to ensure that rds_tcp_state_change
-	 * cannot mark rds_conn_path_up() in the window before lock_sock().
+	 * An incoming syn-ack at this point would end up marking the
+	 * conn as RDS_CONN_UP, and would again permit rds_send_xmit()
+	 * threads through, so ideally we would synchronize on
+	 * RDS_CONN_UP after lock_sock(), but cannot do that: acquiring
+	 * RDS_IN_XMIT after lock_sock() may end up deadlocking with
+	 * tcp_sendmsg(), which takes the socket lock while holding
+	 * RDS_IN_XMIT.  As a result, we set c_state to
+	 * RDS_CONN_RESETTING, to ensure that rds_tcp_state_change
+	 * cannot mark rds_conn_path_up() in the window before
+	 * lock_sock().
 	 *
 	 * Only make that transition if the path is still connecting
 	 * (or already resetting from an earlier duel), so that a

@@ -163,7 +163,18 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	    !rds_conn_path_transition(cp, RDS_CONN_RESETTING,
 				      RDS_CONN_RESETTING))
 		rds_conn_path_drop(cp, 0);
-	wait_event(cp->cp_waitq, !test_bit(RDS_IN_XMIT, &cp->cp_flags));
+	wait_event(cp->cp_waitq,
+		   !test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags));
+
+	/* Read t_sock only while owning RDS_IN_XMIT.  The teardown in
+	 * rds_conn_shutdown() releases the old socket and clears
+	 * t_sock, so a pointer sampled before the wait can be stale
+	 * by the time we wake up.
+	 */
+	osock = tc->t_sock;
+	if (!osock)
+		goto newsock;
+
 	/* reset receive side state for rds_tcp_data_recv() for osock  */
 	cancel_delayed_work_sync(&cp->cp_send_w);
 	cancel_delayed_work_sync(&cp->cp_recv_w);

@@ -182,6 +193,9 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	lock_sock(sock->sk);
 	rds_tcp_set_callbacks(sock, cp);
 	release_sock(sock->sk);
+
+	clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
+	wake_up_all(&cp->cp_waitq);
 }
 
 /* Add tc to rds_tcp_tc_list and set tc->t_sock. See comments

2.25.1

From: Håkon Bugge <[email protected]>

rds_conn_shutdown() quiesces the transmit and receive-refill paths by waiting for RDS_IN_XMIT and RDS_RECV_REFILL to be sampled clear, and then runs the transport shutdown and rds_conn_path_reset(). Sampling the bits clear is not the same as owning them: the moment after the wait_event() returns, rds_send_xmit() can re-acquire RDS_IN_XMIT (or rds_ib_recv_refill() can re-acquire RDS_RECV_REFILL) and run concurrently with the teardown.

The sender does recheck the connection state after taking the lock, but that recheck is a classic store-buffering pattern: teardown writes the state and reads the bit while the sender writes the bit and reads the state. acquire_in_xmit() is only an acquire operation, so on weakly ordered architectures both sides can miss each other's write, and the transmit path then runs while the transport zeroes its rings (e.g. rds_ib_ring_init()) and rds_send_path_reset() rewrites the transmit state under it.

Oracle UEK fixed the same class of crashes - a 14-year tail of BUG_ON()s in rds_ib_sub_signaled(), unexpected op-codes and NULL dereferences in rds_ib_send_cqe_handler() during failover testing - by making the teardown path *acquire* the fastpath bit locks instead of testing them ("rds: Make sure transmit path and connection tear-down does not run concurrently"). Ownership of a single word is decided by RMW atomicity, so no cross-variable ordering is needed.

Do the same here: take both locks before calling the transport shutdown, hold them across rds_conn_path_reset(), and release them explicitly with a wake-up afterwards. The fastpath users of these bits - rds_send_xmit() and rds_ib_recv_refill() - are trylock style and back off while teardown owns the locks, so no new lock dependency is introduced for them. rds_tcp_reset_callbacks() is different: since the previous patch it acquires RDS_IN_XMIT as well, and it blocks doing so, so its wait now spans the teardown instead of at most one send batch. That waiter runs from rds_tcp_accept_one() on the single-threaded krdsd workqueue and holds rds_tcp_accept_lock and t_conn_path_lock while it waits, so a duelling SYN accepted while its path is being torn down parks accept processing for the duration of the teardown - for TCP bounded by the (up to 5 s) drain loop in rds_tcp_conn_path_shutdown(). The window is narrow: the accept-side state check has to pass before the teardown moves the path to RDS_CONN_DISCONNECTING. The alternative to parking is the accept path racing the teardown, which is what these patches close; making the teardown itself non-blocking is a separate item.

One observable side effect: the SENDING flag reported by rds-info has always mirrored RDS_IN_XMIT, so it now also covers the window where teardown owns the bit.

The comment in rds_send_xmit() describing the old sample-based handshake is updated to match.

[achender: reimplement for net-next shutdown path: acquire the existing
RDS_IN_XMIT/RDS_RECV_REFILL bit locks in rds_conn_shutdown() and release after teardown; update comments and commit message]

---
Previously patch 3/4 of v1 and of "net/rds: Bug fix ports, part 2".
v2: reorder after the rds_tcp_reset_callbacks() patch; describe in
the changelog that the accept-side waiter parks on krdsd holding rds_tcp_accept_lock/t_conn_path_lock for the (TCP: <= 5 s) duration
of the teardown, and why.
v1: https://lore.kernel.org/netdev/[email protected]/
part 2 v1: https://lore.kernel.org/netdev/[email protected]/

 net/rds/connection.c | 26 ++++++++++++++++++++++----
 net/rds/send.c       |  5 +++--
 2 files changed, 25 insertions(+), 6 deletions(-)

index ddd7e2291eea..a10b667c06c8 100644
@@ -124,8 +124,8 @@ static void rds_conn_path_reset(struct rds_conn_path *cp)
 	/* Clear the bits the reset is responsible for individually: a
 	 * blanket cp_flags = 0 is a plain store that can clobber a
 	 * concurrent atomic read-modify-write on the same word.
-	 * RDS_IN_XMIT and RDS_RECV_REFILL are already clear here - the
-	 * caller waited for both before tearing the transport down.
+	 * RDS_IN_XMIT and RDS_RECV_REFILL are owned by the caller,
+	 * which releases them once the teardown is complete.
 	 */
 	clear_bit(RDS_LL_SEND_FULL, &cp->cp_flags);
 	clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);

@@ -414,14 +414,32 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 		}
 		mutex_unlock(&cp->cp_cm_lock);
 
+		/* Quiesce the transmit and receive-refill paths by
+		 * acquiring their bit locks, not merely waiting for
+		 * them to be released: with a plain wait, either path
+		 * can re-take its lock the instant after we sample it
+		 * clear and then run concurrently with the transport
+		 * shutdown and the path reset below.  Holding both
+		 * locks across the teardown makes that structurally
+		 * impossible.
+		 */
 		wait_event(cp->cp_waitq,
-			   !test_bit(RDS_IN_XMIT, &cp->cp_flags));
+			   !test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags));
 		wait_event(cp->cp_waitq,
-			   !test_bit(RDS_RECV_REFILL, &cp->cp_flags));
+			   !test_and_set_bit(RDS_RECV_REFILL, &cp->cp_flags));
 
 		conn->c_trans->conn_path_shutdown(cp);
 		rds_conn_path_reset(cp);
 
+		/* Release the two locks and wake any waiter (e.g.
+		 * rds_tcp_reset_callbacks()) that blocked on them while
+		 * we held them.  rds_conn_path_reset() leaves both bits
+		 * alone: ownership ends here, not inside the reset.
+		 */
+		clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
+		clear_bit(RDS_RECV_REFILL, &cp->cp_flags);
+		wake_up_all(&cp->cp_waitq);
+
 		if (!rds_conn_path_transition(cp, RDS_CONN_DISCONNECTING,
 					      RDS_CONN_DOWN) &&
 		    !rds_conn_path_transition(cp, RDS_CONN_ERROR,

index 8aad185e4b1a..b90e0586f818 100644
@@ -244,8 +244,9 @@ int rds_send_xmit(struct rds_conn_path *cp)
 	WRITE_ONCE(cp->cp_send_gen, send_gen);
 
 	/*
-	 * rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
-	 * we do the opposite to avoid races.
+	 * rds_conn_shutdown() sets the conn state and then acquires
+	 * RDS_IN_XMIT; we take the lock first and then check the state,
+	 * so one of us is guaranteed to see the other's update.
 	 */
 	if (!rds_conn_path_up(cp)) {
 		release_in_xmit(cp);

2.25.1

Any thoughts on the AI review?
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]
IDK enough about RDS to judge the severity of the last one.

BTW we will take these via net now that the 7.2 final was tagged. Don't worry about net-next being closed.

Hi Jakub,

I think the last review in patch 5 isnt as severe as the review makes it sound. The shutdown worker can hold RDS_IN_XMIT for the duration of the TCP drain, which in the worst case can be up to 5 sec across 50 retries.

If a duelling SYN shows up in that time, the accept worker on the global krdsd queue sleeps waiting for the bit. That does mean that things in the same queue have to wait behind it since it's single threaded. It can't deadlock though since the accept worker runs on a different queue than the shutdown worker. Shutdowns are on the individual connection path queues. So the accept workers wait for the shutdown workers, but nothing on the conn path queues wait for anything on the global queue, so eventually the drain always finishes and releases the bit.

There is a low-memory fallback where conn paths share the global queue
instead of allocating their own. That's what they used to do before
commit 4716af3897 gave them their own queue. But that just means all the work items are serialized, so they can't race over the bit.

The waiting algorithm itself actually isn't new. The accept path has waited on this bit from the krdsd work item since 335b48d980f6 back in 2016. This series stretches the worst case from "a sender finishes its batch" to "the teardown finishes its drain". Ideally the best fix would be asynchronous teardown, and there is another Oracle-tree change that coordinates that. It's another porting effort on my todo list, but I try to contain changes in small subsets to keep the reviews manageable since a lot of rds patch tend to snowball.

The other commentary nits from the review do have merit though. If you like, I can send a v3 with those corrected, but I think the code itself is ok. The patch 1 comment about cong.c is a pre-existing issue similar to the lost-wakeup issue that patch 1 fixes. So I can send a follow-up converting those waitqueue_active() calls to wq_has_sleeper().

I hope that helps? Thank you for being accommodating with the branch targeting.

Allison

Thanks, yes! IIUC you don't mind sending a v3 so let's go with that option.

You're welcome, I've sent out an updated v3 and one extra patch for cong.c.

Thanks!
Allison