Re: [PATCH mptcp-next v2 1/9] mptcp: add noncontiguous flag

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2021-09-09 at 19:51 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added a "noncontiguous" flag in the msk to track whether the
> data is contiguous on a subflow. When retransmission happens, we could
> set this flag, and once all retransmissions are DATA_ACK'd that flag
> could be cleared.
> 
> When a bad checksum is detected and a single contiguous subflow is in
> use, don't send RST + MP_FAIL, send data_ack + MP_FAIL instead.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/protocol.c |  7 +++++++
>  net/mptcp/protocol.h |  2 ++
>  net/mptcp/subflow.c  | 12 ++++++------
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index bb8a2a231479..81ea03b9fff6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1095,6 +1095,9 @@ static void __mptcp_clean_una(struct sock *sk)
>  
>  		dfrag_uncharge(sk, delta);
>  		cleaned = true;
> +
> +		if (dfrag->resend_count == 0)
> +			WRITE_ONCE(msk->noncontiguous, false);
>  	}
>  
>  	/* all retransmitted data acked, recovery completed */
> @@ -1171,6 +1174,7 @@ mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag *pfrag,
>  	dfrag->overhead = offset - orig_offset + sizeof(struct mptcp_data_frag);
>  	dfrag->offset = offset + sizeof(struct mptcp_data_frag);
>  	dfrag->already_sent = 0;
> +	dfrag->resend_count = 0;
>  	dfrag->page = pfrag->page;
>  
>  	return dfrag;
> @@ -2454,6 +2458,8 @@ static void __mptcp_retrans(struct sock *sk)
>  		dfrag->already_sent = max(dfrag->already_sent, info.sent);
>  		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
>  			 info.size_goal);
> +		dfrag->resend_count++;
> +		WRITE_ONCE(msk->noncontiguous, true);
>  	}
>  
>  	release_sock(ssk);
> @@ -2872,6 +2878,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
>  	WRITE_ONCE(msk->fully_established, false);
>  	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
>  		WRITE_ONCE(msk->csum_enabled, true);
> +	WRITE_ONCE(msk->noncontiguous, false);
>  
>  	msk->write_seq = subflow_req->idsn + 1;
>  	msk->snd_nxt = msk->write_seq;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index d3e6fd1615f1..011f84ae1593 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -213,6 +213,7 @@ struct mptcp_data_frag {
>  	u16 offset;
>  	u16 overhead;
>  	u16 already_sent;
> +	u16 resend_count;
>  	struct page *page;
>  };

Ouch, the above increases mptcp_data_frag size by 8 bytes, due to
holes.

Is this necessary? I think the packet scheduler never reinject with a
single subflow. It used to do that, but it should not do anymore.

Even if the scheduler re-inject with a single subflow, can we instead
keep track of the last retrans sequence number - in __mptcp_retrans().

msk stream is 'continuous' if and only if 'before64(last_retrnas_seq,
msk->snd_una)'

/P