[PATCH mptcp-next v4 0/4] mptcp: misc improvement

[PATCH mptcp-next v4 0/4] mptcp: misc improvement

[Paolo Abeni]

This series is a follow-up the discussion ongoing here:

https://github.com/multipath-tcp/mptcp_net-next/issues/437

Daire reported several sub-optimal MPTCP behaviour on high-latency
links.

At least 3 different items stands off:

* notsent_lowat setting fooling the scheduler
* sub-optimal send buffer autotuning
* delayed acks being de-facto disabled due to subflow rcvlowat

This series is an attempt to address the first 2 points above.

v3 -> v4:
 - added patch 1/4, fixing a pre-existing issue related. That patch
   shoud likely go via -net, included here because the issue can not
   manifest itself until patch 4/4.
   Despite by best attempt to test this, I thing it would be easier
   to just lest syzkaller crunch it more than a bit.

v2 -> v3:
 - avoid ingremental updates, always recompute sum(ssk->sndbuf) to avoid
   drift on memory pressure/decrease
 - add some needed comments

v1 -> v2:
 - fixed compiler warning in patch 3/3 && more local testing
 

Paolo Abeni (4):
  mptcp: fix delegated action races.
  mptcp: consolidate sockopt synchronization
  mptcp: ignore notsent_lowat setting at the subflow level.
  mptcp: refactor sndbuf auto-tuning.

 net/mptcp/protocol.c | 48 +++++++++++++++---------
 net/mptcp/protocol.h | 89 ++++++++++++++++++++++++++++++--------------
 net/mptcp/sockopt.c  | 28 +++-----------
 net/mptcp/subflow.c  | 27 +++++++-------
 4 files changed, 111 insertions(+), 81 deletions(-)

-- 
2.41.0

[PATCH mptcp-next v4 1/4] mptcp: fix delegated action races.

[Paolo Abeni]

The delegated action infrastructure is prone to the following
race: different CPUs can try to schedule different delegated
actions on the same subflow at the same time.

Each of them will check different bits via mptcp_subflow_delegate(),
and will try to schedule the action on the related per-cpu napi
instance.

Depending on the timing, both can observe an empty delegated list
node, causing the same entry to be added simultaneously on two different
lists.

The root cause is that the delegated actions infra does not provide
a single synchronization point. Address the issue reserving an additional
bit to mark the subflow as scheduled for delegation. Acquiring such bit
guarantee the caller to own the delegated list node, and being able to
safely schedule the subflow.

Clear such bit only when the subflow scheduling is completed, ensuring
proper barrier in place.

Additionally swap the meaning of the delegated_action bitmask, to allow
the usage of the existing helper to set multiple bit at once.

Fixes: bcd97734318d ("mptcp: use delegate action to schedule 3rd ack retrans")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 28 ++++++++++++++--------------
 net/mptcp/protocol.h | 35 ++++++++++++-----------------------
 net/mptcp/subflow.c  |  6 ++++--
 3 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1a0b463f8c97..04eda1b8f7a4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3425,24 +3425,21 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
 	sk_reset_timer(ssk, &icsk->icsk_delack_timer, timeout);
 }
 
-void mptcp_subflow_process_delegated(struct sock *ssk)
+void mptcp_subflow_process_delegated(struct sock *ssk, long status)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	struct sock *sk = subflow->conn;
 
-	if (test_bit(MPTCP_DELEGATE_SEND, &subflow->delegated_status)) {
+	if (status & BIT(MPTCP_DELEGATE_SEND)) {
 		mptcp_data_lock(sk);
 		if (!sock_owned_by_user(sk))
 			__mptcp_subflow_push_pending(sk, ssk, true);
 		else
 			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
 		mptcp_data_unlock(sk);
-		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_SEND);
 	}
-	if (test_bit(MPTCP_DELEGATE_ACK, &subflow->delegated_status)) {
+	if (status & BIT(MPTCP_DELEGATE_ACK))
 		schedule_3rdack_retransmission(ssk);
-		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_ACK);
-	}
 }
 
 static int mptcp_hash(struct sock *sk)
@@ -3968,14 +3965,17 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
 		bh_lock_sock_nested(ssk);
