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

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2021-03-25 at 09:53 -0400, Willem de Bruijn wrote:
> On Thu, Mar 25, 2021 at 6:57 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > 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.

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