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

[Geliang Tang <geliang.tang@suse.com>]

On Wed, Dec 14, 2022 at 05:47:12PM -0800, Mat Martineau wrote:
> 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.

Sorry, Mat, I didn't get this idea yet. Please give me more details
about it. We should add sched_seq_start and sched_seq_end in struct
mptcp_sock, right? These sequence numbers should be set in the BPF
context by the users, so we need to add sched_seq_start and
sched_seq_end in struct mptcp_sched_data too. Something likes:

        for (int i = 0; i < MPTCP_SUBFLOWS_MAX; i++) {
                if (!data->contexts[i])
                        break;

                mptcp_subflow_set_scheduled(data->contexts[i], true);
		data->sched_seq_start = SN;
		data->sched_seq_end = SN + LEN;
        }

How can the users know what sequence number (SN) to write from? The
sequence number is generated in the kernel?

We can use (msk->sched_seq_end - msk->sched_seq_start) to replace
"msk->snd_burst", but I still don't know how to check these sequence
numbers in __mptcp_subflow_push_pending().

Thanks,
-Geliang

> 
> 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