| Message ID | 20171014072745.23462-2-den@klaipeden.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | optimisations and reorganisations of tcp_collapse | expand |
On Sat, 2017-10-14 at 16:27 +0900, Koichiro Den wrote: > On the starting point chosen, it could be possible that just one skb > remains in between the range provided, leading to copying and re-insertion > of rb node, which is useless with respect to the rcv buf measurement. > This is rather probable in ooo queue case, in which non-contiguous bloated > packets have been queued up. > > Signed-off-by: Koichiro Den <den@klaipeden.com> > --- > net/ipv4/tcp_input.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index d0682ce2a5d6..1d785b5bf62d 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -4807,7 +4807,8 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root, > start = TCP_SKB_CB(skb)->end_seq; > } > if (end_of_skbs || > - (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN))) > + (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) || > + (TCP_SKB_CB(skb)->seq == start && TCP_SKB_CB(skb)->end_seq == end)) > return; > > __skb_queue_head_init(&tmp); What do you mean by useless ? Surely if this skb contains 17 segments (some USB drivers allocate 8KB per frame), we want to collapse them to save memory. So I do not agree with this patch.
On Sat, 2017-10-14 at 08:42 -0700, Eric Dumazet wrote: > On Sat, 2017-10-14 at 16:27 +0900, Koichiro Den wrote: > > On the starting point chosen, it could be possible that just one skb > > remains in between the range provided, leading to copying and re-insertion > > of rb node, which is useless with respect to the rcv buf measurement. > > This is rather probable in ooo queue case, in which non-contiguous bloated > > packets have been queued up. > > > > Signed-off-by: Koichiro Den <den@klaipeden.com> > > --- > > net/ipv4/tcp_input.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > > index d0682ce2a5d6..1d785b5bf62d 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -4807,7 +4807,8 @@ tcp_collapse(struct sock *sk, struct sk_buff_head > > *list, struct rb_root *root, > > start = TCP_SKB_CB(skb)->end_seq; > > } > > if (end_of_skbs || > > - (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN))) > > + (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) || > > + (TCP_SKB_CB(skb)->seq == start && TCP_SKB_CB(skb)->end_seq == > > end)) > > return; > > > > __skb_queue_head_init(&tmp); > > > What do you mean by useless ? > > Surely if this skb contains 17 segments (some USB drivers allocate 8KB > per frame), we want to collapse them to save memory. > > So I do not agree with this patch. > > I missed that, and sorry about bothering with all, I totally misunderstood it. Thank you for all of them.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index d0682ce2a5d6..1d785b5bf62d 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -4807,7 +4807,8 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root, start = TCP_SKB_CB(skb)->end_seq; } if (end_of_skbs || - (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN))) + (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) || + (TCP_SKB_CB(skb)->seq == start && TCP_SKB_CB(skb)->end_seq == end)) return; __skb_queue_head_init(&tmp);
On the starting point chosen, it could be possible that just one skb remains in between the range provided, leading to copying and re-insertion of rb node, which is useless with respect to the rcv buf measurement. This is rather probable in ooo queue case, in which non-contiguous bloated packets have been queued up. Signed-off-by: Koichiro Den <den@klaipeden.com> --- net/ipv4/tcp_input.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)