Re: [PATCH net-next] ip_gre: remove CRC flag from dev features in gre_gso_segment

[Xin Long <lucien.xin@gmail.com>]

On Thu, Nov 19, 2020 at 12:44 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Nov 2020 14:14:49 +0800 Xin Long wrote:
> > On Wed, Nov 18, 2020 at 8:29 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Mon, 16 Nov 2020 17:15:47 +0800 Xin Long wrote:
> > > > This patch is to let it always do CRC checksum in sctp_gso_segment()
> > > > by removing CRC flag from the dev features in gre_gso_segment() for
> > > > SCTP over GRE, just as it does in Commit 527beb8ef9c0 ("udp: support
> > > > sctp over udp in skb_udp_tunnel_segment") for SCTP over UDP.
> > > >
> > > > It could set csum/csum_start in GSO CB properly in sctp_gso_segment()
> > > > after that commit, so it would do checksum with gso_make_checksum()
> > > > in gre_gso_segment(), and Commit 622e32b7d4a6 ("net: gre: recompute
> > > > gre csum for sctp over gre tunnels") can be reverted now.
> > > >
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > >
> > > Makes sense, but does GRE tunnels don't always have a csum.
> > Do you mean the GRE csum can be offloaded? If so, it seems for GRE tunnel
> > we need the similar one as:
> >
> > commit 4bcb877d257c87298aedead1ffeaba0d5df1991d
> > Author: Tom Herbert <therbert@google.com>
> > Date:   Tue Nov 4 09:06:52 2014 -0800
> >
> >     udp: Offload outer UDP tunnel csum if available
> >
> > I will confirm and implement it in another patch.
> >
> > >
> > > Is the current hardware not capable of generating CRC csums over
> > > encapsulated patches at all?
> > There is, but very rare. The thing is after doing CRC csum, the outer
> > GRE/UDP checksum will have to be recomputed, as it's NOT zero after
> > all fields for CRC scum are summed, which is different from the
> > common checksum. So if it's a GRE/UDP tunnel, the inner CRC csum
> > has to be done there even if the HW supports its offload.
>
> Ack, my point is that for UDP tunnels (at least with IPv4) the UDP
> checksum is optional (should be ignored if the header field is 0),
> and for GRE checksum is optional and it's presence is indicated by
> a bit in the header IIRC.
Yes, it's tunnel->parms.o_flags & TUNNEL_CSUM. When it's not set,
gso_type is set to SKB_GSO_GRE instead of SKB_GSO_GRE_CSUM
by gre_handle_offloads().


>
> So if the HW can compute the CRC csum based on offsets, without parsing
> the packet, it should be able to do the CRC on tunneled packets w/o
> checksum in the outer header.
Right, we can only force it to do CRC csum there when SKB_GSO_GRE_CSUM was set:

        need_csum = !!(skb_shinfo(skb)->gso_type & SKB_GSO_GRE_CSUM);
        skb->encap_hdr_csum = need_csum;

        features &= skb->dev->hw_enc_features;
+       if (need_csum)
+               features &= ~NETIF_F_SCTP_CRC;

I will give it a try.

BTW, __skb_udp_tunnel_segment() doesn't need this, as For UDP encaped SCTP,
the UDP csum is always set.

>
> IDK how realistic this is, whether it'd work today, and whether we care
> about it...