[PATCH mptcp-next 0/2] BPF redundant scheduler, part 3

[PATCH mptcp-next 0/2] BPF redundant scheduler, part 3

[Geliang Tang]

- The DSS issue has been fixed in this version, and all tests
(mptcp_connect.sh, mptcp_join.sh, simult_flows.sh and BPF test_progs)
passed.
- No need to set already_sent to 0, drop this.
- Add retrans_redundant flag.
- depends on "BPF redundant scheduler, part 2" v22.

Geliang Tang (2):
  Squash to "mptcp: use get_send wrapper, v22"
  mptcp: retrans for redundant sends

 net/mptcp/protocol.c | 45 ++++++++++++++++++++++++++++++++++++++++----
 net/mptcp/protocol.h |  1 +
 2 files changed, 42 insertions(+), 4 deletions(-)

-- 
2.35.3

[PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22"

[Geliang Tang]

Cleanup.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 712b183fd80e..ea2e48a74772 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1565,7 +1565,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 		int ret = 0;
 
 		if (mptcp_sched_get_send(msk))
-			break;
+			goto out;
 
 		push_count = 0;
 
@@ -1605,6 +1605,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 		}
 	}
 
+out:
 	/* at this point we held the socket lock for the last subflow we used */
 	if (ssk)
 		mptcp_push_release(ssk, &info);
-- 
2.35.3

[PATCH mptcp-next 2/2] mptcp: retrans for redundant sends

[Geliang Tang]