-		if (!sock_owned_by_user(ssk) &&
-		    mptcp_subflow_has_delegated_action(subflow))
-			mptcp_subflow_process_delegated(ssk);
-		/* ... elsewhere tcp_release_cb_override already processed
-		 * the action or will do at next release_sock().
-		 * In both case must dequeue the subflow here - on the same
-		 * CPU that scheduled it.
-		 */
+		if (!sock_owned_by_user(ssk)) {
+			mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
+		} else {
+			/* tcp_release_cb_override already processed
+			 * the action or will do at next release_sock().
+			 * In both case must dequeue the subflow here - on the same
+			 * CPU that scheduled it.
+			 */
+			smp_wmb();
+			clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
+		}
 		bh_unlock_sock(ssk);
 		sock_put(ssk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3c938e3560e4..7c7ad087d8ac 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -444,9 +444,11 @@ struct mptcp_delegated_action {
 
 DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
 
-#define MPTCP_DELEGATE_SEND		0
-#define MPTCP_DELEGATE_ACK		1
+#define MPTCP_DELEGATE_SCHEDULED	0
+#define MPTCP_DELEGATE_SEND		1
+#define MPTCP_DELEGATE_ACK		2
 
+#define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
 /* MPTCP subflow context */
 struct mptcp_subflow_context {
 	struct	list_head node;/* conn_list of subflows */
@@ -564,23 +566,24 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
 	return subflow->map_seq + mptcp_subflow_get_map_offset(subflow);
 }
 
-void mptcp_subflow_process_delegated(struct sock *ssk);
+void mptcp_subflow_process_delegated(struct sock *ssk, long actions);
 
 static inline void mptcp_subflow_delegate(struct mptcp_subflow_context *subflow, int action)
 {
+	long old, mask = BIT(MPTCP_DELEGATE_SCHEDULED) | BIT(action);
 	struct mptcp_delegated_action *delegated;
 	bool schedule;
 
 	/* the caller held the subflow bh socket lock */
 	lockdep_assert_in_softirq();
 
-	/* The implied barrier pairs with mptcp_subflow_delegated_done(), and
-	 * ensures the below list check sees list updates done prior to status
-	 * bit changes
+	/* The implied barrier pairs with tcp_release_cb_override()
+	 * mptcp_napi_poll(), and ensures the below list check sees list
+	 * updates done prior to delegated status bits changes
 	 */
-	if (!test_and_set_bit(action, &subflow->delegated_status)) {
-		/* still on delegated list from previous scheduling */
-		if (!list_empty(&subflow->delegated_node))
+	old = set_mask_bits(&subflow->delegated_status, 0, mask);
+	if (!(old & BIT(MPTCP_DELEGATE_SCHEDULED))) {
+		if (WARN_ON_ONCE(!list_empty(&subflow->delegated_node)))
 			return;
 
 		delegated = this_cpu_ptr(&mptcp_delegated_actions);
@@ -605,20 +608,6 @@ mptcp_subflow_delegated_next(struct mptcp_delegated_action *delegated)
 	return ret;
 }
 
-static inline bool mptcp_subflow_has_delegated_action(const struct mptcp_subflow_context *subflow)
-{
-	return !!READ_ONCE(subflow->delegated_status);
-}
-
-static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *subflow, int action)
-{
-	/* pairs with mptcp_subflow_delegate, ensures delegate_node is updated before
-	 * touching the status bit
-	 */
-	smp_wmb();
-	clear_bit(action, &subflow->delegated_status);
-}
-
 int mptcp_is_enabled(const struct net *net);
 unsigned int mptcp_get_add_addr_timeout(const struct net *net);
 int mptcp_is_checksum_enabled(const struct net *net);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 918c1a235790..0d860cb730b4 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1956,9 +1956,11 @@ static void subflow_ulp_clone(const struct request_sock *req,
 static void tcp_release_cb_override(struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+	long status;
 
-	if (mptcp_subflow_has_delegated_action(subflow))
-		mptcp_subflow_process_delegated(ssk);
+	status = set_mask_bits(&subflow->delegated_status, MPTCP_DELEGATE_ACTIONS_MASK, 0);
+	if (status)
+		mptcp_subflow_process_delegated(ssk, status);
 
 	tcp_release_cb(ssk);
 }
-- 
2.41.0

[PATCH mptcp-next v4 2/4] mptcp: consolidate sockopt synchronization

[Paolo Abeni]

Move the socket option synchronization for active subflows
at subflow creation time. This allows removing the now unused
unlocked variant of such helper.

While at that, clean-up a bit the mptcp_subflow_create_socket()
errors path.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c |  2 --
 net/mptcp/sockopt.c  | 22 ----------------------
 net/mptcp/subflow.c  | 18 +++++++++---------
 3 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 04eda1b8f7a4..f727a7ee662d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -121,8 +121,6 @@ struct sock *__mptcp_nmpc_sk(struct mptcp_sock *msk)
 		ret = __mptcp_socket_create(msk);
 		if (ret)
 			return ERR_PTR(ret);
-
-		mptcp_sockopt_sync(msk, msk->first);
 	}
 
 	return msk->first;
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 8260202c0066..f44b364b0055 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1444,28 +1444,6 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
 	inet_assign_bit(FREEBIND, ssk, inet_test_bit(FREEBIND, sk));
 }
 
-static void __mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk)
-{
-	bool slow = lock_sock_fast(ssk);
-
-	sync_socket_options(msk, ssk);
-
-	unlock_sock_fast(ssk, slow);
-}
-
-void mptcp_sockopt_sync(struct mptcp_sock *msk, struct sock *ssk)
-{
-	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
-
-	msk_owned_by_me(msk);
-
-	if (READ_ONCE(subflow->setsockopt_seq) != msk->setsockopt_seq) {
-		__mptcp_sockopt_sync(msk, ssk);
-
-		subflow->setsockopt_seq = msk->setsockopt_seq;
-	}
-}
-
 void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 0d860cb730b4..6102e74121c3 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1525,8 +1525,6 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	if (addr.ss_family == AF_INET6)
 		addrlen = sizeof(struct sockaddr_in6);
 #endif
-	mptcp_sockopt_sync(msk, ssk);
-
 	ssk->sk_bound_dev_if = ifindex;
 	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
 	if (err)
@@ -1637,7 +1635,7 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 
 	err = security_mptcp_add_subflow(sk, sf->sk);
 	if (err)
-		goto release_ssk;
+		goto err_free;
 
 	/* the newly created socket has to be in the same cgroup as its parent */
 	mptcp_attach_cgroup(sk, sf->sk);
@@ -1651,15 +1649,12 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 	get_net_track(net, &sf->sk->ns_tracker, GFP_KERNEL);
 	sock_inuse_add(net, 1);
 	err = tcp_set_ulp(sf->sk, "mptcp");
+	if (err)
+		goto err_free;
 
-release_ssk:
+	mptcp_sockopt_sync_locked(mptcp_sk(sk), sf->sk);
 	release_sock(sf->sk);
 
-	if (err) {
-		sock_release(sf);
-		return err;
-	}
-
 	/* the newly created socket really belongs to the owning MPTCP master
 	 * socket, even if for additional subflows the allocation is performed
 	 * by a kernel workqueue. Adjust inode references, so that the
@@ -1679,6 +1674,11 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 	mptcp_subflow_ops_override(sf->sk);
 
 	return 0;
+
+err_free:
+	release_sock(sf->sk);
+	sock_release(sf);
+	return err;
 }
 
 static struct mptcp_subflow_context *subflow_create_ctx(struct sock *sk,
-- 
2.41.0

[PATCH mptcp-next v4 3/4] mptcp: ignore notsent_lowat setting at the subflow level.

[Paolo Abeni]

Any latency related tuning taking action at the subflow level does
not really affect the user-space, as only the main MPTCP socket is
relevant.

Anyway any limiting setting may foul the MPTCP scheduler, not being
able to fully use the subflow-level cwin, leading to very poor b/w
usage.

Enforce notsent_lowat to be a no-op on every subflow.

Note that TCP_NOTSENT_LOWAT is currently not supported, and properly
dealing with that will require more invasive changes.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/sockopt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index f44b364b0055..453d6c78c25c 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1450,6 +1450,12 @@ void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)
 
 	msk_owned_by_me(msk);
 
+	/* subflows must ignore any latency-related settings: will not affect
+	 * the user-space - only the msk is relevant - but will foul the
+	 * mptcp scheduler
+	 */
+	tcp_sk(ssk)->notsent_lowat = UINT_MAX;
+
 	if (READ_ONCE(subflow->setsockopt_seq) != msk->setsockopt_seq) {
 		sync_socket_options(msk, ssk);
 
-- 
2.41.0

[PATCH mptcp-next v4 4/4] mptcp: refactor sndbuf auto-tuning.

[Paolo Abeni]

The MPTCP protocol account for the data enqueued on all the subflows
to the main socket send buffer, while the send buffer auto-tuning
algorithm set the main socket send buffer size as the max size among
the subflows.

That causes bad performances when at least one subflow is sndbuf
limited, e.g. due to very high latency, as the MPTCP scheduler can't
even fill such buffer.

Change the send-buffer auto-tuning algorithm to compute the main socket
send buffer size as the sum of all the subflows buffer size.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v2 -> v3:
 - avoid ingremental updates, always recompute sum(ssk->sndbuf) to avoid
   drift on memory pressure/decrease
---
 net/mptcp/protocol.c | 18 +++++++++++++--
 net/mptcp/protocol.h | 54 ++++++++++++++++++++++++++++++++++++++++----
 net/mptcp/subflow.c  |  3 +--
 3 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f727a7ee662d..0a9d00e794d4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -891,6 +891,7 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
 	mptcp_sockopt_sync_locked(msk, ssk);
 	mptcp_subflow_joined(msk, ssk);
 	mptcp_stop_tout_timer(sk);
+	__mptcp_propagate_sndbuf(sk, ssk);
 	return true;
 }
 
@@ -1077,15 +1078,16 @@ static void mptcp_enter_memory_pressure(struct sock *sk)
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	bool first = true;
 
-	sk_stream_moderate_sndbuf(sk);
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
 		if (first)
 			tcp_enter_memory_pressure(ssk);
 		sk_stream_moderate_sndbuf(ssk);
+
 		first = false;
 	}
+	__mptcp_sync_sndbuf(sk);
 }
 
 /* ensure we get enough memory for the frag hdr, beyond some minimal amount of
@@ -2436,6 +2438,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		WRITE_ONCE(msk->first, NULL);
 
 out:
+	__mptcp_sync_sndbuf(sk);
 	if (need_push)
 		__mptcp_push_pending(sk, 0);
 
@@ -3214,7 +3217,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 	 * uses the correct data
 	 */
 	mptcp_copy_inaddrs(nsk, ssk);
-	mptcp_propagate_sndbuf(nsk, ssk);
+	__mptcp_propagate_sndbuf(nsk, ssk);
 
 	mptcp_rcv_space_init(msk, ssk);
 	bh_unlock_sock(nsk);
