Re: [PATCH RFC 0/3] Support fraglist GRO/GSO

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Fri, Dec 21, 2018 at 10:23 AM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> This patchset adds support to do GRO/GSO by chaining packets
> of the same flow at the SKB frag_list pointer. This avoids
> the overhead to merge payloads into one big packet, and
> on the other end, if GSO is needed it avoids the overhead
> of splitting the big packet back to the native form.
>
> Patch 1 prepares GSO to handle fraglist GSO packets.
> Patch 2 adds the core infrastructure to do fraglist
> GRO/GSO. Patch 3 enables IPv4 UDP to use fraglist
> GRO/GSO if no GRO supported socket is found.
>
> I have only forwarding performance measurements so far:
>
> I used used my IPsec forwarding test setup for this:
>
>            ------------         ------------
>         -->| router 1 |-------->| router 2 |--
>         |  ------------         ------------  |
>         |                                     |
>         |       --------------------          |
>         --------|Spirent Testcenter|<----------
>                 --------------------
>
> net-next (December 10th):
>
> Single stream UDP frame size 1460 Bytes: 1.341.700 fps (15.67 Gbps).
>
> ----------------------------------------------------------------------
>
> net-next (December 10th) + hack to enable forwarding for standard UDP GRO:
>
> Single stream UDP frame size 1460 Bytes: 1.651.200 fps (19.28 Gbps).
>
> ----------------------------------------------------------------------
>
> net-next (December 10th) + fraglist UDP GRO/GSO:
>
> Single stream UDP frame size 1460 Bytes: 2.742.500 fps (32.03 Gbps).

That's an impressive speed-up over regular UDP GRO. Definitely worth
looking into more, then.

Sorry for the delay. I still haven't parsed everything yet, but a few
high level questions and comments.

This sounds similar to GSO_BY_FRAGS as used by SCTP. Can perhaps reuse
that or deduplicate a bit. It is nice that this uses a separate
skb_segment_list function; skb_segment is arguably too complex as is
already.

This requires UDP GSO to always be enabled, similar to TCP GSO (as of
commit "tcp: switch to GSO being always on").

I would prefer to split the patch that adds UDP GRO on the forwarding
path into one that enables it for existing GRO (the hack you refer to
above) and a second one to optionally convert to listified processing.

Ideally, we can use existing segmentation on paths where hardware UDP
LSO is supported. I'm not quite sure how to decide between the two
yet. Worst case, perhaps globally use listified forwarding unless any
device is registered with hardware offload, then use regular
segmentation.

For both existing UDP GRO and listified, we should verify that this is
not a potential DoS vector before enabling by default.

A few smaller questions, not necessarily exhaustive (or all sensible ;)
- 1/3
  - do gso handlers never return the original skb currently?
- 2/3
  - did you mean CHECKSUM_PARTIAL?
  - are all those assignments really needed, given that nskb was
    already a fully formed udp packet with just its skb->data moved?
  - calls skb_needs_linearize on the first of the segments in the list only?
- 3/3
   - after pskb_may_pull must reload all ptrs into the data (uh)
   - there are some differences in preparation before the skb is
     passed to skb_gro_receive_list vs skb_gro_receive. Is this
     really needed? They should be interchangeable?