Re: [PATCH net-next 0/4] net: cleanup for UDP tunnel's GRO

On 08.07.2016 17:27, Alexander Duyck wrote:

On Fri, Jul 8, 2016 at 1:57 PM, Hannes Frederic Sowa
[off-list ref] wrote:

quoted

On 08.07.2016 16:17, Shmulik Ladkani wrote:

quoted

On Fri, 8 Jul 2016 09:21:40 -0700 Alexander Duyck [off-list ref] wrote:

quoted

On Thu, Jul 7, 2016 at 8:58 AM, Paolo Abeni [off-list ref] wrote:

quoted

With udp tunnel offload in place, the kernel can do GRO for some udp tunnels
at the ingress device level. Currently both the geneve and the vxlan drivers
implement an additional GRO aggregation point via gro_cells.
The latter takes effect for tunnels using zero checksum udp packets, which are
currently explicitly not aggregated by the udp offload layer.

This patch series adapts the udp tunnel offload to process also zero checksum
udp packets, if the tunnel's socket allow it. Aggregation, if possible is always
performed at the ingress device level.

Then the gro_cells hooks, in both vxlan and geneve driver are removed.

I think removing the gro_cells hooks may be taking things one step too far.

+1

quoted

I get that there is an impression that it is redundant but there are a
number of paths that could lead to VXLAN or GENEVE frames being
received that are not aggregated via GRO.

There's the case where the vxlan/geneve datagrams get IP fragmented, and
IP frags are not GROed.
GRO aggregation at the vxlan/geneve level is beneficial for this case.

Isn't this a misconfiguration? TCP should not fragment at all, not even
in vxlan/geneve if one cares about performance? And UDP is not GRO'ed
anyway.

The problem is that the DF bit of the outer header is what matters,
not the inner one.  I believe the default for many of the UDP tunnels
is to not set the DF bit on the outer header.  The fact is
fragmentation shouldn't happen, but it can and we need to keep that in
mind when we work on these sort of things.

"Old" tunnel protocols inherit the outer DF bit from the inner one,
geneve and vxlan do not. I think we simply never should set DF bit
because vxlan pmtu with source port rss perturbation breaks pmtu
discovery anyway. On the other hand doing so and not having end-to-end
protection of the VNI because of proposed 0 checksum is also...
otherwise at least the Ethernet FCS could protect the frame.

There have been a number of cases in the past with tunnels where "it
works for me" has been how things have been implemented and we need to
avoid that as it creates a significant amount of pain for end users
when they do things and they don't work because they have strayed off
of the one use case that has been tested.

We certainly don't want to break fragmentation with vxlan and this patch
doesn't change so.

I really do wonder if GRO on top of fragmentation does have any effect.
Would be great if someone has data for that already?

Bye,
Hannes