[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <030bcf7a14ada8caa464bb33916e5abc19eab67c.camel@redhat.com>
Date: Thu, 25 Mar 2021 17:47:36 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Network Development <netdev@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Steffen Klassert <steffen.klassert@...unet.com>,
Alexander Lobakin <alobakin@...me>
Subject: 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 <pabeni@...hat.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
Powered by blists - more mailing lists