On Thu, 2016-09-08 at 14:02 +0300, Ilpo Järvinen wrote:
> On Wed, 7 Sep 2016, Eric Dumazet wrote:
> While testing, was there any check done for the data that was delivered
> in order to ensure that no corruption occured (either by you or Yaogong)?
> ...This kind of changes have some potential to cause some corruption to
> the stream content and it would be nice to be sure there wasn't any
> accidents.
Sure, we did tests on real GFE, serving real data to users.
My ssh/scp sessions did not catch any error, even with up to 10% packet
losses.
> > }
> >
> > - seq = TCP_SKB_CB(skb)->seq;
> > - end_seq = TCP_SKB_CB(skb)->end_seq;
>
> I hate to nitpick but moving these variables earlier and the
> TCP_SKB_CB(skb)->seq/end_seq simplifications seem unrelated and
> could be done in another patch?
You are right.
I could split this patch in 3 parts if David requests it.
1) One patch adding skb_rbtree_purge(struct rb_root *root) helper
2) One patch doing this seq/end_seq cleanup.
3) RB patch
> > -
> > - for (;;) {
> > - struct sk_buff *next = NULL;
> >
> > - if (!skb_queue_is_last(&tp->out_of_order_queue, skb))
> > - next = skb_queue_next(&tp->out_of_order_queue, skb);
> > - skb = next;
> > + for (head = skb;;) {
> > + skb = tcp_skb_next(skb, NULL);
> >
> > - /* Segment is terminated when we see gap or when
> > - * we are at the end of all the queue. */
> > + /* Range is terminated when we see a gap or when
> > + * we are at the queue end.
> > + */
> > if (!skb ||
> > after(TCP_SKB_CB(skb)->seq, end) ||
> > before(TCP_SKB_CB(skb)->end_seq, start)) {
> > - tcp_collapse(sk, &tp->out_of_order_queue,
> > + tcp_collapse(sk, NULL, &tp->out_of_order_queue,
> > head, skb, start, end);
> > - head = skb;
> > - if (!skb)
> > - break;
> > - /* Start new segment */
> > + goto new_range;
> > + }
> > +
> > + if (unlikely(before(TCP_SKB_CB(skb)->seq, start)))
> > start = TCP_SKB_CB(skb)->seq;
> > + if (after(TCP_SKB_CB(skb)->end_seq, end))
> > end = TCP_SKB_CB(skb)->end_seq;
> > - } else {
> > - if (before(TCP_SKB_CB(skb)->seq, start))
> > - start = TCP_SKB_CB(skb)->seq;
> > - if (after(TCP_SKB_CB(skb)->end_seq, end))
> > - end = TCP_SKB_CB(skb)->end_seq;
> > - }
> > }
> > }
>
> I tried long to think if I could propose alternative layout which would
> make this function to exit from the end but couldn't come up anything
> sensible. As is, it's always exiting within that top if block which is
> somewhat unintuitive :-).
>
> Acked-By: Ilpo Järvinen <ilpo.jarvinen@helsinki>
>
Thanks for the review !
