Date:   Mon, 14 Jan 2019 13:50:42 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     Network Development <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH RFC 0/3] Support fraglist GRO/GSO

On Sun, Dec 23, 2018 at 08:15:40PM -0500, Willem de Bruijn wrote:
> On Fri, Dec 21, 2018 at 10:23 AM Steffen Klassert
> <steffen.klassert@...unet.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.

Sorry for the huge delay on my side. I was off for quite some time
(vacation directly followed by illness).

> 
> 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.

The hack was to skip the socket lookup. While that was ok for a
forwarding test, it will affect the local receive path of course.

Currently, I check if there is a GRO capable socket. If yes,
do standard GRO. If no, do listified GRO regardless if packets
are going to be forwarded or locally received. So UDP GRO is
always on with this.

> 
> 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.

We would need to do an early route lookup to check if the packets
are going to be forwarded or locally received. The current patchset
does not implement this, but could be done. Maybe doing a route
lookup based on some static key that will be enabled when forwarding
on the receiving device is enabled.

But even if the route lookup tell us that the packet should go the
forwarding path, netfilter (bpfilter?) could reroute the packet.
If we do an early route lookup, it would be good to have some early
netfilter (bpfilter?) too, so that we can know which path the packets
go. In this case we could do listified GRO even for TCP, if we can
know that we have to do software segmentation later on.

Btw. do we already have hardware that can do UDP LSO? If not,
the do listified GRO if no GRO capable socket is present would
be a not too intrusive patchset with what we could start
(given that we want always on UDP GRO).

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

Yes, but should'nt this be the same as with TCP GRO?

> 
> A few smaller questions, not necessarily exhaustive (or all sensible ;)
> - 1/3
>   - do gso handlers never return the original skb currently?

As far as I know, yes. But with the idea from Paolo to just
take a refcount on the skb, we might be able to handle the
return without changes to standard GSO.

> - 2/3
>   - did you mean CHECKSUM_PARTIAL?

The checksum should be ok as is, so should not be calculated again.

>   - are all those assignments really needed, given that nskb was
>     already a fully formed udp packet with just its skb->data moved?

I've already minimized these assignments compared to standard GSO.
Which of the assignments do you think are not needed?

>   - 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)

Yes, will fix this.

>    - 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?

Maybe, I'll look into this.

Thanks for your review!

Powered by blists - more mailing lists