@@ -3392,6 +3395,8 @@ static void mptcp_release_cb(struct sock *sk)
 			__mptcp_set_connected(sk);
 		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
 			__mptcp_error_report(sk);
+		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
+			__mptcp_sync_sndbuf(sk);
 	}
 
 	__mptcp_update_rmem(sk);
@@ -3436,6 +3441,14 @@ void mptcp_subflow_process_delegated(struct sock *ssk, long status)
 			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
 		mptcp_data_unlock(sk);
 	}
+	if (status & BIT(MPTCP_DELEGATE_SNDBUF)) {
+		mptcp_data_lock(sk);
+		if (!sock_owned_by_user(sk))
+			__mptcp_sync_sndbuf(sk);
+		else
+			__set_bit(MPTCP_SYNC_SNDBUF, &mptcp_sk(sk)->cb_flags);
+		mptcp_data_unlock(sk);
+	}
 	if (status & BIT(MPTCP_DELEGATE_ACK))
 		schedule_3rdack_retransmission(ssk);
 }
@@ -3520,6 +3533,7 @@ bool mptcp_finish_join(struct sock *ssk)
 	/* active subflow, already present inside the conn_list */
 	if (!list_empty(&subflow->node)) {
 		mptcp_subflow_joined(msk, ssk);
+		mptcp_propagate_sndbuf(parent, ssk);
 		return true;
 	}
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7c7ad087d8ac..ab775e48c11d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -123,6 +123,7 @@
 #define MPTCP_RETRANSMIT	4
 #define MPTCP_FLUSH_JOIN_LIST	5
 #define MPTCP_CONNECTED		6
+#define MPTCP_SYNC_SNDBUF	7
 
 struct mptcp_skb_cb {
 	u64 map_seq;
@@ -447,6 +448,7 @@ DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
 #define MPTCP_DELEGATE_SCHEDULED	0
 #define MPTCP_DELEGATE_SEND		1
 #define MPTCP_DELEGATE_ACK		2
+#define MPTCP_DELEGATE_SNDBUF		3
 
 #define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
 /* MPTCP subflow context */
@@ -520,6 +522,9 @@ struct mptcp_subflow_context {
 
 	u32	setsockopt_seq;
 	u32	stale_rcv_tstamp;
+	int     cached_sndbuf;	    /* sndbuf size when last synced with the msk sndbuf,
+				     * protected by the msk socket lock
+				     */
 
 	struct	sock *tcp_sock;	    /* tcp sk backpointer */
 	struct	sock *conn;	    /* parent mptcp_sock */
@@ -768,13 +773,52 @@ static inline bool mptcp_data_fin_enabled(const struct mptcp_sock *msk)
 	       READ_ONCE(msk->write_seq) == READ_ONCE(msk->snd_nxt);
 }
 
-static inline bool mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
+static inline void __mptcp_sync_sndbuf(struct sock *sk)
 {
-	if ((sk->sk_userlocks & SOCK_SNDBUF_LOCK) || ssk->sk_sndbuf <= READ_ONCE(sk->sk_sndbuf))
-		return false;
+	struct mptcp_subflow_context *subflow;
+	int ssk_sndbuf, new_sndbuf;
+
+	if (sk->sk_userlocks & SOCK_SNDBUF_LOCK)
+		return;
+
+	new_sndbuf = sock_net(sk)->ipv4.sysctl_tcp_wmem[0];
+	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
+		ssk_sndbuf =  READ_ONCE(mptcp_subflow_tcp_sock(subflow)->sk_sndbuf);
+
+		subflow->cached_sndbuf = ssk_sndbuf;
+		new_sndbuf += ssk_sndbuf;
+	}
+
+	/* the msk max wmem limit is <nr_subflows> * tcp wmem[2] */
+	WRITE_ONCE(sk->sk_sndbuf, new_sndbuf);
+}
+
+/* The called held both the msk socket and the subflow socket locks,
+ * possibly under BH
+ */
+static inline void __mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+
+	if (READ_ONCE(ssk->sk_sndbuf) - subflow->cached_sndbuf)
+		__mptcp_sync_sndbuf(sk);
+}
+
+/* the caller held only the subflow socket lock, either in process or
+ * BH context. Additionally this can be called under the msk data lock,
+ * so we can't acquire such lock here: let the delegate action acquires
+ * the needed locks in suitable order.
+ */
+static inline void mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+
+	if (likely(READ_ONCE(ssk->sk_sndbuf) == subflow->cached_sndbuf))
+		return;
 
