| Message ID | 20191108220953.30904-4-fw@strlen.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: add and use wmem accounting for rtx queue | expand |
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@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@lists.01.org > To unsubscribe send an email to mptcp-leave@lists.01.org > -- Mat Martineau Intel
Mat Martineau <mathew.j.martineau@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().
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); + + 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)
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@strlen.de> --- net/mptcp/protocol.h | 2 +- net/mptcp/subflow.c | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-)