On Mon, Jan 14, 2019 at 7:50 AM Steffen Klassert
[off-list ref] wrote:

On Sun, Dec 23, 2018 at 08:15:40PM -0500, Willem de Bruijn wrote:

quoted

On Fri, Dec 21, 2018 at 10:23 AM Steffen Klassert
[off-list ref] wrote:

quoted

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

quoted

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.

quoted

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

quoted

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.

quoted

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.

quoted

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

quoted

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