Re: [PATCH v2 net-next] tcp: avoid expensive pskb_expand_head() calls

Ilpo Järvinen
2012-04-20T12:27:43Z

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2004 bytes --]

On Thu, 19 Apr 2012, Eric Dumazet wrote:

> On Thu, 2012-04-19 at 14:57 +0300, Ilpo Järvinen wrote:
>
> > I'm not concerned of the output side, that seems to work because
> > of the in tcp_retransmit_skb getting rid of the extra first.
> >
> > The ACK input stuff is more interesting, e.g., this one in
> > tcp_mark_head_lost:
> >
> > err = tcp_fragment(sk, skb, (packets - oldcnt) * mss, mss);
> >
> > It splits from TCP_SKB_CB(skb)->seq + (packets - oldcnt) * mss whereas
> > I think the desired point would be: TCP_SKB_CB(skb)->seq + offset_ack +
> > (packets - oldcnt) * mss?
> >
> > ...There is similar case in sacktag code too while it's aligning to mss
> > boundaries in tcp_match_skb_to_sack.
>
> Hmm yes, so maybe its safer to update TCP_SKB_CB(skb)->seq in
> tcp_tso_acked() (as well as offset_ack) and make needed adjustements in
> tcp_fragment() if we find offset_ack being not null.

I suppose that somewhat works, it helps here a lot that you work only with
the head skb making lot of cases not possible... I once made something
similar (before I came up the current shift/merge approach) and ended up
to this:
http://www.mail-archive.com/netdev@vger.kernel.org/msg56191.html

...But...

There's another can of worms still it seems.... At least tcp_skb_seglen
that returns weird results when pcount becomes 1!

...Somewhat related to the pcount == 1 problem, I've long wondered if the
zeroed gso_size with pcount == 1 is worth keeping in the first place?

However, I kind of liked the neatness in the original approach where ->seq
does not lie. That would have kept most of stuff very localized because
each skb is still fully valid and consistent with itself, whereas
introducing lies adds lots of hidden traps (except for the pcount of
course, but consistency for it has not been there for some years
already :-)). The tcp_match_skb_to_sack code seems to actually work
exactly because of this consistency (if I now on the second/third read got
it right).

--
i.