Re: [PATCH net v2 3/4] udp: do not transition UDP fraglist to unnecessary checksum

[Antoine Tenart <atenart@kernel.org>]

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.

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

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

Thanks!
Antoine