-	WRITE_ONCE(sk->sk_sndbuf, ssk->sk_sndbuf);
-	return true;
+	local_bh_disable();
+	mptcp_subflow_delegate(subflow, MPTCP_DELEGATE_SNDBUF);
+	local_bh_enable();
 }
 
 static inline void mptcp_write_space(struct sock *sk)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6102e74121c3..0eae952064b1 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -421,6 +421,7 @@ static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct soc
 
 void __mptcp_set_connected(struct sock *sk)
 {
+	__mptcp_propagate_sndbuf(sk, mptcp_sk(sk)->first);
 	if (sk->sk_state == TCP_SYN_SENT) {
 		inet_sk_state_store(sk, TCP_ESTABLISHED);
 		sk->sk_state_change(sk);
@@ -472,7 +473,6 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 		return;
 
 	msk = mptcp_sk(parent);
-	mptcp_propagate_sndbuf(parent, sk);
 	subflow->rel_write_seq = 1;
 	subflow->conn_finished = 1;
 	subflow->ssn_offset = TCP_SKB_CB(skb)->seq;
@@ -1728,7 +1728,6 @@ static void subflow_state_change(struct sock *sk)
 
 	msk = mptcp_sk(parent);
 	if (subflow_simultaneous_connect(sk)) {
-		mptcp_propagate_sndbuf(parent, sk);
 		mptcp_do_fallback(sk);
 		mptcp_rcv_space_init(msk, sk);
 		pr_fallback(msk);
-- 
2.41.0

Re: mptcp: refactor sndbuf auto-tuning.: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6640879297888256
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6640879297888256/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5726085623578624
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5726085623578624/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5163135670157312
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5163135670157312/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4600185716736000
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4600185716736000/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/96ce451d7205


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next v4 1/4] mptcp: fix delegated action races.

[Paolo Abeni]

On Wed, 2023-09-20 at 11:09 +0200, Paolo Abeni wrote:
> The delegated action infrastructure is prone to the following
> race: different CPUs can try to schedule different delegated
> actions on the same subflow at the same time.
> 
> Each of them will check different bits via mptcp_subflow_delegate(),
> and will try to schedule the action on the related per-cpu napi
> instance.
> 
> Depending on the timing, both can observe an empty delegated list
> node, causing the same entry to be added simultaneously on two different
> lists.
> 
> The root cause is that the delegated actions infra does not provide
> a single synchronization point. Address the issue reserving an additional
> bit to mark the subflow as scheduled for delegation. Acquiring such bit
> guarantee the caller to own the delegated list node, and being able to
> safely schedule the subflow.
> 
> Clear such bit only when the subflow scheduling is completed, ensuring
> proper barrier in place.
> 
> Additionally swap the meaning of the delegated_action bitmask, to allow
> the usage of the existing helper to set multiple bit at once.

I just noted this last paragraph is obsolete - it refers to a previous,
never shared, revision of this patch.

Should be dropped at merge time.

> 
> Fixes: bcd97734318d ("mptcp: use delegate action to schedule 3rd ack retrans")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 28 ++++++++++++++--------------
>  net/mptcp/protocol.h | 35 ++++++++++++-----------------------
>  net/mptcp/subflow.c  |  6 ++++--
>  3 files changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 1a0b463f8c97..04eda1b8f7a4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3425,24 +3425,21 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
>  	sk_reset_timer(ssk, &icsk->icsk_delack_timer, timeout);
>  }
>  
> -void mptcp_subflow_process_delegated(struct sock *ssk)
> +void mptcp_subflow_process_delegated(struct sock *ssk, long status)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	struct sock *sk = subflow->conn;
>  
> -	if (test_bit(MPTCP_DELEGATE_SEND, &subflow->delegated_status)) {
> +	if (status & BIT(MPTCP_DELEGATE_SEND)) {
>  		mptcp_data_lock(sk);
>  		if (!sock_owned_by_user(sk))
>  			__mptcp_subflow_push_pending(sk, ssk, true);
>  		else
>  			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
>  		mptcp_data_unlock(sk);
> -		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_SEND);
>  	}
> -	if (test_bit(MPTCP_DELEGATE_ACK, &subflow->delegated_status)) {
> +	if (status & BIT(MPTCP_DELEGATE_ACK))
>  		schedule_3rdack_retransmission(ssk);
> -		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_ACK);
> -	}
>  }
>  
>  static int mptcp_hash(struct sock *sk)
> @@ -3968,14 +3965,17 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>  
>  		bh_lock_sock_nested(ssk);
> -		if (!sock_owned_by_user(ssk) &&
> -		    mptcp_subflow_has_delegated_action(subflow))
> -			mptcp_subflow_process_delegated(ssk);
> -		/* ... elsewhere tcp_release_cb_override already processed
> -		 * the action or will do at next release_sock().
> -		 * In both case must dequeue the subflow here - on the same
> -		 * CPU that scheduled it.
> -		 */
> +		if (!sock_owned_by_user(ssk)) {
> +			mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
> +		} else {
> +			/* tcp_release_cb_override already processed
> +			 * the action or will do at next release_sock().
> +			 * In both case must dequeue the subflow here - on the same
> +			 * CPU that scheduled it.
> +			 */
> +			smp_wmb();
> +			clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
> +		}
>  		bh_unlock_sock(ssk);
>  		sock_put(ssk);
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3c938e3560e4..7c7ad087d8ac 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -444,9 +444,11 @@ struct mptcp_delegated_action {
>  
>  DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
>  
> -#define MPTCP_DELEGATE_SEND		0
> -#define MPTCP_DELEGATE_ACK		1
> +#define MPTCP_DELEGATE_SCHEDULED	0
> +#define MPTCP_DELEGATE_SEND		1
> +#define MPTCP_DELEGATE_ACK		2
>  
> +#define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
>  /* MPTCP subflow context */
>  struct mptcp_subflow_context {
>  	struct	list_head node;/* conn_list of subflows */
> @@ -564,23 +566,24 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
>  	return subflow->map_seq + mptcp_subflow_get_map_offset(subflow);
>  }
>  
> -void mptcp_subflow_process_delegated(struct sock *ssk);
> +void mptcp_subflow_process_delegated(struct sock *ssk, long actions);
>  
>  static inline void mptcp_subflow_delegate(struct mptcp_subflow_context *subflow, int action)
>  {
> +	long old, mask = BIT(MPTCP_DELEGATE_SCHEDULED) | BIT(action);
>  	struct mptcp_delegated_action *delegated;
>  	bool schedule;
>  
>  	/* the caller held the subflow bh socket lock */
>  	lockdep_assert_in_softirq();
>  
> -	/* The implied barrier pairs with mptcp_subflow_delegated_done(), and
> -	 * ensures the below list check sees list updates done prior to status
> -	 * bit changes
> +	/* The implied barrier pairs with tcp_release_cb_override()
> +	 * mptcp_napi_poll(), and ensures the below list check sees list
> +	 * updates done prior to delegated status bits changes
>  	 */
> -	if (!test_and_set_bit(action, &subflow->delegated_status)) {
> -		/* still on delegated list from previous scheduling */
> -		if (!list_empty(&subflow->delegated_node))
> +	old = set_mask_bits(&subflow->delegated_status, 0, mask);
> +	if (!(old & BIT(MPTCP_DELEGATE_SCHEDULED))) {
> +		if (WARN_ON_ONCE(!list_empty(&subflow->delegated_node)))
>  			return;
>  
>  		delegated = this_cpu_ptr(&mptcp_delegated_actions);
> @@ -605,20 +608,6 @@ mptcp_subflow_delegated_next(struct mptcp_delegated_action *delegated)
>  	return ret;
>  }
>  
> -static inline bool mptcp_subflow_has_delegated_action(const struct mptcp_subflow_context *subflow)
> -{
> -	return !!READ_ONCE(subflow->delegated_status);
> -}
> -
> -static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *subflow, int action)
> -{
> -	/* pairs with mptcp_subflow_delegate, ensures delegate_node is updated before
> -	 * touching the status bit
> -	 */
> -	smp_wmb();
> -	clear_bit(action, &subflow->delegated_status);
> -}
> -
>  int mptcp_is_enabled(const struct net *net);
>  unsigned int mptcp_get_add_addr_timeout(const struct net *net);
>  int mptcp_is_checksum_enabled(const struct net *net);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 918c1a235790..0d860cb730b4 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1956,9 +1956,11 @@ static void subflow_ulp_clone(const struct request_sock *req,
>  static void tcp_release_cb_override(struct sock *ssk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	long status;
>  
> -	if (mptcp_subflow_has_delegated_action(subflow))
> -		mptcp_subflow_process_delegated(ssk);
> +	status = set_mask_bits(&subflow->delegated_status, MPTCP_DELEGATE_ACTIONS_MASK, 0);
> +	if (status)
> +		mptcp_subflow_process_delegated(ssk, status);
>  
>  	tcp_release_cb(ssk);
>  }

Re: [PATCH mptcp-next v4 1/4] mptcp: fix delegated action races.

[Mat Martineau]

On Wed, 20 Sep 2023, Paolo Abeni wrote:

> The delegated action infrastructure is prone to the following
> race: different CPUs can try to schedule different delegated
> actions on the same subflow at the same time.
>
> Each of them will check different bits via mptcp_subflow_delegate(),
> and will try to schedule the action on the related per-cpu napi
> instance.
>
> Depending on the timing, both can observe an empty delegated list
> node, causing the same entry to be added simultaneously on two different
> lists.
>
> The root cause is that the delegated actions infra does not provide
> a single synchronization point. Address the issue reserving an additional
> bit to mark the subflow as scheduled for delegation. Acquiring such bit
> guarantee the caller to own the delegated list node, and being able to
> safely schedule the subflow.
>
> Clear such bit only when the subflow scheduling is completed, ensuring
> proper barrier in place.
>
> Additionally swap the meaning of the delegated_action bitmask, to allow
> the usage of the existing helper to set multiple bit at once.
>
> Fixes: bcd97734318d ("mptcp: use delegate action to schedule 3rd ack retrans")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 28 ++++++++++++++--------------
> net/mptcp/protocol.h | 35 ++++++++++++-----------------------
> net/mptcp/subflow.c  |  6 ++++--
> 3 files changed, 30 insertions(+), 39 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 1a0b463f8c97..04eda1b8f7a4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3425,24 +3425,21 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
> 	sk_reset_timer(ssk, &icsk->icsk_delack_timer, timeout);
> }
>
> -void mptcp_subflow_process_delegated(struct sock *ssk)
> +void mptcp_subflow_process_delegated(struct sock *ssk, long status)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> 	struct sock *sk = subflow->conn;
>
> -	if (test_bit(MPTCP_DELEGATE_SEND, &subflow->delegated_status)) {
> +	if (status & BIT(MPTCP_DELEGATE_SEND)) {
> 		mptcp_data_lock(sk);
> 		if (!sock_owned_by_user(sk))
> 			__mptcp_subflow_push_pending(sk, ssk, true);
> 		else
> 			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
> 		mptcp_data_unlock(sk);
> -		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_SEND);
> 	}
> -	if (test_bit(MPTCP_DELEGATE_ACK, &subflow->delegated_status)) {
> +	if (status & BIT(MPTCP_DELEGATE_ACK))
> 		schedule_3rdack_retransmission(ssk);
> -		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_ACK);
> -	}
> }
>
> static int mptcp_hash(struct sock *sk)
> @@ -3968,14 +3965,17 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
> 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>
> 		bh_lock_sock_nested(ssk);
> -		if (!sock_owned_by_user(ssk) &&
> -		    mptcp_subflow_has_delegated_action(subflow))
> -			mptcp_subflow_process_delegated(ssk);
> -		/* ... elsewhere tcp_release_cb_override already processed
> -		 * the action or will do at next release_sock().
> -		 * In both case must dequeue the subflow here - on the same
> -		 * CPU that scheduled it.
> -		 */
> +		if (!sock_owned_by_user(ssk)) {
> +			mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
> +		} else {
> +			/* tcp_release_cb_override already processed
> +			 * the action or will do at next release_sock().
> +			 * In both case must dequeue the subflow here - on the same
> +			 * CPU that scheduled it.
> +			 */
> +			smp_wmb();
> +			clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
> +		}
> 		bh_unlock_sock(ssk);
> 		sock_put(ssk);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3c938e3560e4..7c7ad087d8ac 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -444,9 +444,11 @@ struct mptcp_delegated_action {
>
> DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
>
> -#define MPTCP_DELEGATE_SEND		0
> -#define MPTCP_DELEGATE_ACK		1
> +#define MPTCP_DELEGATE_SCHEDULED	0
> +#define MPTCP_DELEGATE_SEND		1
> +#define MPTCP_DELEGATE_ACK		2
>
> +#define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
> /* MPTCP subflow context */
> struct mptcp_subflow_context {
> 	struct	list_head node;/* conn_list of subflows */
> @@ -564,23 +566,24 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
> 	return subflow->map_seq + mptcp_subflow_get_map_offset(subflow);
> }
>
> -void mptcp_subflow_process_delegated(struct sock *ssk);
> +void mptcp_subflow_process_delegated(struct sock *ssk, long actions);
>
> static inline void mptcp_subflow_delegate(struct mptcp_subflow_context *subflow, int action)
> {
> +	long old, mask = BIT(MPTCP_DELEGATE_SCHEDULED) | BIT(action);
> 	struct mptcp_delegated_action *delegated;
> 	bool schedule;
>
> 	/* the caller held the subflow bh socket lock */
> 	lockdep_assert_in_softirq();
>
> -	/* The implied barrier pairs with mptcp_subflow_delegated_done(), and
> -	 * ensures the below list check sees list updates done prior to status
> -	 * bit changes
> +	/* The implied barrier pairs with tcp_release_cb_override()
> +	 * mptcp_napi_poll(), and ensures the below list check sees list
> +	 * updates done prior to delegated status bits changes
> 	 */
> -	if (!test_and_set_bit(action, &subflow->delegated_status)) {
> -		/* still on delegated list from previous scheduling */
> -		if (!list_empty(&subflow->delegated_node))
> +	old = set_mask_bits(&subflow->delegated_status, 0, mask);

Hi Paolo -

The naming of the 'mask' variable here made it a little harder to 
understand at first glance, as the middle arg of the set_mask_bits macro 
is the one that does the masking (clearing) of bits. How about 'set_bits' 
(or similar) instead.

> +	if (!(old & BIT(MPTCP_DELEGATE_SCHEDULED))) {
> +		if (WARN_ON_ONCE(!list_empty(&subflow->delegated_node)))
> 			return;
>
> 		delegated = this_cpu_ptr(&mptcp_delegated_actions);
> @@ -605,20 +608,6 @@ mptcp_subflow_delegated_next(struct mptcp_delegated_action *delegated)
> 	return ret;
> }
>
> -static inline bool mptcp_subflow_has_delegated_action(const struct mptcp_subflow_context *subflow)
> -{
> -	return !!READ_ONCE(subflow->delegated_status);
> -}
> -
> -static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *subflow, int action)
> -{
> -	/* pairs with mptcp_subflow_delegate, ensures delegate_node is updated before
> -	 * touching the status bit
> -	 */
> -	smp_wmb();
> -	clear_bit(action, &subflow->delegated_status);
> -}
> -
> int mptcp_is_enabled(const struct net *net);
> unsigned int mptcp_get_add_addr_timeout(const struct net *net);
> int mptcp_is_checksum_enabled(const struct net *net);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 918c1a235790..0d860cb730b4 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1956,9 +1956,11 @@ static void subflow_ulp_clone(const struct request_sock *req,
> static void tcp_release_cb_override(struct sock *ssk)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	long status;
>
> -	if (mptcp_subflow_has_delegated_action(subflow))
> -		mptcp_subflow_process_delegated(ssk);
> +	status = set_mask_bits(&subflow->delegated_status, MPTCP_DELEGATE_ACTIONS_MASK, 0);

Can you add a comment here regarding the interaction with 
mptcp_napi_poll()?

Thanks,

Mat

Re: [PATCH mptcp-next v4 4/4] mptcp: refactor sndbuf auto-tuning.

[Mat Martineau]

On Wed, 20 Sep 2023, Paolo Abeni wrote:

> The MPTCP protocol account for the data enqueued on all the subflows
> to the main socket send buffer, while the send buffer auto-tuning
> algorithm set the main socket send buffer size as the max size among
> the subflows.
>
> That causes bad performances when at least one subflow is sndbuf
> limited, e.g. due to very high latency, as the MPTCP scheduler can't
> even fill such buffer.
>
> Change the send-buffer auto-tuning algorithm to compute the main socket
> send buffer size as the sum of all the subflows buffer size.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v2 -> v3:
> - avoid ingremental updates, always recompute sum(ssk->sndbuf) to avoid
>   drift on memory pressure/decrease
> ---
> net/mptcp/protocol.c | 18 +++++++++++++--
> net/mptcp/protocol.h | 54 ++++++++++++++++++++++++++++++++++++++++----
> net/mptcp/subflow.c  |  3 +--
> 3 files changed, 66 insertions(+), 9 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f727a7ee662d..0a9d00e794d4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -891,6 +891,7 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
> 	mptcp_sockopt_sync_locked(msk, ssk);
> 	mptcp_subflow_joined(msk, ssk);
> 	mptcp_stop_tout_timer(sk);
> +	__mptcp_propagate_sndbuf(sk, ssk);
> 	return true;
> }
>
> @@ -1077,15 +1078,16 @@ static void mptcp_enter_memory_pressure(struct sock *sk)
> 	struct mptcp_sock *msk = mptcp_sk(sk);
> 	bool first = true;
>
> -	sk_stream_moderate_sndbuf(sk);
> 	mptcp_for_each_subflow(msk, subflow) {
> 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>
> 		if (first)
> 			tcp_enter_memory_pressure(ssk);
> 		sk_stream_moderate_sndbuf(ssk);
> +
> 		first = false;
> 	}
> +	__mptcp_sync_sndbuf(sk);
> }
>
> /* ensure we get enough memory for the frag hdr, beyond some minimal amount of
> @@ -2436,6 +2438,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> 		WRITE_ONCE(msk->first, NULL);
>
> out:
> +	__mptcp_sync_sndbuf(sk);
> 	if (need_push)
> 		__mptcp_push_pending(sk, 0);
>
> @@ -3214,7 +3217,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
> 	 * uses the correct data
> 	 */
> 	mptcp_copy_inaddrs(nsk, ssk);
> -	mptcp_propagate_sndbuf(nsk, ssk);
> +	__mptcp_propagate_sndbuf(nsk, ssk);
>
> 	mptcp_rcv_space_init(msk, ssk);
> 	bh_unlock_sock(nsk);
> @@ -3392,6 +3395,8 @@ static void mptcp_release_cb(struct sock *sk)
> 			__mptcp_set_connected(sk);
> 		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
> 			__mptcp_error_report(sk);
> +		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
> +			__mptcp_sync_sndbuf(sk);
> 	}
>
> 	__mptcp_update_rmem(sk);
> @@ -3436,6 +3441,14 @@ void mptcp_subflow_process_delegated(struct sock *ssk, long status)
> 			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
> 		mptcp_data_unlock(sk);
> 	}
> +	if (status & BIT(MPTCP_DELEGATE_SNDBUF)) {
> +		mptcp_data_lock(sk);
> +		if (!sock_owned_by_user(sk))
> +			__mptcp_sync_sndbuf(sk);
> +		else
> +			__set_bit(MPTCP_SYNC_SNDBUF, &mptcp_sk(sk)->cb_flags);
> +		mptcp_data_unlock(sk);
> +	}
> 	if (status & BIT(MPTCP_DELEGATE_ACK))
> 		schedule_3rdack_retransmission(ssk);
> }
> @@ -3520,6 +3533,7 @@ bool mptcp_finish_join(struct sock *ssk)
> 	/* active subflow, already present inside the conn_list */
> 	if (!list_empty(&subflow->node)) {
> 		mptcp_subflow_joined(msk, ssk);
> +		mptcp_propagate_sndbuf(parent, ssk);
> 		return true;
> 	}
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7c7ad087d8ac..ab775e48c11d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -123,6 +123,7 @@
> #define MPTCP_RETRANSMIT	4
> #define MPTCP_FLUSH_JOIN_LIST	5
> #define MPTCP_CONNECTED		6
> +#define MPTCP_SYNC_SNDBUF	7
>
> struct mptcp_skb_cb {
> 	u64 map_seq;
> @@ -447,6 +448,7 @@ DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
> #define MPTCP_DELEGATE_SCHEDULED	0
> #define MPTCP_DELEGATE_SEND		1
> #define MPTCP_DELEGATE_ACK		2
> +#define MPTCP_DELEGATE_SNDBUF		3
>
> #define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
> /* MPTCP subflow context */
> @@ -520,6 +522,9 @@ struct mptcp_subflow_context {
>
> 	u32	setsockopt_seq;
> 	u32	stale_rcv_tstamp;
> +	int     cached_sndbuf;	    /* sndbuf size when last synced with the msk sndbuf,
> +				     * protected by the msk socket lock
> +				     */
>
> 	struct	sock *tcp_sock;	    /* tcp sk backpointer */
> 	struct	sock *conn;	    /* parent mptcp_sock */
> @@ -768,13 +773,52 @@ static inline bool mptcp_data_fin_enabled(const struct mptcp_sock *msk)
> 	       READ_ONCE(msk->write_seq) == READ_ONCE(msk->snd_nxt);
> }
>
> -static inline bool mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
> +static inline void __mptcp_sync_sndbuf(struct sock *sk)
> {
> -	if ((sk->sk_userlocks & SOCK_SNDBUF_LOCK) || ssk->sk_sndbuf <= READ_ONCE(sk->sk_sndbuf))
> -		return false;
> +	struct mptcp_subflow_context *subflow;
> +	int ssk_sndbuf, new_sndbuf;
> +
> +	if (sk->sk_userlocks & SOCK_SNDBUF_LOCK)
> +		return;
> +
> +	new_sndbuf = sock_net(sk)->ipv4.sysctl_tcp_wmem[0];
> +	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
> +		ssk_sndbuf =  READ_ONCE(mptcp_subflow_tcp_sock(subflow)->sk_sndbuf);
> +
> +		subflow->cached_sndbuf = ssk_sndbuf;
> +		new_sndbuf += ssk_sndbuf;
> +	}
> +
> +	/* the msk max wmem limit is <nr_subflows> * tcp wmem[2] */
> +	WRITE_ONCE(sk->sk_sndbuf, new_sndbuf);
> +}
> +
> +/* The called held both the msk socket and the subflow socket locks,
> + * possibly under BH
> + */
> +static inline void __mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +
> +	if (READ_ONCE(ssk->sk_sndbuf) - subflow->cached_sndbuf)

