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

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

On Mon, Jan 14, 2019 at 7:50 AM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> 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@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.
>
> 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.

I understand. What I suggest is to split into two: enable GRO on the
forwarding path independently from converting the method to listified
GRO.

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

Yes, mlx5

I don't think that the route lookup is needed. If listified is cheaper
for local delivery, too, then we can make that the default unless a
device is active with h/w offload and ip forwarding is enabled. If it
isn't, then use it iff ip forwarding is enabled. I think it's fine to
mispredict between the two in edge cases with netfilter mangling, as
long as all paths can correctly handle both types of GRO packets.

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

That is by now well-tested. I don't think we can simply assume
equivalence for UDP, also because that is easier to spoof.

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

That would be great.

> > - 2/3
> >   - did you mean CHECKSUM_PARTIAL?
>
> The checksum should be ok as is, so should not be calculated again.

But CHECKSUM_PARTIAL is not valid on the egress path? As per the
comments at the top of skbuff.h. If the checksum field is correct and
validated, it should be CHECKSUM_NONE.

It is expected as can be seen by the netif_needs_gso() check and
commit cdbee74ce74c ("net: do not do gso for CHECKSUM_UNNECESSARY in
netif_needs_gso"). But that seems like an uncommon protocol edge case
that does not apply to UDP.

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

I had expected that mac_len, mac_header, network_header, truesize are
all already correct based on their initial assignment in the ingress
path. But I may definitely be mistaken here.