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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Sun, 11 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 | 45 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2342b9469181..4c07add44b02 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -45,6 +45,7 @@ static struct percpu_counter mptcp_sockets_allocated ____cacheline_aligned_in_sm
>
> static void __mptcp_destroy_sock(struct sock *sk);
> static void __mptcp_check_send_data_fin(struct sock *sk);
> +static void __mptcp_retrans(struct sock *sk);
>
> DEFINE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
> static struct net_device mptcp_napi_dev;
> @@ -997,7 +998,7 @@ static void __mptcp_clean_una(struct sock *sk)
>
> 		if (unlikely(dfrag == msk->first_pending)) {
> 			/* in recovery mode can see ack after the current snd head */
> -			if (WARN_ON_ONCE(!msk->recovery))
> +			if (!msk->recovery)
> 				break;
>
> 			WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
> @@ -1012,7 +1013,7 @@ static void __mptcp_clean_una(struct sock *sk)
>
> 		/* prevent wrap around in recovery mode */
> 		if (unlikely(delta > dfrag->already_sent)) {
> -			if (WARN_ON_ONCE(!msk->recovery))
> +			if (!msk->recovery)
> 				goto out;
> 			if (WARN_ON_ONCE(delta > dfrag->data_len))
> 				goto out;
> @@ -1111,6 +1112,7 @@ struct mptcp_sendmsg_info {
> 	u16 sent;
> 	unsigned int flags;
> 	bool data_lock_held;
> +	struct mptcp_data_frag *last;
> };
>
> static int mptcp_check_allowed_size(const struct mptcp_sock *msk, struct sock *ssk,
> @@ -1526,6 +1528,7 @@ static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
> 		info->sent = dfrag->already_sent;
> 		info->limit = dfrag->data_len;
> 		len = dfrag->data_len - dfrag->already_sent;
> +		info->last = dfrag;
> 		while (len > 0) {
> 			int ret = 0;
>
> @@ -1562,14 +1565,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, *dfrag;
> 	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))
> 			break;
> @@ -1578,6 +1586,19 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
>
> 		mptcp_for_each_subflow(msk, subflow) {
> 			if (READ_ONCE(subflow->scheduled)) {
> +				if (i > 0) {
> +					WRITE_ONCE(msk->first_pending, head);
> +					mptcp_push_release(ssk, &info, do_check_data_fin);
> +
> +					while ((dfrag = mptcp_send_head(sk))) {
> +						__mptcp_retrans(sk);
> +						if (dfrag == info.last)
> +							break;
> +						WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
> +					}
> +					goto out;
> +				}
> +
> 				mptcp_subflow_set_scheduled(subflow, false);
>
> 				prev_ssk = ssk;
> @@ -1605,6 +1626,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
> 						push_count--;
> 					continue;
> 				}
> +				i++;
> 				do_check_data_fin = true;
> 				msk->last_snd = ssk;
> 			}
> @@ -1614,6 +1636,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
> 	/* at this point we held the socket lock for the last subflow we used */
> 	mptcp_push_release(ssk, &info, do_check_data_fin);
>
> +out:
> 	/* ensure the rtx timer is running */
> 	if (!mptcp_timer_pending(sk))
> 		mptcp_reset_timer(sk);
> @@ -1628,13 +1651,18 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
> 	struct mptcp_sendmsg_info info = {
> 		.data_lock_held = true,
> 	};
> +	struct mptcp_data_frag *head;
> 	bool keep_pushing = true;
> 	struct sock *xmit_ssk;
> 	int copied = 0;
>
> +	head = mptcp_send_head(sk);
> +	if (!head)
> +		goto out;
> +
> 	info.flags = 0;
> 	while (mptcp_send_head(sk) && keep_pushing) {
> -		int ret = 0;
> +		int ret = 0, i = 0;
>
> 		/* check for a different subflow usage only after
> 		 * spooling the first chunk of data
> @@ -1659,18 +1687,27 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
> 			ret = __subflow_push_pending(sk, ssk, &info);
> 			if (ret <= 0)
> 				keep_pushing = false;
> +			i++;
> 			copied += ret;
> 			msk->last_snd = ssk;
> 		}
>
> 		mptcp_for_each_subflow(msk, subflow) {
> 			if (READ_ONCE(subflow->scheduled)) {
> +				if (i > 0) {
> +					WRITE_ONCE(msk->first_pending, head);
> +					if (!test_and_set_bit(MPTCP_WORK_RTX, &msk->flags))
> +						mptcp_schedule_work(sk);

Hi Geliang -

It's not going to perform well enough to use the work queue for redundant 
sends.

MPTCP_WORK_RTX works ok for retransmissions because that is expected to be 
infrequent. A redundant scheduler would be sending everything on multiple 
subflows.

The retransmission code has some ideas for making redundant sends work, 
but redundant sending is different:

1. I think the scheduler will need to store some sequence numbers at the 
msk level so the same data is sent on different subflows, even on the 
__mptcp_subflow_push_pending() code path. This is a change for regular 
(not redundant) schedulers too, and lets the schedulers select which 
subflows to send on AND what data to send.

2. The redundant send code should not behave exactly like retransmit. For 
example, don't call mptcp_sched_get_retrans() or use the retransmit MIB 
counters. Maybe it's simpler to have a separate function rather than add a 
bunch of conditionals to __mptcp_retrans()?

3. When using the __mptcp_subflow_push_pending() code path, the 
MPTCP_DELEGATE_SEND technique should be used repeatedly until all 
redundantly scheduled subflows have sent their data.


I'll check with other community members at the meeting tomorrow to see 
if some other ideas come up.


> +					goto out;
> +				}
> +
> 				mptcp_subflow_set_scheduled(subflow, false);
>
> 				xmit_ssk = mptcp_subflow_tcp_sock(subflow);
> 				if (xmit_ssk != ssk) {
> 					mptcp_subflow_delegate(subflow,
> 							       MPTCP_DELEGATE_SEND);
> +					i++;
> 					msk->last_snd = xmit_ssk;
> 					keep_pushing = false;
> 				}
> -- 
> 2.35.3
>
>
>

Thanks,

--
Mat Martineau
Intel