Re: [PATCH mptcp-next 6/6] mptcp: defer sk_data_ready to the worker

On 7/27/26 1:29 PM, Geliang Tang wrote:
> From: Geliang Tang <[email protected]>
> 
> When MPTCP carries TLS, the data path runs under mptcp_data_lock().
> Reaching sk->sk_data_ready(sk) synchronously ends up at
> tls_strp_check_rcv() -> mptcp_recv_skb() -> mptcp_move_skbs(), which
> calls mptcp_data_lock() on the same sk and recurses on sk_lock.slock.

Is mptcp_move_skbs() really needed in mptcp_recv_skb()? Anyway it looks 
like that is not the only constraint: AFAICS, before the mptcp_recv_skb() 
calls, the TLS code would call __mptcp_read_sock() which in turns calls  
mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() that requires holding 
the msk socket lock in process context, while the mptcp/TLS caller is in 
BH scope.

> Fix this by deferring sk->sk_data_ready(sk) to mptcp_worker() via a
> new MPTCP_WORK_DATA_READY bit, re-using the existing
> mptcp_schedule_work()/mptcp_cancel_work() infrastructure. The wakeup
> bit is consumed after the SOCK_DEAD && TCP_CLOSE destroy branch, so a
> socket that reaches the destroy path drops the pending wakeup rather
> than running it post-free.

I think that unconditionally adding the work latency for non-TLS application 
is a no-go.

Instead I *think* that the constraints in __mptcp_read_sock() could be relaxed
with something alike the following (completely untested):
---
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ca644ec53eed..b09e267f5d46 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2986,6 +2986,15 @@ static void mptcp_do_fastclose(struct sock *sk)
 	}
 }
 
+static void mptcp_read_complete(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+
+	mptcp_cleanup_rbuf(msk, msk->read_copied);
+	mptcp_rcv_space_adjust(msk, msk->read_copied);
+	msk->read_copied = 0;
+}
+
 static void mptcp_worker(struct work_struct *work)
 {
 	struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
@@ -3026,6 +3035,9 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
+	if (test_and_clear_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags))
+		__mptcp_read_complete(sk);
+
 	fail_tout = msk->first ? READ_ONCE(mptcp_subflow_ctx(msk->first)->fail_tout) : 0;
 	if (fail_tout && time_after(jiffies, fail_tout))
 		mptcp_mp_fail_no_response(msk);
@@ -4381,9 +4393,6 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
 	struct sk_buff *skb;
 	u32 offset;
 
-	if (!list_empty(&msk->backlog_list))
-		mptcp_move_skbs(sk);
-
 	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
 		offset = MPTCP_SKB_CB(skb)->offset;
 		if (offset < skb->len) {
@@ -4395,10 +4404,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
 	return NULL;
 }
 
-/*
- * Note:
- *	- It is assumed that the socket was locked by the caller.
- */
+/* Can be invoked in BH scope */
 static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 			     sk_read_actor_t recv_actor, bool noack)
 {
@@ -4441,11 +4447,14 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 	if (noack)
 		goto out;
 
-	mptcp_rcv_space_adjust(msk, copied);
-
+	/* The backlog flushing is only needed when some data is actually
+	 * moved and will take place in the workers's release callback.
+	 */
 	if (copied > 0) {
 		mptcp_recv_skb(sk, &offset);
-		mptcp_cleanup_rbuf(msk, copied);
+		msk->read_copied = copied;
+		set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
+		mptcp_schedule_work(sk);
 	}
 out:
 	return copied;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4a2d40cd7b13..89902a98d383 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -115,6 +115,7 @@
 #define MPTCP_WORK_RTX		1
 #define MPTCP_FALLBACK_DONE	2
 #define MPTCP_WORK_CLOSE_SUBFLOW 3
+#define MPTCP_WORK_READ_COMPLETE 4
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
@@ -305,6 +306,7 @@ struct mptcp_sock {
 	u32		last_data_sent;
 	u32		last_data_recv;
 	u32		last_ack_recv;
+	int		read_copied;
 	unsigned long	timer_ival;
 	u32		token;
 	unsigned long	flags;