Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Paolo Abeni <pabeni@redhat.com>]

On Fri, 2016-07-08 at 16:03 -0500, Tom Herbert wrote:
> On Thu, Jul 7, 2016 at 10:58 AM, Paolo Abeni <pabeni@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 <pabeni@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