Re: [PATCH v3 net-next] net: introduce dev_set_forwarding()

Eric Dumazet
2013-11-03T16:28:27Z

On Sun, 2013-11-03 at 20:28 +0800, Herbert Xu wrote:
> On Sat, Nov 02, 2013 at 07:01:37AM -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Christoph Paasch and Jerry Chu reported crashes in skb_segment() caused
> > by commit 8a29111c7ca6 ("net: gro: allow to build full sized skb")
> >
> > skb_segment() only deals with a frag_list chain containing MSS sized
> > fragments. Even if we fix this problem, its better if GRO layer
> > doesn't build skb with a frag_list in the first place, to let TSO
> > packets reaching output devices.
> >
> > David Miller and Ben Hutchings suggested we keep track of number of
> > forwarding users to be able to :
> >
> > - Disable LRO
> > - Make sure GRO layer do not use skb frag_list to extend skb capacity
>
> Why are we still doing this instead of fixing skb_segment to
> deal with skb frag_list properly?
>
> LRO is legacy code and we should not be adding similar cruft
> to GRO.

1) Because we should not call skb_segment() at all on a router ?

2) If you aggregate too much on a router, you increase latencies,
or if you prefer the RTT of TCP flows.

3) Because skb_segment() layer only builds MSS sized skb, so this
remove TSO ability on output path

Splitting a 45 MSS packet into 3 TSO packets (16 + 16 + X MSS) is going
to be quite complex, given the gso_segment() stuff is meant to segment
in MSS packets. Adding complexity in this already complex stuff is
simply not worth it.

For local TCP, its different, because if you receive such high
throughput, ability to build full size GRO packet helps to reduce number
of ACK segments and number of SKB in receive queue (or OFO queue),
without impacting ACK clocking and TCP dynamics.

And even if a router does not do this aggregation, the final receiver
will do.

So in conclusion, GRO is like TSO : Its not because they are able to use
64KB skbs you always _have_ to fill skb to max capacity.