linux-sctp.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexander Duyck <alexander.duyck@gmail.com>
To: Xin Long <lucien.xin@gmail.com>
Cc: network dev <netdev@vger.kernel.org>,
	"linux-sctp @ vger . kernel . org" <linux-sctp@vger.kernel.org>,
	Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Guillaume Nault <gnault@redhat.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Lorenzo Bianconi <lorenzo@kernel.org>
Subject: Re: [PATCH net-next] ip_gre: remove CRC flag from dev features in gre_gso_segment
Date: Wed, 18 Nov 2020 12:35:21 -0800	[thread overview]
Message-ID: <CAKgT0UdnAfYA1h2dRb4naWZRn5CBfe-0jGd_Vr=hmejX6hR1og@mail.gmail.com> (raw)
In-Reply-To: <52ee1b515df977b68497b1b08290d00a22161279.1605518147.git.lucien.xin@gmail.com>

On Mon, Nov 16, 2020 at 1:17 AM Xin Long <lucien.xin@gmail.com> 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>
> ---
>  net/ipv4/gre_offload.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
> index e0a2465..a5935d4 100644
> --- a/net/ipv4/gre_offload.c
> +++ b/net/ipv4/gre_offload.c
> @@ -15,12 +15,12 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
>                                        netdev_features_t features)
>  {
>         int tnl_hlen = skb_inner_mac_header(skb) - skb_transport_header(skb);
> -       bool need_csum, need_recompute_csum, gso_partial;
>         struct sk_buff *segs = ERR_PTR(-EINVAL);
>         u16 mac_offset = skb->mac_header;
>         __be16 protocol = skb->protocol;
>         u16 mac_len = skb->mac_len;
>         int gre_offset, outer_hlen;
> +       bool need_csum, gso_partial;
>
>         if (!skb->encapsulation)
>                 goto out;
> @@ -41,10 +41,10 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
>         skb->protocol = skb->inner_protocol;
>
>         need_csum = !!(skb_shinfo(skb)->gso_type & SKB_GSO_GRE_CSUM);
> -       need_recompute_csum = skb->csum_not_inet;
>         skb->encap_hdr_csum = need_csum;
>
>         features &= skb->dev->hw_enc_features;
> +       features &= ~NETIF_F_SCTP_CRC;
>
>         /* segment inner packet. */
>         segs = skb_mac_gso_segment(skb, features);

Why just blindly strip NETIF_F_SCTP_CRC? It seems like it would make
more sense if there was an explanation as to why you are stripping the
offload. I know there are many NICs that could very easily perform
SCTP CRC offload on the inner data as long as they didn't have to
offload the outer data. For example the Intel NICs should be able to
do it, although when I wrote the code up enabling their offloads I
think it is only looking at the outer headers so that might require
updating to get it to not use the software fallback.

It really seems like we should only be clearing NETIF_F_SCTP_CRC if
need_csum is true since we must compute the CRC before we can compute
the GRE checksum.

> @@ -99,15 +99,7 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
>                 }
>
>                 *(pcsum + 1) = 0;
> -               if (need_recompute_csum && !skb_is_gso(skb)) {
> -                       __wsum csum;
> -
> -                       csum = skb_checksum(skb, gre_offset,
> -                                           skb->len - gre_offset, 0);
> -                       *pcsum = csum_fold(csum);
> -               } else {
> -                       *pcsum = gso_make_checksum(skb, 0);
> -               }
> +               *pcsum = gso_make_checksum(skb, 0);
>         } while ((skb = skb->next));
>  out:
>         return segs;

This change doesn't make much sense to me. How are we expecting
gso_make_checksum to be able to generate a valid checksum when we are
dealing with a SCTP frame? From what I can tell it looks like it is
just setting the checksum to ~0 and checksum start to the transport
header which isn't true because SCTP is using a CRC, not a 1's
complement checksum, or am I missing something? As such in order to
get the gre checksum we would need to compute it over the entire
payload data wouldn't we? Has this been tested with an actual GRE
tunnel that had checksums enabled? If so was it verified that the GSO
frames were actually being segmented at the NIC level and not at the
GRE tunnel level?

  parent reply	other threads:[~2020-11-18 20:35 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-16  9:15 [PATCH net-next] ip_gre: remove CRC flag from dev features in gre_gso_segment Xin Long
2020-11-18  0:29 ` Jakub Kicinski
2020-11-18  6:14   ` Xin Long
2020-11-18 16:44     ` Jakub Kicinski
2020-11-19  5:32       ` Xin Long
2020-11-18 20:35 ` Alexander Duyck [this message]
2020-11-19  5:53   ` Xin Long
2020-11-19 17:24     ` Alexander Duyck
2020-11-20 10:22       ` Xin Long
2020-11-20 16:10         ` Alexander Duyck
2020-11-23  9:14           ` Xin Long
2020-11-23 22:23             ` Alexander Duyck
2020-11-24 13:30               ` Xin Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKgT0UdnAfYA1h2dRb4naWZRn5CBfe-0jGd_Vr=hmejX6hR1og@mail.gmail.com' \
    --to=alexander.duyck@gmail.com \
    --cc=davem@davemloft.net \
    --cc=gnault@redhat.com \
    --cc=kuba@kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=lorenzo@kernel.org \
    --cc=lucien.xin@gmail.com \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).