Redundant sends need to work more like the MPTCP retransmit code path.
When the scheduler selects multiple subflows, the first subflow to send
is a "normal" transmit, and any other subflows would act like a retransmit
when accessing the dfrags.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++++++++++++++---
 net/mptcp/protocol.h |  1 +
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ea2e48a74772..545b29c0e01c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -997,6 +997,8 @@ static void __mptcp_clean_una(struct sock *sk)
 			break;
 
 		if (unlikely(dfrag == msk->first_pending)) {
+			if (msk->retrans_redundant)
+				break;
 			/* in recovery mode can see ack after the current snd head */
 			if (WARN_ON_ONCE(!msk->recovery))
 				break;
@@ -1013,6 +1015,8 @@ static void __mptcp_clean_una(struct sock *sk)
 
 		/* prevent wrap around in recovery mode */
 		if (unlikely(delta > dfrag->already_sent)) {
+			if (msk->retrans_redundant)
+				goto out;
 			if (WARN_ON_ONCE(!msk->recovery))
 				goto out;
 			if (WARN_ON_ONCE(delta > dfrag->data_len))
@@ -1473,7 +1477,8 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 
 static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
 {
-	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
+	if (info->mss_now)
+		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
 	release_sock(ssk);
 }
 
@@ -1555,14 +1560,19 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 	struct sock *prev_ssk = NULL, *ssk = NULL;
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct mptcp_subflow_context *subflow;
+	struct mptcp_data_frag *head = NULL;
 	struct mptcp_sendmsg_info info = {
 				.flags = flags,
 	};
 	bool do_check_data_fin = false;
 	int push_count = 1;
 
+	head = mptcp_send_head(sk);
+	if (!head)
+		goto out;
+
 	while (mptcp_send_head(sk) && (push_count > 0)) {
-		int ret = 0;
+		int ret = 0, i = 0;
 
 		if (mptcp_sched_get_send(msk))
 			goto out;
@@ -1571,6 +1581,14 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 
 		mptcp_for_each_subflow(msk, subflow) {
 			if (READ_ONCE(subflow->scheduled)) {
+				if (i > 0) {
+					if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags))
+						mptcp_schedule_work(sk);
+					WRITE_ONCE(msk->first_pending, head);
+					msk->retrans_redundant = true;
+					goto out;
+				}
+
 				prev_ssk = ssk;
 				ssk = mptcp_subflow_tcp_sock(subflow);
 
@@ -1598,6 +1616,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 						push_count--;
 					continue;
 				}
+				i++;
 				do_check_data_fin = true;
 				msk->last_snd = ssk;
 				mptcp_subflow_set_scheduled(subflow, false);
@@ -1621,16 +1640,21 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct mptcp_subflow_context *subflow;
+	struct mptcp_data_frag *head = NULL;
 	struct mptcp_sendmsg_info info = {
 		.data_lock_held = true,
 	};
 	bool push = true;
 	int copied = 0;
 
+	head = mptcp_send_head(sk);
+	if (!head)
+		goto out;
+
 	info.flags = 0;
 	while (mptcp_send_head(sk) && push) {
 		bool delegate = false;
-		int ret = 0;
+		int ret = 0, i = 0;
 
 		/* check for a different subflow usage only after
 		 * spooling the first chunk of data
@@ -1652,6 +1676,14 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
 			if (READ_ONCE(subflow->scheduled)) {
 				struct sock *xmit_ssk = mptcp_subflow_tcp_sock(subflow);
 
+				if (i > 0) {
+					if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags))
+						mptcp_schedule_work(sk);
+					WRITE_ONCE(msk->first_pending, head);
+					msk->retrans_redundant = true;
+					goto out;
+				}
+
 				if (xmit_ssk != ssk) {
 					/* Only delegate to one subflow recently,
 					 * TODO: chain delegated calls for more subflows.
@@ -1660,6 +1692,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
 						goto out;
 					mptcp_subflow_delegate(subflow,
 							       MPTCP_DELEGATE_SEND);
+					i++;
 					msk->last_snd = ssk;
 					mptcp_subflow_set_scheduled(subflow, false);
 					delegate = true;
@@ -1672,6 +1705,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
 					push = false;
 					continue;
 				}
+				i++;
 				copied += ret;
 				msk->last_snd = ssk;
 				mptcp_subflow_set_scheduled(subflow, false);
@@ -2593,6 +2627,7 @@ static void __mptcp_retrans(struct sock *sk)
 		}
 	}
 	dfrag->already_sent = max(dfrag->already_sent, len);
+	msk->retrans_redundant = false;
 
 reset_timer:
 	mptcp_check_and_set_pending(sk);
@@ -2724,6 +2759,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
 	WRITE_ONCE(msk->allow_infinite_fallback, true);
 	msk->recovery = false;
+	msk->retrans_redundant = false;
 
 	mptcp_pm_data_init(msk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index adfb758a842f..e6bebde2f3ab 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -290,6 +290,7 @@ struct mptcp_sock {
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
 	bool		csum_enabled;
 	bool		allow_infinite_fallback;
+	bool		retrans_redundant;
 	u8		mpc_endpoint_id;
 	u8		recvmsg_inq:1,
 			cork:1,
-- 
2.35.3

Re: [PATCH mptcp-next 1/2] Squash to "mptcp: use get_send wrapper, v22"

[Mat Martineau]

On Mon, 5 Dec 2022, Geliang Tang wrote:

> Cleanup.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/protocol.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 712b183fd80e..ea2e48a74772 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1565,7 +1565,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
> 		int ret = 0;
>
> 		if (mptcp_sched_get_send(msk))
> -			break;
> +			goto out;

I prefer the original code here. It looks like you were making this code 
look more similar to __mptcp_subflow_push_pending(), but I'm not sure that 
the changes to __mptcp_push_pending() in patch 2 are needed. (See my reply 
to patch 2 for more details)

>
> 		push_count = 0;
>
> @@ -1605,6 +1605,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
> 		}
> 	}
>
> +out:
> 	/* at this point we held the socket lock for the last subflow we used */
> 	if (ssk)
> 		mptcp_push_release(ssk, &info);
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next 2/2] mptcp: retrans for redundant sends

[Mat Martineau]

On Mon, 5 Dec 2022, Geliang Tang wrote:

> Redundant sends need to work more like the MPTCP retransmit code path.
> When the scheduler selects multiple subflows, the first subflow to send
> is a "normal" transmit, and any other subflows would act like a retransmit
> when accessing the dfrags.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++++++++++++++---
> net/mptcp/protocol.h |  1 +
> 2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ea2e48a74772..545b29c0e01c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -997,6 +997,8 @@ static void __mptcp_clean_una(struct sock *sk)
> 			break;
>
> 		if (unlikely(dfrag == msk->first_pending)) {
> +			if (msk->retrans_redundant)
> +				break;
> 			/* in recovery mode can see ack after the current snd head */
> 			if (WARN_ON_ONCE(!msk->recovery))
> 				break;
> @@ -1013,6 +1015,8 @@ static void __mptcp_clean_una(struct sock *sk)
>
> 		/* prevent wrap around in recovery mode */
> 		if (unlikely(delta > dfrag->already_sent)) {
> +			if (msk->retrans_redundant)
> +				goto out;
> 			if (WARN_ON_ONCE(!msk->recovery))
> 				goto out;
> 			if (WARN_ON_ONCE(delta > dfrag->data_len))
> @@ -1473,7 +1477,8 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>
> static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
> {
> -	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
> +	if (info->mss_now)
> +		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
> 	release_sock(ssk);
> }
>
> @@ -1555,14 +1560,19 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
> 	struct sock *prev_ssk = NULL, *ssk = NULL;
> 	struct mptcp_sock *msk = mptcp_sk(sk);
> 	struct mptcp_subflow_context *subflow;
> +	struct mptcp_data_frag *head = NULL;
> 	struct mptcp_sendmsg_info info = {
> 				.flags = flags,
> 	};
> 	bool do_check_data_fin = false;
> 	int push_count = 1;
>
> +	head = mptcp_send_head(sk);
> +	if (!head)
> +		goto out;
> +
> 	while (mptcp_send_head(sk) && (push_count > 0)) {
> -		int ret = 0;
> +		int ret = 0, i = 0;
>
> 		if (mptcp_sched_get_send(msk))
> 			goto out;
> @@ -1571,6 +1581,14 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
>
> 		mptcp_for_each_subflow(msk, subflow) {
> 			if (READ_ONCE(subflow->scheduled)) {
> +				if (i > 0) {
> +					if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags))
> +						mptcp_schedule_work(sk);
> +					WRITE_ONCE(msk->first_pending, head);
> +					msk->retrans_redundant = true;
> +					goto out;
> +				}
> +

Hi Geliang -

On this code path, the msk lock is not released between subflow sends, so 
I don't think it's necessary to use the retrans_redundant approach for 
__mptcp_push_pending().

Did you find that the retrans_redundant path was needed in both 
__mptcp_push_pending() and __mptcp_subflow_push_pending()?

> 				prev_ssk = ssk;
> 				ssk = mptcp_subflow_tcp_sock(subflow);
>
> @@ -1598,6 +1616,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
> 						push_count--;
> 					continue;
> 				}
> +				i++;
> 				do_check_data_fin = true;
> 				msk->last_snd = ssk;
> 				mptcp_subflow_set_scheduled(subflow, false);
> @@ -1621,16 +1640,21 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
> {
> 	struct mptcp_sock *msk = mptcp_sk(sk);
> 	struct mptcp_subflow_context *subflow;
> +	struct mptcp_data_frag *head = NULL;
> 	struct mptcp_sendmsg_info info = {
> 		.data_lock_held = true,
> 	};
> 	bool push = true;
> 	int copied = 0;
>
> +	head = mptcp_send_head(sk);
> +	if (!head)
> +		goto out;
> +
> 	info.flags = 0;
> 	while (mptcp_send_head(sk) && push) {
> 		bool delegate = false;
> -		int ret = 0;
> +		int ret = 0, i = 0;
>
> 		/* check for a different subflow usage only after
> 		 * spooling the first chunk of data
> @@ -1652,6 +1676,14 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
> 			if (READ_ONCE(subflow->scheduled)) {
> 				struct sock *xmit_ssk = mptcp_subflow_tcp_sock(subflow);
>
> +				if (i > 0) {
> +					if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags))
> +						mptcp_schedule_work(sk);
> +					WRITE_ONCE(msk->first_pending, head);
> +					msk->retrans_redundant = true;
> +					goto out;
> +				}
> +

I need to look at this in some more detail, but my initial impression is 
that redundant sends will need to avoid the worker thread and instead 
add calls to the retrans code through MPTCP_DELEGATE_SEND.


- Mat

> 				if (xmit_ssk != ssk) {
> 					/* Only delegate to one subflow recently,
> 					 * TODO: chain delegated calls for more subflows.
> @@ -1660,6 +1692,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
> 						goto out;
> 					mptcp_subflow_delegate(subflow,
> 							       MPTCP_DELEGATE_SEND);
> +					i++;
> 					msk->last_snd = ssk;
> 					mptcp_subflow_set_scheduled(subflow, false);
> 					delegate = true;
> @@ -1672,6 +1705,7 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
> 					push = false;
> 					continue;
> 				}
> +				i++;
> 				copied += ret;
> 				msk->last_snd = ssk;
> 				mptcp_subflow_set_scheduled(subflow, false);
> @@ -2593,6 +2627,7 @@ static void __mptcp_retrans(struct sock *sk)
> 		}
> 	}
> 	dfrag->already_sent = max(dfrag->already_sent, len);
> +	msk->retrans_redundant = false;
>
> reset_timer:
> 	mptcp_check_and_set_pending(sk);
> @@ -2724,6 +2759,7 @@ static int __mptcp_init_sock(struct sock *sk)
> 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
> 	WRITE_ONCE(msk->allow_infinite_fallback, true);
> 	msk->recovery = false;
> +	msk->retrans_redundant = false;
>
> 	mptcp_pm_data_init(msk);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index adfb758a842f..e6bebde2f3ab 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -290,6 +290,7 @@ struct mptcp_sock {
> 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
> 	bool		csum_enabled;
> 	bool		allow_infinite_fallback;
> +	bool		retrans_redundant;
> 	u8		mpc_endpoint_id;
> 	u8		recvmsg_inq:1,
> 			cork:1,
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel