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

On Thu, 2021-03-25 at 09:53 -0400, Willem de Bruijn wrote:

On Thu, Mar 25, 2021 at 6:57 AM Paolo Abeni [off-list ref] wrote:

quoted

AFAICS, it depends ;) From skbuff.h:

 *   skb->csum_level indicates the number of consecutive checksums found in
 *   the packet minus one that have been verified as CHECKSUM_UNNECESSARY.

if skb->csum_level > 0, the NIC validate additional headers. The intel
ixgbe driver use that for vxlan RX csum offload. Such field translates
into:

        NAPI_GRO_CB(skb)->csum_cnt

inside the GRO engine, and skb_gro_incr_csum_unnecessary takes care of
the updating it after validation.

True. I glanced over those cases.

More importantly, where exactly do these looped packets get converted
from CHECKSUM_PARTIAL to CHECKSUM_NONE before this patch?

Very good question! It took a bit finding the exact place.

int __iptunnel_pull_header(struct sk_buff *skb, int hdr_len,
			   __be16 inner_proto, bool raw_proto, bool xnet)
{
	if (unlikely(!pskb_may_pull(skb, hdr_len)))
		return -ENOMEM;

	skb_pull_rcsum(skb, hdr_len);
        // here ^^^ via skb_pull_rcsum -> skb_postpull_rcsum() -> __skb_postpull_rcsum()

well, this is actually with _this_ patch applied: it does not change
the place where the ip_summed is set.

quoted

My understanding is that the following should be better:

static inline void udp_post_segment_fix_csum(struct sk_buff *skb)
{
        /* UDP-lite can't land here - no GRO */
        WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

        /* UDP packets generated with UDP_SEGMENT and traversing:
         * UDP tunnel(xmit) -> veth (segmentation) -> veth (gro) -> UDP tunnel (rx)
         * land here with CHECKSUM_NONE. Instead of adding another check
         * in the tunnel fastpath, we can force valid csums here:
         * packets are locally generated and the GRO engine already validated
         * the csum.
         * Additionally fixup the UDP CB
         */
        UDP_SKB_CB(skb)->cscov = skb->len;
        if (skb->ip_summed == CHECKSUM_NONE && !skb->csum_valid)
                skb->csum_valid = 1;
}

I'll use the above in v2.

Do I understand correctly that this avoids matching tunneled packets
that arrive from the network with rx checksumming disabled, because
__skb_gro_checksum_complete will have been called on the outer packet
and have set skb->csum_valid?

Exactly. I did the test, and perf probes showed that.

Yes, this just (1) identifying the packet as being of local source and
then (2) setting csum_valid sounds great to me, thanks.

Will try to submit v2 soon, after some more testing.

Thanks for all the feedback!

Paolo