Message-ID: <718747435bca1a76377fca226a085396e0415ffc.camel@redhat.com>
Date: Thu, 21 Mar 2024 13:42:01 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Antoine Tenart <atenart@...nel.org>, Willem de Bruijn
 <willemdebruijn.kernel@...il.com>, davem@...emloft.net,
 edumazet@...gle.com,  kuba@...nel.org
Cc: steffen.klassert@...unet.com, netdev@...r.kernel.org
Subject: Re: [PATCH net v2 3/4] udp: do not transition UDP fraglist to
 unnecessary checksum

On Thu, 2024-03-21 at 09:48 +0100, Antoine Tenart wrote:
> Quoting Willem de Bruijn (2024-03-20 21:43:55)
> > Antoine Tenart wrote:
> > > Quoting Willem de Bruijn (2024-03-20 14:00:48)
> > > > Antoine Tenart wrote:
> > > > > Quoting Willem de Bruijn (2024-03-19 14:38:20)
> > > > > > 
> > > > > > The original patch converted to CHECKSUM_UNNECESSARY for a reason.
> > > > > > The skb->csum of the main gso_skb is not valid?
> > > > > > 
> > > > > > Should instead only the csum_level be adjusted, to always keep
> > > > > > csum_level == 0?
> > > > > 
> > > > > The above trace is an ICMPv6 packet being tunneled and GROed at the UDP
> > > > > level, thus we have:
> > > > >   UDP(CHECKSUM_PARTIAL)/Geneve/ICMPv6(was CHECKSUM_NONE)
> > > > > csum_level would need to be 1 here; but we can't know that.
> > > > 
> > > > Is this a packet looped internally? Else it is not CHECKSUM_PARTIAL.
> > > 
> > > I'm not sure to follow, CHECKSUM_NONE packets going in a tunnel will be
> > > encapsulated and the outer UDP header will be CHECKSUM_PARTIAL. The
> > > packet can be looped internally or going to a remote host.
> > 
> > That is on transmit. To come into contact with UDP_GRO while having
> > CHECKSUM_PARTIAL the packet will have to loop into the receive path,
> > in some way that triggers GRO. Perhaps through gro_cells, as other
> > GRO paths are hardware NIC drivers.
> 
> I get what you meant now, thanks. Yes, those Tx packets loop into the Rx
> path. One easy way is through veth pairs, eg. packet get tunneled in a
> netns, connected to another one via a veth pair.
> 
> > > > > There is another issue (no kernel trace): if a packet has partial csum
> > > > > and is being GROed that information is lost and the packet ends up with
> > > > > an invalid csum.
> > > > 
> > > > CHECKSUM_PARTIAL should be converted to CHECKSUM_UNNECESSARY for this
> > > > reason. CHECKSUM_PARTIAL implies the header is prepared with pseudo
> > > > header checksum. Similarly CHECKSUM_COMPLETE implies skb csum is valid.
> > > > CHECKSUM_UNNECESSARY has neither expectations.
> > > 
> > > But not if the packet is sent to a remote host. Otherwise an inner
> > > partial csum is never fixed by the stack/NIC before going out.
> > 
> > The stack will only offload a single checksum. With local checksum
> > offload, this can be the inner checksum and the outer can be cheaply
> > computed in software. udp_set_csum() handles this. It indeed sets lco
> > if the inner packet has CHECKSUM_PARTIAL. Otherwise it sets ip_summed
> > to CHECKSUM_PARTIAL, now pointing to the outer UDP header.
> > 
> > You're right. Regardless of whether it points to the inner or outer
> > checksum, a conversion of CHECKSUM_PARTIAL to CHECKSUM_UNNECESSARY
> > will break checksum offload in the forwarding case.
> > 
> > > > > Packets with CHECKSUM_UNNECESSARY should end up with the same info. My
> > > > > impression is this checksum conversion is at best setting the same info
> > > > > and otherwise is overriding valuable csum information.
> > > > > 
> > > > > Or would packets with CSUM_NONE being GROed would benefit from the
> > > > > CHECKSUM_UNNECESSARY conversion?
> > > > 
> > > > Definitely. If the packet has CHECKSUM_NONE and GRO checks its
> > > > validity in software, converting it to CHECKSUM_UNNECESSARY avoids
> > > > potential additional checks at later stages in the packet path.
> > > 
> > > Makes sense. The current code really looks like
> > > __skb_incr_checksum_unnecessary, w/o the CHECKSUM_NONE check to only
> > > convert those packets.
> 
> If I sum up our discussion CHECKSUM_NONE conversion is wanted,
> CHECKSUM_UNNECESSARY conversion is a no-op and CHECKSUM_PARTIAL
> conversion breaks things. What about we just convert CHECKSUM_NONE to
> CHECKSUM_UNNECESSARY?
> 
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index 50a8a65fad23..44779d4c538b 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -174,7 +174,7 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
>                 if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
>                         if (skb->csum_level < SKB_MAX_CSUM_LEVEL)
>                                 skb->csum_level++;
> -               } else {
> +               } else if (skb->ip_summed == CHECKSUM_NONE) {
>                         skb->ip_summed = CHECKSUM_UNNECESSARY;
>                         skb->csum_level = 0;
>                 }
> 
> Or directly call __skb_incr_checksum_unnecessary

I think calling __skb_incr_checksum_unnecessary() would be the better
option.

@Willem: FTR, Antoine and me discussed this series internally for a
bit, and the above variant was also discussed. I was unable to find a
good reason for the CHECKSUM_NONE -> CHECKSUM_UNNECESSARY conversion
back then, so it seemed more clean to drop the whole chunk.

Cheers,

Paolo

Powered by blists - more mailing lists