Re: [PATCH net-next v2 1/8] udp: fixup csum for GSO receive slow path

On Mon, 2021-03-29 at 11:24 -0400, Willem de Bruijn wrote:

On Mon, Mar 29, 2021 at 11:01 AM Paolo Abeni [off-list ref] wrote:

quoted

On Mon, 2021-03-29 at 09:52 -0400, Willem de Bruijn wrote:

quoted

quoted

+       if (skb->ip_summed == CHECKSUM_NONE && !skb->csum_valid)
+               skb->csum_valid = 1;

Not entirely obvious is that UDP packets arriving on a device with rx
checksum offload off, i.e., with CHECKSUM_NONE, are not matched by
this test.

I assume that such packets are not coalesced by the GRO layer in the
first place. But I can't immediately spot the reason for it..

As you point out, such packets will already have had their checksum
verified at this point, so this branch only matches tunneled packets.
That point is just not immediately obvious from the code.

I understand is a matter of comment clarity ?!?

I'll rewrite the related code comment - in udp_post_segment_fix_csum()
- as:

	/* UDP packets generated with UDP_SEGMENT and traversing:
	 *
         * UDP tunnel(xmit) -> veth (segmentation) -> veth (gro) -> UDP tunnel (rx)
	 * 
         * land here with CHECKSUM_NONE, because __iptunnel_pull_header() converts
         * CHECKSUM_PARTIAL into NONE.
	 * SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST packets with no UDP tunnel will land
	 * here with valid checksum, as the GRO engine validates the UDP csum
	 * before the aggregation and nobody strips such info in between.
	 * Instead of adding another check in the tunnel fastpath, we can force
	 * a valid csum here.
         * Additionally fixup the UDP CB.
         */

Would that be clear enough?

quoted

I do see checksum validation in the GRO engine for CHECKSUM_NONE UDP
packet prior to this series.

I *think* the checksum-and-copy optimization is lost
since 573e8fca255a27e3573b51f9b183d62641c47a3d.

Wouldn't this have been introduced with UDP_GRO?

Uhmm.... looks like the checksum-and-copy optimization has been lost
and recovered a few times. I think the last one
with 9fd1ff5d2ac7181844735806b0a703c942365291, which move the csum
validation before the static branch on udp_encap_needed_key.

Can we agree re-introducing the optimization is independent from this
series?

Thanks!

Paolo