[MPTCP] Re: [RFC PATCH 3/6] subflow: store known available bytes in subflow

[MPTCP] Re: [RFC PATCH 3/6] subflow: store known available bytes in subflow

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

On Fri, 8 Nov 2019, Florian Westphal wrote:

> Instead of just a flag, store the number of bytes that are known to be
> available for reading.
>
> This will allow a followup patch to include this number in the dss ack.
> At this time, we can only ack up to msk->ack_seq, but that number is
> only updated when userspace reads from the socket.
>
> This could lead to a needless retransmission if userspace doesn't
> read fast enough.
>
> Ideally we could update this number to include all the pending data,
> i.e. tp->rcv_nxt - tp->copied_seq, but there is no guarantee that the
> next skb in sk_receive_queue contains the next packet in logical mptcp
> sequence space.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> net/mptcp/protocol.h |  2 +-
> net/mptcp/subflow.c  | 18 ++++++++++++++----
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 83b06382e56a..bc7e8fa54afa 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -215,8 +215,8 @@ struct mptcp_subflow_context {
> 		conn_finished : 1,
> 		map_valid : 1,
> 		backup : 1,
> -		data_avail : 1,
> 		rx_eof : 1;
> +	u32	data_avail;
> 	u32	remote_nonce;
> 	u64	thmac;
> 	u32	local_nonce;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 76156a7aaea8..0d686f91e0b4 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -612,7 +612,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
> bool mptcp_subflow_data_available(struct sock *sk)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> -	struct sk_buff *skb;
> +	const struct sk_buff *skb;
> +	u32 data_avail = 0;
>
> 	/* check if current mapping is still valid */
> 	if (subflow->map_valid &&
> @@ -636,9 +637,18 @@ bool mptcp_subflow_data_available(struct sock *sk)
> 	}
>
> 	skb = skb_peek(&sk->sk_receive_queue);
> -	subflow->data_avail = skb &&
> -		       before(tcp_sk(sk)->copied_seq, TCP_SKB_CB(skb)->end_seq);
> -	return subflow->data_avail;
> +	if (skb) {
> +		const struct tcp_sock *tp = tcp_sk(sk);
> +		u32 map_remaining = subflow->map_data_len -
> +				    mptcp_subflow_get_map_offset(subflow);
> +

It looks like the mapping can be assumed valid here, since 
subflow_check_data_avail() returned true. It's not obvious looking at this 
function that map_valid should be true at this point - is it worth a 
WARN_ONCE or a comment?

> +		data_avail = TCP_SKB_CB(skb)->end_seq - tp->copied_seq;
> +		data_avail = min_t(u32, data_avail, map_remaining);
> +	}
> +
> +	subflow->data_avail = data_avail;
> +
> +	return subflow->data_avail > 0;
> }
>
> static void subflow_data_ready(struct sock *sk)
> -- 
> 2.23.0
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel

[MPTCP] Re: [RFC PATCH 3/6] subflow: store known available bytes in subflow

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > 	skb = skb_peek(&sk->sk_receive_queue);
> > -	subflow->data_avail = skb &&
> > -		       before(tcp_sk(sk)->copied_seq, TCP_SKB_CB(skb)->end_seq);
> > -	return subflow->data_avail;
> > +	if (skb) {
> > +		const struct tcp_sock *tp = tcp_sk(sk);
> > +		u32 map_remaining = subflow->map_data_len -
> > +				    mptcp_subflow_get_map_offset(subflow);
> > +
> 
> It looks like the mapping can be assumed valid here, since
> subflow_check_data_avail() returned true. It's not obvious looking at this
> function that map_valid should be true at this point - is it worth a
> WARN_ONCE or a comment?

I think I'll go with a WARN_ON_ONCE().