Re: [PATCH net v3 3/4] udp: do not transition UDP GRO fraglist partial checksums to unnecessary

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Antoine Tenart wrote:
> Quoting Willem de Bruijn (2024-03-25 19:39:46)
> > Antoine Tenart wrote:
> > > Quoting Willem de Bruijn (2024-03-22 18:29:40)
> > > > 
> > > > Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment)
> > > > have the same checksumming behavior?
> > > 
> > > They can't as non-fraglist GRO packets can be aggregated, csum can't
> > > just be converted there.
> > 
> > I suppose this could be done. But it is just simpler to convert to
> > CHECKSUM_UNNECESSARY.
> 
> Oh, do you mean using the non-fraglist behavior in fraglist?
> udp_gro_complete_segment converts all packets to CHECKSUM_PARTIAL (as
> packets could have been aggregated) but that's not required in fraglist.
> 
> To say it another way: my understanding is packets in the non-fraglist
> case have to be converted to CHECKSUM_PARTIAL, while the fraglist case
> can keep the checksum info as-is (and have the conversion to unnecessary
> as an optimization when applicable).

That makes sense. Thanks.
 
> > You mean that on segmentation, the segments are restored and thus
> > skb->csum of each segment is again correct, right?
> 
> In the fraglist case, yes.
> 
> > I suppose this could be converted to CHECKSUM_UNNECESSARY if just
> > for equivalence between the two UDP_GRO methods and simplicity.
> > 
> > But also fine to leave as is.
> 
> I'm not sure I got your suggestion as I don't see how non-fraglist
> packets could be converted to CHECKSUM_UNNECESSARY when being aggregated
> (uh->len can change there).
> 
> This series is aiming at fixing known issues and unifying the behavior
> would be net-next material IMO. I'll send a v4 for those fixes, but then
> I'm happy to discuss the above suggestion and investigate; so let's
> continue the discussion here in parallel.

The above explains why the solutions are distinct. No need to try to
unify more in a follow-up, then.

> > Can you at least summarize this in the commit message? Currently
> > CHECKSUM_COMPLETE is not mentioned, but the behavior is not trivial.
> > It may be helpful next time we again stumble on this code and do a
> > git blame.
> 
> Sure, I'll try to improve the commit log.
> 
> Thanks!
> Antoine