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