On Fri, 2016-07-08 at 16:03 -0500, Tom Herbert wrote: > On Thu, Jul 7, 2016 at 10:58 AM, Paolo Abeni <pab...@redhat.com> wrote: > > currently, UDP packets with zero checksum are not allowed to > > use udp offload's GRO. This patch admits such packets to > > GRO, if the related socket settings allow it: ipv6 packets > > are not admitted if the sockets don't have the no_check6_rx > > flag set. > > > > Signed-off-by: Paolo Abeni <pab...@redhat.com> > > --- > > net/ipv4/udp_offload.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > index 9c37338..ac783f4 100644 > > --- a/net/ipv4/udp_offload.c > > +++ b/net/ipv4/udp_offload.c > > @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, > > struct sk_buff *skb, > > struct sock *sk; > > > > if (NAPI_GRO_CB(skb)->encap_mark || > > - (skb->ip_summed != CHECKSUM_PARTIAL && > > + (uh->check && skb->ip_summed != CHECKSUM_PARTIAL && > > NAPI_GRO_CB(skb)->csum_cnt == 0 && > > !NAPI_GRO_CB(skb)->csum_valid)) > > Paolo, > > I think you might be misunderstanding the intent of this conditional. > It is trying to deduce that that the inner TCP checksum has likely > been validated or can be validated without doing packet checksum > calculation. This is trying to avoid doing host side checksum > calculation in the GRO path and really has little to do with rather > uh->check is zero or not. The assumption was that we shouldn't compute > whole packet checksums in the GRO path because of performance. If this > assumption is no longer valid (i.e. there's good data saying doing > checksums in GRO path is a benefit) then all the checksum parts of > this conditional should be removed.
Oh, my bad! I was hit by an ixgbe errata (82599 sometimes marks zero checksum udp packets with CHECKSUM_NONE), so in my tests the above condition was matched by 0 csum UDP packets. Than I misread csum_cnt documentation, assuming it was not incremented for zero checksum UDP packets: I thought that the matches I saw were due to !uh->check instead of missing offload. Thank you for the clarification, Paolo
