| Message ID | 915cf19849b12e6bd8deabbb406be51eff92c764.1596216310.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: multiple xmit substreams support | expand |
Paolo Abeni <pabeni@redhat.com> wrote: Hmmm, a changelog would have been nice 8-) > - /* discard mapped data */ > - pr_debug("discarding %zu bytes, current map len=%d", delta, > - map_remaining); > - if (delta) { > - read_descriptor_t desc = { > - .count = delta, > - }; > - int ret; > - > - ret = tcp_read_sock(ssk, &desc, subflow_read_actor); tcp_read_sock tosses 'delta' bytes, and may zap multiple skbs, but after this change only at most one skb gets tossed. > return 0; > } Nit: After this change, mptcp_subflow_discard_data() always returns 0. > @@ -853,7 +824,7 @@ static bool subflow_check_data_avail(struct sock *ssk) > /* only accept in-sequence mapping. Old values are spurious > * retransmission > */ > - if (mptcp_subflow_discard_data(ssk, old_ack - ack_seq)) > + if (mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq)) > goto fatal; ... so this conditional can be removed. > return false; Wouldn't it make sense to get rid of the return false too so that this checks if there is another skb to process? This would also obsolete my 'may zap multiple skbs' comment above since the caller would invoke the discard helper again if next skb is also outdated.
On Sun, 2020-08-02 at 01:18 +0200, Florian Westphal wrote: > Paolo Abeni <pabeni@redhat.com> wrote: > > Hmmm, a changelog would have been nice 8-) whoops... I added the comments to most patch just before the submit, but I forgot about this one... > > > - /* discard mapped data */ > > - pr_debug("discarding %zu bytes, current map len=%d", delta, > > - map_remaining); > > - if (delta) { > > - read_descriptor_t desc = { > > - .count = delta, > > - }; > > - int ret; > > - > > - ret = tcp_read_sock(ssk, &desc, subflow_read_actor); > > tcp_read_sock tosses 'delta' bytes, and may zap multiple skbs, but after > this change only at most one skb gets tossed. > > > return 0; > > } > > Nit: After this change, mptcp_subflow_discard_data() always returns 0. > > > @@ -853,7 +824,7 @@ static bool subflow_check_data_avail(struct sock *ssk) > > /* only accept in-sequence mapping. Old values are spurious > > * retransmission > > */ > > - if (mptcp_subflow_discard_data(ssk, old_ack - ack_seq)) > > + if (mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq)) > > goto fatal; > > ... so this conditional can be removed. > > > return false; > > Wouldn't it make sense to get rid of the return false too so that this > checks if there is another skb to process? This would also obsolete my > 'may zap multiple skbs' comment above since the caller would invoke the > discard helper again if next skb is also outdated. Yep, will look at this option. Thanks! /P
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 2b93c2c56316..dfcd7eab35ad 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -352,7 +352,6 @@ int mptcp_is_enabled(struct net *net); void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow, struct mptcp_options_received *mp_opt); bool mptcp_subflow_data_available(struct sock *sk); -int mptcp_subflow_discard_data(struct sock *sk, unsigned limit); void __init mptcp_subflow_init(void); /* called with sk socket lock held */ diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index b926f4f4fb9f..b2bde12b5933 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -735,49 +735,20 @@ static enum mapping_status get_mapping_status(struct sock *ssk, return MAPPING_OK; } -static int subflow_read_actor(read_descriptor_t *desc, - struct sk_buff *skb, - unsigned int offset, size_t len) +static int mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb, + unsigned limit) { - size_t copy_len = min(desc->count, len); - - desc->count -= copy_len; + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); + bool fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN; + u32 incr; - pr_debug("flushed %zu bytes, %zu left", copy_len, desc->count); - return copy_len; -} + incr = limit >= skb->len ? skb->len + fin : limit; -int mptcp_subflow_discard_data(struct sock *ssk, unsigned limit) -{ - struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); - u32 map_remaining; - size_t delta; - - map_remaining = subflow->map_data_len - - mptcp_subflow_get_map_offset(subflow); - delta = min_t(size_t, limit, map_remaining); - - /* discard mapped data */ - pr_debug("discarding %zu bytes, current map len=%d", delta, - map_remaining); - if (delta) { - read_descriptor_t desc = { - .count = delta, - }; - int ret; - - ret = tcp_read_sock(ssk, &desc, subflow_read_actor); - if (ret < 0) { - ssk->sk_err = -ret; - return ret; - } - if (ret < delta) - return 0; - if (delta == map_remaining) { - subflow->data_avail = 0; - subflow->map_valid = 0; - } - } + pr_debug("discarding=%d len=%d seq=%d", incr, skb->len, + subflow->map_subflow_seq); + tcp_sk(ssk)->copied_seq += incr; + if (incr >= skb->len) + sk_eat_skb(ssk, skb); return 0; } @@ -853,7 +824,7 @@ static bool subflow_check_data_avail(struct sock *ssk) /* only accept in-sequence mapping. Old values are spurious * retransmission */ - if (mptcp_subflow_discard_data(ssk, old_ack - ack_seq)) + if (mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq)) goto fatal; return false; }
Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.h | 1 - net/mptcp/subflow.c | 53 ++++++++++---------------------------------- 2 files changed, 12 insertions(+), 42 deletions(-)