Hi Paolo -

How about

      if (READ_ONCE(ssk->sk_sndbuf) != subflow->cached_sndbuf)

instead?

Mat


> +		__mptcp_sync_sndbuf(sk);
> +}
> +
> +/* the caller held only the subflow socket lock, either in process or
> + * BH context. Additionally this can be called under the msk data lock,
> + * so we can't acquire such lock here: let the delegate action acquires
> + * the needed locks in suitable order.
> + */
> +static inline void mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +
> +	if (likely(READ_ONCE(ssk->sk_sndbuf) == subflow->cached_sndbuf))
> +		return;
>
> -	WRITE_ONCE(sk->sk_sndbuf, ssk->sk_sndbuf);
> -	return true;
> +	local_bh_disable();
> +	mptcp_subflow_delegate(subflow, MPTCP_DELEGATE_SNDBUF);
> +	local_bh_enable();
> }
>
> static inline void mptcp_write_space(struct sock *sk)
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6102e74121c3..0eae952064b1 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -421,6 +421,7 @@ static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct soc
>
> void __mptcp_set_connected(struct sock *sk)
> {
> +	__mptcp_propagate_sndbuf(sk, mptcp_sk(sk)->first);
> 	if (sk->sk_state == TCP_SYN_SENT) {
> 		inet_sk_state_store(sk, TCP_ESTABLISHED);
> 		sk->sk_state_change(sk);
> @@ -472,7 +473,6 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
> 		return;
>
> 	msk = mptcp_sk(parent);
> -	mptcp_propagate_sndbuf(parent, sk);
> 	subflow->rel_write_seq = 1;
> 	subflow->conn_finished = 1;
> 	subflow->ssn_offset = TCP_SKB_CB(skb)->seq;
> @@ -1728,7 +1728,6 @@ static void subflow_state_change(struct sock *sk)
>
> 	msk = mptcp_sk(parent);
> 	if (subflow_simultaneous_connect(sk)) {
> -		mptcp_propagate_sndbuf(parent, sk);
> 		mptcp_do_fallback(sk);
> 		mptcp_rcv_space_init(msk, sk);
> 		pr_fallback(msk);
> -- 
> 2.41.0
>
>
>

Re: [PATCH mptcp-next v4 4/4] mptcp: refactor sndbuf auto-tuning.

[Paolo Abeni]

On Thu, 2023-09-21 at 16:55 -0700, Mat Martineau wrote:
> On Wed, 20 Sep 2023, Paolo Abeni wrote:
> 
> > The MPTCP protocol account for the data enqueued on all the subflows
> > to the main socket send buffer, while the send buffer auto-tuning
> > algorithm set the main socket send buffer size as the max size among
> > the subflows.
> > 
> > That causes bad performances when at least one subflow is sndbuf
> > limited, e.g. due to very high latency, as the MPTCP scheduler can't
> > even fill such buffer.
> > 
> > Change the send-buffer auto-tuning algorithm to compute the main socket
> > send buffer size as the sum of all the subflows buffer size.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v2 -> v3:
> > - avoid ingremental updates, always recompute sum(ssk->sndbuf) to avoid
> >   drift on memory pressure/decrease
> > ---
> > net/mptcp/protocol.c | 18 +++++++++++++--
> > net/mptcp/protocol.h | 54 ++++++++++++++++++++++++++++++++++++++++----
> > net/mptcp/subflow.c  |  3 +--
> > 3 files changed, 66 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index f727a7ee662d..0a9d00e794d4 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -891,6 +891,7 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
> > 	mptcp_sockopt_sync_locked(msk, ssk);
> > 	mptcp_subflow_joined(msk, ssk);
> > 	mptcp_stop_tout_timer(sk);
> > +	__mptcp_propagate_sndbuf(sk, ssk);
> > 	return true;
> > }
> > 
> > @@ -1077,15 +1078,16 @@ static void mptcp_enter_memory_pressure(struct sock *sk)
> > 	struct mptcp_sock *msk = mptcp_sk(sk);
> > 	bool first = true;
> > 
> > -	sk_stream_moderate_sndbuf(sk);
> > 	mptcp_for_each_subflow(msk, subflow) {
> > 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > 
> > 		if (first)
> > 			tcp_enter_memory_pressure(ssk);
> > 		sk_stream_moderate_sndbuf(ssk);
> > +
> > 		first = false;
> > 	}
> > +	__mptcp_sync_sndbuf(sk);
> > }
> > 
> > /* ensure we get enough memory for the frag hdr, beyond some minimal amount of
> > @@ -2436,6 +2438,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > 		WRITE_ONCE(msk->first, NULL);
> > 
> > out:
> > +	__mptcp_sync_sndbuf(sk);
> > 	if (need_push)
> > 		__mptcp_push_pending(sk, 0);
> > 
> > @@ -3214,7 +3217,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
> > 	 * uses the correct data
> > 	 */
> > 	mptcp_copy_inaddrs(nsk, ssk);
> > -	mptcp_propagate_sndbuf(nsk, ssk);
> > +	__mptcp_propagate_sndbuf(nsk, ssk);
> > 
> > 	mptcp_rcv_space_init(msk, ssk);
> > 	bh_unlock_sock(nsk);
> > @@ -3392,6 +3395,8 @@ static void mptcp_release_cb(struct sock *sk)
> > 			__mptcp_set_connected(sk);
> > 		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
> > 			__mptcp_error_report(sk);
> > +		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
> > +			__mptcp_sync_sndbuf(sk);
> > 	}
> > 
> > 	__mptcp_update_rmem(sk);
> > @@ -3436,6 +3441,14 @@ void mptcp_subflow_process_delegated(struct sock *ssk, long status)
> > 			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
> > 		mptcp_data_unlock(sk);
> > 	}
> > +	if (status & BIT(MPTCP_DELEGATE_SNDBUF)) {
> > +		mptcp_data_lock(sk);
> > +		if (!sock_owned_by_user(sk))
> > +			__mptcp_sync_sndbuf(sk);
> > +		else
> > +			__set_bit(MPTCP_SYNC_SNDBUF, &mptcp_sk(sk)->cb_flags);
> > +		mptcp_data_unlock(sk);
> > +	}
> > 	if (status & BIT(MPTCP_DELEGATE_ACK))
> > 		schedule_3rdack_retransmission(ssk);
> > }
> > @@ -3520,6 +3533,7 @@ bool mptcp_finish_join(struct sock *ssk)
> > 	/* active subflow, already present inside the conn_list */
> > 	if (!list_empty(&subflow->node)) {
> > 		mptcp_subflow_joined(msk, ssk);
> > +		mptcp_propagate_sndbuf(parent, ssk);
> > 		return true;
> > 	}
> > 
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 7c7ad087d8ac..ab775e48c11d 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -123,6 +123,7 @@
> > #define MPTCP_RETRANSMIT	4
> > #define MPTCP_FLUSH_JOIN_LIST	5
> > #define MPTCP_CONNECTED		6
> > +#define MPTCP_SYNC_SNDBUF	7
> > 
> > struct mptcp_skb_cb {
> > 	u64 map_seq;
> > @@ -447,6 +448,7 @@ DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
> > #define MPTCP_DELEGATE_SCHEDULED	0
> > #define MPTCP_DELEGATE_SEND		1
> > #define MPTCP_DELEGATE_ACK		2
> > +#define MPTCP_DELEGATE_SNDBUF		3
> > 
> > #define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
> > /* MPTCP subflow context */
> > @@ -520,6 +522,9 @@ struct mptcp_subflow_context {
> > 
> > 	u32	setsockopt_seq;
> > 	u32	stale_rcv_tstamp;
> > +	int     cached_sndbuf;	    /* sndbuf size when last synced with the msk sndbuf,
> > +				     * protected by the msk socket lock
> > +				     */
> > 
> > 	struct	sock *tcp_sock;	    /* tcp sk backpointer */
> > 	struct	sock *conn;	    /* parent mptcp_sock */
> > @@ -768,13 +773,52 @@ static inline bool mptcp_data_fin_enabled(const struct mptcp_sock *msk)
> > 	       READ_ONCE(msk->write_seq) == READ_ONCE(msk->snd_nxt);
> > }
> > 
> > -static inline bool mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
> > +static inline void __mptcp_sync_sndbuf(struct sock *sk)
> > {
> > -	if ((sk->sk_userlocks & SOCK_SNDBUF_LOCK) || ssk->sk_sndbuf <= READ_ONCE(sk->sk_sndbuf))
> > -		return false;
> > +	struct mptcp_subflow_context *subflow;
> > +	int ssk_sndbuf, new_sndbuf;
> > +
> > +	if (sk->sk_userlocks & SOCK_SNDBUF_LOCK)
> > +		return;
> > +
> > +	new_sndbuf = sock_net(sk)->ipv4.sysctl_tcp_wmem[0];
> > +	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
> > +		ssk_sndbuf =  READ_ONCE(mptcp_subflow_tcp_sock(subflow)->sk_sndbuf);
> > +
> > +		subflow->cached_sndbuf = ssk_sndbuf;
> > +		new_sndbuf += ssk_sndbuf;
> > +	}
> > +
> > +	/* the msk max wmem limit is <nr_subflows> * tcp wmem[2] */
> > +	WRITE_ONCE(sk->sk_sndbuf, new_sndbuf);
> > +}
> > +
> > +/* The called held both the msk socket and the subflow socket locks,
> > + * possibly under BH
> > + */
> > +static inline void __mptcp_propagate_sndbuf(struct sock *sk, struct sock *ssk)
> > +{
> > +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > +
> > +	if (READ_ONCE(ssk->sk_sndbuf) - subflow->cached_sndbuf)
> 
> Hi Paolo -
> 
> How about
> 
>       if (READ_ONCE(ssk->sk_sndbuf) != subflow->cached_sndbuf)
> 
> instead?

Yep, better. This is a left-over from previous revisions where I
actually computed the delta.

I will change in v5

/P

Re: [PATCH mptcp-next v4 1/4] mptcp: fix delegated action races.

[Paolo Abeni]

On Thu, 2023-09-21 at 16:42 -0700, Mat Martineau wrote:
> On Wed, 20 Sep 2023, Paolo Abeni wrote:
> 
> > The delegated action infrastructure is prone to the following
> > race: different CPUs can try to schedule different delegated
> > actions on the same subflow at the same time.
> > 
> > Each of them will check different bits via mptcp_subflow_delegate(),
> > and will try to schedule the action on the related per-cpu napi
> > instance.
> > 
> > Depending on the timing, both can observe an empty delegated list
> > node, causing the same entry to be added simultaneously on two different
> > lists.
> > 
> > The root cause is that the delegated actions infra does not provide
> > a single synchronization point. Address the issue reserving an additional
> > bit to mark the subflow as scheduled for delegation. Acquiring such bit
> > guarantee the caller to own the delegated list node, and being able to
> > safely schedule the subflow.
> > 
> > Clear such bit only when the subflow scheduling is completed, ensuring
> > proper barrier in place.
> > 
> > Additionally swap the meaning of the delegated_action bitmask, to allow
> > the usage of the existing helper to set multiple bit at once.
> > 
> > Fixes: bcd97734318d ("mptcp: use delegate action to schedule 3rd ack retrans")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c | 28 ++++++++++++++--------------
> > net/mptcp/protocol.h | 35 ++++++++++++-----------------------
> > net/mptcp/subflow.c  |  6 ++++--
> > 3 files changed, 30 insertions(+), 39 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 1a0b463f8c97..04eda1b8f7a4 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -3425,24 +3425,21 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
> > 	sk_reset_timer(ssk, &icsk->icsk_delack_timer, timeout);
> > }
> > 
> > -void mptcp_subflow_process_delegated(struct sock *ssk)
> > +void mptcp_subflow_process_delegated(struct sock *ssk, long status)
> > {
> > 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > 	struct sock *sk = subflow->conn;
> > 
> > -	if (test_bit(MPTCP_DELEGATE_SEND, &subflow->delegated_status)) {
> > +	if (status & BIT(MPTCP_DELEGATE_SEND)) {
> > 		mptcp_data_lock(sk);
> > 		if (!sock_owned_by_user(sk))
> > 			__mptcp_subflow_push_pending(sk, ssk, true);
> > 		else
> > 			__set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->cb_flags);
> > 		mptcp_data_unlock(sk);
> > -		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_SEND);
> > 	}
> > -	if (test_bit(MPTCP_DELEGATE_ACK, &subflow->delegated_status)) {
> > +	if (status & BIT(MPTCP_DELEGATE_ACK))
> > 		schedule_3rdack_retransmission(ssk);
> > -		mptcp_subflow_delegated_done(subflow, MPTCP_DELEGATE_ACK);
> > -	}
> > }
> > 
> > static int mptcp_hash(struct sock *sk)
> > @@ -3968,14 +3965,17 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
> > 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > 
> > 		bh_lock_sock_nested(ssk);
> > -		if (!sock_owned_by_user(ssk) &&
> > -		    mptcp_subflow_has_delegated_action(subflow))
> > -			mptcp_subflow_process_delegated(ssk);
> > -		/* ... elsewhere tcp_release_cb_override already processed
> > -		 * the action or will do at next release_sock().
> > -		 * In both case must dequeue the subflow here - on the same
> > -		 * CPU that scheduled it.
> > -		 */
> > +		if (!sock_owned_by_user(ssk)) {
> > +			mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
> > +		} else {
> > +			/* tcp_release_cb_override already processed
> > +			 * the action or will do at next release_sock().
> > +			 * In both case must dequeue the subflow here - on the same
> > +			 * CPU that scheduled it.
> > +			 */
> > +			smp_wmb();
> > +			clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
> > +		}
> > 		bh_unlock_sock(ssk);
> > 		sock_put(ssk);
> > 
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 3c938e3560e4..7c7ad087d8ac 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -444,9 +444,11 @@ struct mptcp_delegated_action {
> > 
> > DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
> > 
> > -#define MPTCP_DELEGATE_SEND		0
> > -#define MPTCP_DELEGATE_ACK		1
> > +#define MPTCP_DELEGATE_SCHEDULED	0
> > +#define MPTCP_DELEGATE_SEND		1
> > +#define MPTCP_DELEGATE_ACK		2
> > 
> > +#define MPTCP_DELEGATE_ACTIONS_MASK	(~BIT(MPTCP_DELEGATE_SCHEDULED))
> > /* MPTCP subflow context */
> > struct mptcp_subflow_context {
> > 	struct	list_head node;/* conn_list of subflows */
> > @@ -564,23 +566,24 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
> > 	return subflow->map_seq + mptcp_subflow_get_map_offset(subflow);
> > }
> > 
> > -void mptcp_subflow_process_delegated(struct sock *ssk);
> > +void mptcp_subflow_process_delegated(struct sock *ssk, long actions);
> > 
> > static inline void mptcp_subflow_delegate(struct mptcp_subflow_context *subflow, int action)
> > {
> > +	long old, mask = BIT(MPTCP_DELEGATE_SCHEDULED) | BIT(action);
> > 	struct mptcp_delegated_action *delegated;
> > 	bool schedule;
> > 
> > 	/* the caller held the subflow bh socket lock */
> > 	lockdep_assert_in_softirq();
> > 
> > -	/* The implied barrier pairs with mptcp_subflow_delegated_done(), and
> > -	 * ensures the below list check sees list updates done prior to status
> > -	 * bit changes
> > +	/* The implied barrier pairs with tcp_release_cb_override()
> > +	 * mptcp_napi_poll(), and ensures the below list check sees list
> > +	 * updates done prior to delegated status bits changes
> > 	 */
> > -	if (!test_and_set_bit(action, &subflow->delegated_status)) {
> > -		/* still on delegated list from previous scheduling */
> > -		if (!list_empty(&subflow->delegated_node))
> > +	old = set_mask_bits(&subflow->delegated_status, 0, mask);
> 
> Hi Paolo -
> 
> The naming of the 'mask' variable here made it a little harder to 
> understand at first glance, as the middle arg of the set_mask_bits macro 
> is the one that does the masking (clearing) of bits. How about 'set_bits' 
> (or similar) instead.

Indeed a better name would help. 'set_bits' is fine by me.

> 
> > +	if (!(old & BIT(MPTCP_DELEGATE_SCHEDULED))) {
> > +		if (WARN_ON_ONCE(!list_empty(&subflow->delegated_node)))
> > 			return;
> > 
> > 		delegated = this_cpu_ptr(&mptcp_delegated_actions);
> > @@ -605,20 +608,6 @@ mptcp_subflow_delegated_next(struct mptcp_delegated_action *delegated)
> > 	return ret;
> > }
> > 
> > -static inline bool mptcp_subflow_has_delegated_action(const struct mptcp_subflow_context *subflow)
> > -{
> > -	return !!READ_ONCE(subflow->delegated_status);
> > -}
> > -
> > -static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *subflow, int action)
> > -{
> > -	/* pairs with mptcp_subflow_delegate, ensures delegate_node is updated before
> > -	 * touching the status bit
> > -	 */
> > -	smp_wmb();
> > -	clear_bit(action, &subflow->delegated_status);
> > -}
> > -
> > int mptcp_is_enabled(const struct net *net);
> > unsigned int mptcp_get_add_addr_timeout(const struct net *net);
> > int mptcp_is_checksum_enabled(const struct net *net);
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 918c1a235790..0d860cb730b4 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1956,9 +1956,11 @@ static void subflow_ulp_clone(const struct request_sock *req,
> > static void tcp_release_cb_override(struct sock *ssk)
> > {
> > 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > +	long status;
> > 
> > -	if (mptcp_subflow_has_delegated_action(subflow))
> > -		mptcp_subflow_process_delegated(ssk);
> > +	status = set_mask_bits(&subflow->delegated_status, MPTCP_DELEGATE_ACTIONS_MASK, 0);
> 
> Can you add a comment here regarding the interaction with 
> mptcp_napi_poll()?

I'll add something alike:

	/* process and clear all the pending actions, but leave the 
	 * subflow into the napi queue. To respect locking,  
	 * only the same CPU that originated the action can touch
	 * the list. mptcp_napi_poll will take care of it.
	 */


/P