[PATCH net-next 0/3] net: add support for ip generic checksum offload for gre

[PATCH net-next 0/3] net: add support for ip generic checksum offload for gre

[Xin Long]

NETIF_F_IP(V6)_CSUM is only for TCP and UDP checksum offload, and
NETIF_F_HW_CSUM is not only for TCP and UDP's, but also for all
other kinds of 1's complement checksums offload, like GRE's. This
patchset is to support it for GRE.

To support GRE checksum offload, this patchset is to extend the
csum_not_inet/csum_type of sk_buff in Patch 1/3, and define new
type of CSUM_T_IP_GENERIC to get ip generic checksum processed
in skb_csum_hwoffload_help() in Patch 2/3, then implement it on
TX path and GSO path in Patch 3/3.

Xin Long (3):
  net: rename csum_not_inet to csum_type
  net: add CSUM_T_IP_GENERIC csum_type
  ip_gre: add csum offload support for gre header

 include/linux/skbuff.h | 22 +++++++++++++---------
 include/net/gre.h      | 20 ++++++++------------
 net/core/dev.c         | 19 ++++++++++++++-----
 net/ipv4/gre_offload.c | 16 ++++++++++++++--
 net/sched/act_csum.c   |  2 +-
 net/sctp/offload.c     |  2 +-
 net/sctp/output.c      |  3 ++-
 7 files changed, 53 insertions(+), 31 deletions(-)

-- 
2.1.0

[PATCH net-next 1/3] net: rename csum_not_inet to csum_type

[Xin Long]

This patch is to rename csum_not_inet to csum_type, as later
more csum type would be introduced in the next patch.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/skbuff.h | 19 +++++++++++--------
 net/core/dev.c         |  2 +-
 net/sched/act_csum.c   |  2 +-
 net/sctp/offload.c     |  2 +-
 net/sctp/output.c      |  2 +-
 5 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 186dad2..67b0a01 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -169,7 +169,7 @@
  *   skb_csum_hwoffload_help() can be called to resolve CHECKSUM_PARTIAL based
  *   on network device checksumming capabilities: if a packet does not match
  *   them, skb_checksum_help or skb_crc32c_help (depending on the value of
- *   csum_not_inet, see item D.) is called to resolve the checksum.
+ *   csum_type, see item D.) is called to resolve the checksum.
  *
  * CHECKSUM_NONE:
  *
@@ -190,12 +190,12 @@
  *   NETIF_F_SCTP_CRC - This feature indicates that a device is capable of
  *     offloading the SCTP CRC in a packet. To perform this offload the stack
  *     will set csum_start and csum_offset accordingly, set ip_summed to
- *     CHECKSUM_PARTIAL and set csum_not_inet to 1, to provide an indication in
- *     the skbuff that the CHECKSUM_PARTIAL refers to CRC32c.
+ *     CHECKSUM_PARTIAL and set csum_type to CSUM_T_SCTP_CRC, to provide an
+ *     indication in the skbuff that the CHECKSUM_PARTIAL refers to CRC32c.
  *     A driver that supports both IP checksum offload and SCTP CRC32c offload
  *     must verify which offload is configured for a packet by testing the
- *     value of skb->csum_not_inet; skb_crc32c_csum_help is provided to resolve
- *     CHECKSUM_PARTIAL on skbs where csum_not_inet is set to 1.
+ *     value of skb->csum_type; skb_crc32c_csum_help is provided to resolve
+ *     CHECKSUM_PARTIAL on skbs where csum_type is set to CSUM_T_SCTP_CRC.
  *
  *   NETIF_F_FCOE_CRC - This feature indicates that a device is capable of
  *     offloading the FCOE CRC in a packet. To perform this offload the stack
@@ -222,6 +222,9 @@
 #define CHECKSUM_COMPLETE	2
 #define CHECKSUM_PARTIAL	3
 
+#define CSUM_T_INET		0
+#define CSUM_T_SCTP_CRC		1
+
 /* Maximum value in skb->csum_level */
 #define SKB_MAX_CSUM_LEVEL	3
 
@@ -677,7 +680,7 @@ typedef unsigned char *sk_buff_data_t;
  *	@encapsulation: indicates the inner headers in the skbuff are valid
  *	@encap_hdr_csum: software checksum is needed
  *	@csum_valid: checksum is already valid
- *	@csum_not_inet: use CRC32c to resolve CHECKSUM_PARTIAL
+ *	@csum_type: do the checksum offload according to this type
  *	@csum_complete_sw: checksum was completed by software
  *	@csum_level: indicates the number of consecutive checksums found in
  *		the packet minus one that have been verified as
@@ -836,7 +839,7 @@ struct sk_buff {
 	__u8			vlan_present:1;
 	__u8			csum_complete_sw:1;
 	__u8			csum_level:2;
-	__u8			csum_not_inet:1;
+	__u8			csum_type:1;
 	__u8			dst_pending_confirm:1;
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	__u8			ndisc_nodetype:2;
@@ -4623,7 +4626,7 @@ static inline void skb_reset_redirect(struct sk_buff *skb)
 
 static inline bool skb_csum_is_sctp(struct sk_buff *skb)
 {
-	return skb->csum_not_inet;
+	return skb->csum_type == CSUM_T_SCTP_CRC;
 }
 
 static inline void skb_set_kcov_handle(struct sk_buff *skb,
diff --git a/net/core/dev.c b/net/core/dev.c
index d9ce02e..3241de2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3279,7 +3279,7 @@ int skb_crc32c_csum_help(struct sk_buff *skb)
 						  crc32c_csum_stub));
 	*(__le32 *)(skb->data + offset) = crc32c_csum;
 	skb->ip_summed = CHECKSUM_NONE;
-	skb->csum_not_inet = 0;
+	skb->csum_type = 0;
 out:
 	return ret;
 }
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4fa4fcb..9fabb6e 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -376,7 +376,7 @@ static int tcf_csum_sctp(struct sk_buff *skb, unsigned int ihl,
 	sctph->checksum = sctp_compute_cksum(skb,
 					     skb_network_offset(skb) + ihl);
 	skb->ip_summed = CHECKSUM_NONE;
-	skb->csum_not_inet = 0;
+	skb->csum_type = 0;
 
 	return 1;
 }
diff --git a/net/sctp/offload.c b/net/sctp/offload.c
index eb874e3..372f373 100644
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c
@@ -26,7 +26,7 @@
 static __le32 sctp_gso_make_checksum(struct sk_buff *skb)
 {
 	skb->ip_summed = CHECKSUM_NONE;
-	skb->csum_not_inet = 0;
+	skb->csum_type = 0;
 	/* csum and csum_start in GSO CB may be needed to do the UDP
 	 * checksum when it's a UDP tunneling packet.
 	 */
diff --git a/net/sctp/output.c b/net/sctp/output.c
index 6614c9f..a8cf0191 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -523,7 +523,7 @@ static int sctp_packet_pack(struct sctp_packet *packet,
 	} else {
 chksum:
 		head->ip_summed = CHECKSUM_PARTIAL;
-		head->csum_not_inet = 1;
+		head->csum_type = CSUM_T_SCTP_CRC;
 		head->csum_start = skb_transport_header(head) - head->head;
 		head->csum_offset = offsetof(struct sctphdr, checksum);
 	}
-- 
2.1.0

[PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type

[Xin Long]

This patch is to extend csum_type field to 2 bits, and introduce
CSUM_T_IP_GENERIC csum type, and add the support for this in
skb_csum_hwoffload_help(), just like CSUM_T_SCTP_CRC.

Note here it moves dst_pending_confirm field below ndisc_nodetype
to avoid a memory hole.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/skbuff.h |  5 +++--
 net/core/dev.c         | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 67b0a01..d5011fb 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -224,6 +224,7 @@
 
 #define CSUM_T_INET		0
 #define CSUM_T_SCTP_CRC		1
+#define CSUM_T_IP_GENERIC	2
 
 /* Maximum value in skb->csum_level */
 #define SKB_MAX_CSUM_LEVEL	3
@@ -839,11 +840,11 @@ struct sk_buff {
 	__u8			vlan_present:1;
 	__u8			csum_complete_sw:1;
 	__u8			csum_level:2;
-	__u8			csum_type:1;
-	__u8			dst_pending_confirm:1;
+	__u8			csum_type:2;
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	__u8			ndisc_nodetype:2;
 #endif
+	__u8			dst_pending_confirm:1;
 
 	__u8			ipvs_property:1;
 	__u8			inner_protocol_type:1;
diff --git a/net/core/dev.c b/net/core/dev.c
index 3241de2..6d48af2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3617,11 +3617,20 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
 int skb_csum_hwoffload_help(struct sk_buff *skb,
 			    const netdev_features_t features)
 {
-	if (unlikely(skb_csum_is_sctp(skb)))
-		return !!(features & NETIF_F_SCTP_CRC) ? 0 :
-			skb_crc32c_csum_help(skb);
+	if (likely(!skb->csum_type))
+		return !!(features & NETIF_F_CSUM_MASK) ? 0 :
+		       skb_checksum_help(skb);
 
-	return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
+	if (skb_csum_is_sctp(skb)) {
+		return !!(features & NETIF_F_SCTP_CRC) ? 0 :
+		       skb_crc32c_csum_help(skb);
+	} else if (skb->csum_type == CSUM_T_IP_GENERIC) {
+		return !!(features & NETIF_F_HW_CSUM) ? 0 :
+		       skb_checksum_help(skb);
+	} else {
+		pr_warn("Wrong csum type: %d\n", skb->csum_type);
+		return 1;
+	}
 }
 EXPORT_SYMBOL(skb_csum_hwoffload_help);
 
-- 
2.1.0

[PATCH net-next 3/3] ip_gre: add csum offload support for gre header

[Xin Long]

This patch is to add csum offload support for gre header:

On the TX path in gre_build_header(), when CHECKSUM_PARTIAL's set
for inner proto, it will calculate the csum for outer proto, and
inner csum will be offloaded later. Otherwise, CHECKSUM_PARTIAL
and csum_start/offset will be set for outer proto, and the outer
csum will be offloaded later.

On the GSO path in gre_gso_segment(), when CHECKSUM_PARTIAL is
not set for inner proto and the hardware supports csum offload,
CHECKSUM_PARTIAL and csum_start/offset will be set for outer
proto, and outer csum will be offloaded later. Otherwise, it
will do csum for outer proto by calling gso_make_checksum().

Note that SCTP has to do the csum by itself for non GSO path in
sctp_packet_pack(), as gre_build_header() can't handle the csum
with CHECKSUM_PARTIAL set for SCTP CRC csum offload.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/gre.h      | 20 ++++++++------------
 net/ipv4/gre_offload.c | 16 ++++++++++++++--
 net/sctp/output.c      |  1 +
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/include/net/gre.h b/include/net/gre.h
index b60f212..250b2fb 100644
--- a/include/net/gre.h
+++ b/include/net/gre.h
@@ -106,17 +106,6 @@ static inline __be16 gre_tnl_flags_to_gre_flags(__be16 tflags)
 	return flags;
 }
 
-static inline __sum16 gre_checksum(struct sk_buff *skb)
-{
-	__wsum csum;
-
-	if (skb->ip_summed == CHECKSUM_PARTIAL)
-		csum = lco_csum(skb);
-	else
-		csum = skb_checksum(skb, 0, skb->len, 0);
-	return csum_fold(csum);
-}
-
 static inline void gre_build_header(struct sk_buff *skb, int hdr_len,
 				    __be16 flags, __be16 proto,
 				    __be32 key, __be32 seq)
@@ -146,7 +135,14 @@ static inline void gre_build_header(struct sk_buff *skb, int hdr_len,
 		    !(skb_shinfo(skb)->gso_type &
 		      (SKB_GSO_GRE | SKB_GSO_GRE_CSUM))) {
 			*ptr = 0;
-			*(__sum16 *)ptr = gre_checksum(skb);
+			if (skb->ip_summed == CHECKSUM_PARTIAL) {
+				*(__sum16 *)ptr = csum_fold(lco_csum(skb));
+			} else {
+				skb->csum_type = CSUM_T_IP_GENERIC;
+				skb->ip_summed = CHECKSUM_PARTIAL;
+				skb->csum_start = skb_transport_header(skb) - skb->head;
+				skb->csum_offset = sizeof(*greh);
+			}
 		}
 	}
 }
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index 10bc49b..12d6996 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -15,10 +15,10 @@ 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, offload_csum, gso_partial, need_ipsec;
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
 	u16 mac_offset = skb->mac_header;
 	__be16 protocol = skb->protocol;
-	bool need_csum, gso_partial;
 	u16 mac_len = skb->mac_len;
 	int gre_offset, outer_hlen;
 
@@ -47,6 +47,11 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 	if (need_csum)
 		features &= ~NETIF_F_SCTP_CRC;
 
+	need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
+	/* Try to offload checksum if possible */
+	offload_csum = !!(need_csum && !need_ipsec &&
+			  (skb->dev->features & NETIF_F_HW_CSUM));
+
 	/* segment inner packet. */
 	segs = skb_mac_gso_segment(skb, features);
 	if (IS_ERR_OR_NULL(segs)) {
@@ -100,7 +105,14 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 		}
 
 		*(pcsum + 1) = 0;
-		*pcsum = gso_make_checksum(skb, 0);
+		if (skb->encapsulation || !offload_csum) {
+			*pcsum = gso_make_checksum(skb, 0);
+		} else {
+			skb->csum_type = CSUM_T_IP_GENERIC;
+			skb->ip_summed = CHECKSUM_PARTIAL;
+			skb->csum_start = skb_transport_header(skb) - skb->head;
+			skb->csum_offset = sizeof(*greh);
+		}
 	} while ((skb = skb->next));
 out:
 	return segs;
diff --git a/net/sctp/output.c b/net/sctp/output.c
index a8cf0191..52e12df 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -515,6 +515,7 @@ static int sctp_packet_pack(struct sctp_packet *packet,
 		return 1;
 
 	if (!(tp->dst->dev->features & NETIF_F_SCTP_CRC) ||
+	    tp->dst->dev->type == ARPHRD_IPGRE ||
 	    dst_xfrm(tp->dst) || packet->ipfragok || tp->encap_port) {
 		struct sctphdr *sh =
 			(struct sctphdr *)skb_transport_header(head);
-- 
2.1.0

Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type

[Alexander Duyck]

On Thu, Jan 21, 2021 at 12:46 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> This patch is to extend csum_type field to 2 bits, and introduce
> CSUM_T_IP_GENERIC csum type, and add the support for this in
> skb_csum_hwoffload_help(), just like CSUM_T_SCTP_CRC.
>
> Note here it moves dst_pending_confirm field below ndisc_nodetype
> to avoid a memory hole.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/linux/skbuff.h |  5 +++--
>  net/core/dev.c         | 17 +++++++++++++----
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 67b0a01..d5011fb 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -224,6 +224,7 @@
>
>  #define CSUM_T_INET            0
>  #define CSUM_T_SCTP_CRC                1
> +#define CSUM_T_IP_GENERIC      2
>
>  /* Maximum value in skb->csum_level */
>  #define SKB_MAX_CSUM_LEVEL     3
> @@ -839,11 +840,11 @@ struct sk_buff {
>         __u8                    vlan_present:1;
>         __u8                    csum_complete_sw:1;
>         __u8                    csum_level:2;
> -       __u8                    csum_type:1;
> -       __u8                    dst_pending_confirm:1;
> +       __u8                    csum_type:2;
>  #ifdef CONFIG_IPV6_NDISC_NODETYPE
>         __u8                    ndisc_nodetype:2;
>  #endif
> +       __u8                    dst_pending_confirm:1;
>
>         __u8                    ipvs_property:1;
>         __u8                    inner_protocol_type:1;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3241de2..6d48af2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3617,11 +3617,20 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
>  int skb_csum_hwoffload_help(struct sk_buff *skb,
>                             const netdev_features_t features)
>  {
> -       if (unlikely(skb_csum_is_sctp(skb)))
> -               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> -                       skb_crc32c_csum_help(skb);
> +       if (likely(!skb->csum_type))
> +               return !!(features & NETIF_F_CSUM_MASK) ? 0 :
> +                      skb_checksum_help(skb);
>
> -       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
> +       if (skb_csum_is_sctp(skb)) {
> +               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> +                      skb_crc32c_csum_help(skb);
> +       } else if (skb->csum_type == CSUM_T_IP_GENERIC) {
> +               return !!(features & NETIF_F_HW_CSUM) ? 0 :
> +                      skb_checksum_help(skb);
> +       } else {
> +               pr_warn("Wrong csum type: %d\n", skb->csum_type);
> +               return 1;
> +       }

Is the only difference between CSUM_T_IP_GENERIC the fact that we
check for NETIF_F_HW_CSUM versus using NETIF_F_CSUM_MASK? If so I
don't think adding the new bit is adding all that much value. Instead
you could probably just catch this in the testing logic here.

You could very easily just fold CSUM_T_IP_GENERIC into CSUM_T_INET,
and then in the checks here you split up the checks for
NETIF_F_HW_CSUM as follows:

 if (skb_csum_is_sctp(skb))
    return !!(features & NETIF_F_SCTP_CRC) ? 0 : skb_crc32c_csum_help(skb);

if (skb->csum_type) {
    pr_warn("Wrong csum type: %d\n", skb->csum_type);
    return 1;
}

if (features & NETIF_F_HW_CSUM)
    return 0;

if (features & NETIF_F_CSUM_MASK) {
    switch (skb->csum_offset) {
    case offsetof(struct tcphdr, check):
    case offsetof(struct udphdr, check):
            return 0;
    }
}

return skb_checksum_help(skb);

Re: [PATCH net-next 1/3] net: rename csum_not_inet to csum_type

[Jakub Kicinski]

On Thu, 21 Jan 2021 16:45:36 +0800 Xin Long wrote:
> This patch is to rename csum_not_inet to csum_type, as later
> more csum type would be introduced in the next patch.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:1073:11: error: ‘struct sk_buff’ has no member named ‘csum_not_inet’; did you mean ‘csum_offset’?
 1073 |  if (skb->csum_not_inet || skb_is_gso(skb) ||
      |           ^~~~~~~~~~~~~
      |           csum_offset

Re: [PATCH net-next 1/3] net: rename csum_not_inet to csum_type

[Xin Long]

On Fri, Jan 22, 2021 at 10:50 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 21 Jan 2021 16:45:36 +0800 Xin Long wrote:
> > This patch is to rename csum_not_inet to csum_type, as later
> > more csum type would be introduced in the next patch.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:1073:11: error: ‘struct sk_buff’ has no member named ‘csum_not_inet’; did you mean ‘csum_offset’?
>  1073 |  if (skb->csum_not_inet || skb_is_gso(skb) ||
>       |           ^~~~~~~~~~~~~
>       |           csum_offset
I will replace it with skb_csum_is_sctp(). Thanks.

Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type

[Xin Long]

On Fri, Jan 22, 2021 at 2:13 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 12:46 AM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > This patch is to extend csum_type field to 2 bits, and introduce
> > CSUM_T_IP_GENERIC csum type, and add the support for this in
> > skb_csum_hwoffload_help(), just like CSUM_T_SCTP_CRC.
> >
> > Note here it moves dst_pending_confirm field below ndisc_nodetype
> > to avoid a memory hole.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/linux/skbuff.h |  5 +++--
> >  net/core/dev.c         | 17 +++++++++++++----
> >  2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 67b0a01..d5011fb 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -224,6 +224,7 @@
> >
> >  #define CSUM_T_INET            0
> >  #define CSUM_T_SCTP_CRC                1
> > +#define CSUM_T_IP_GENERIC      2
> >
> >  /* Maximum value in skb->csum_level */
> >  #define SKB_MAX_CSUM_LEVEL     3
> > @@ -839,11 +840,11 @@ struct sk_buff {
> >         __u8                    vlan_present:1;
> >         __u8                    csum_complete_sw:1;
> >         __u8                    csum_level:2;
> > -       __u8                    csum_type:1;
> > -       __u8                    dst_pending_confirm:1;
> > +       __u8                    csum_type:2;
> >  #ifdef CONFIG_IPV6_NDISC_NODETYPE
> >         __u8                    ndisc_nodetype:2;
> >  #endif
> > +       __u8                    dst_pending_confirm:1;
> >
> >         __u8                    ipvs_property:1;
> >         __u8                    inner_protocol_type:1;
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3241de2..6d48af2 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3617,11 +3617,20 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
> >  int skb_csum_hwoffload_help(struct sk_buff *skb,
> >                             const netdev_features_t features)
> >  {
> > -       if (unlikely(skb_csum_is_sctp(skb)))
> > -               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> > -                       skb_crc32c_csum_help(skb);
> > +       if (likely(!skb->csum_type))
> > +               return !!(features & NETIF_F_CSUM_MASK) ? 0 :
> > +                      skb_checksum_help(skb);
> >
> > -       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
> > +       if (skb_csum_is_sctp(skb)) {
> > +               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> > +                      skb_crc32c_csum_help(skb);
> > +       } else if (skb->csum_type == CSUM_T_IP_GENERIC) {
> > +               return !!(features & NETIF_F_HW_CSUM) ? 0 :
> > +                      skb_checksum_help(skb);
> > +       } else {
> > +               pr_warn("Wrong csum type: %d\n", skb->csum_type);
> > +               return 1;
> > +       }
>
> Is the only difference between CSUM_T_IP_GENERIC the fact that we
> check for NETIF_F_HW_CSUM versus using NETIF_F_CSUM_MASK? If so I
> don't think adding the new bit is adding all that much value. Instead
> you could probably just catch this in the testing logic here.
>
> You could very easily just fold CSUM_T_IP_GENERIC into CSUM_T_INET,
> and then in the checks here you split up the checks for
> NETIF_F_HW_CSUM as follows:
If so, better not to touch csum_not_inet now. I will drop the patch 1/3.

>
>  if (skb_csum_is_sctp(skb))
>     return !!(features & NETIF_F_SCTP_CRC) ? 0 : skb_crc32c_csum_help(skb);
>
> if (skb->csum_type) {
>     pr_warn("Wrong csum type: %d\n", skb->csum_type);
>     return 1;
> }
>
> if (features & NETIF_F_HW_CSUM)
>     return 0;
>
> if (features & NETIF_F_CSUM_MASK) {
>     switch (skb->csum_offset) {
>     case offsetof(struct tcphdr, check):
>     case offsetof(struct udphdr, check):
>             return 0;
>     }
Question is: is it reliable to check the type by skb->csum_offset?
What if one day there's another protocol, whose the checksum field
is on the same offset, which is also using the CSUM_T_IP_GENERIC?

> }
>
> return skb_checksum_help(skb);

Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type

[Alexander Duyck]

On Thu, Jan 21, 2021 at 7:18 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 2:13 AM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Thu, Jan 21, 2021 at 12:46 AM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > This patch is to extend csum_type field to 2 bits, and introduce
> > > CSUM_T_IP_GENERIC csum type, and add the support for this in
> > > skb_csum_hwoffload_help(), just like CSUM_T_SCTP_CRC.
> > >
> > > Note here it moves dst_pending_confirm field below ndisc_nodetype
> > > to avoid a memory hole.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  include/linux/skbuff.h |  5 +++--
> > >  net/core/dev.c         | 17 +++++++++++++----
> > >  2 files changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > index 67b0a01..d5011fb 100644
> > > --- a/include/linux/skbuff.h
> > > +++ b/include/linux/skbuff.h
> > > @@ -224,6 +224,7 @@
> > >
> > >  #define CSUM_T_INET            0
> > >  #define CSUM_T_SCTP_CRC                1
> > > +#define CSUM_T_IP_GENERIC      2
> > >
> > >  /* Maximum value in skb->csum_level */
> > >  #define SKB_MAX_CSUM_LEVEL     3
> > > @@ -839,11 +840,11 @@ struct sk_buff {
> > >         __u8                    vlan_present:1;
> > >         __u8                    csum_complete_sw:1;
> > >         __u8                    csum_level:2;
> > > -       __u8                    csum_type:1;
> > > -       __u8                    dst_pending_confirm:1;
> > > +       __u8                    csum_type:2;
> > >  #ifdef CONFIG_IPV6_NDISC_NODETYPE
> > >         __u8                    ndisc_nodetype:2;
> > >  #endif
> > > +       __u8                    dst_pending_confirm:1;
> > >
> > >         __u8                    ipvs_property:1;
> > >         __u8                    inner_protocol_type:1;
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 3241de2..6d48af2 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -3617,11 +3617,20 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
> > >  int skb_csum_hwoffload_help(struct sk_buff *skb,
> > >                             const netdev_features_t features)
> > >  {
> > > -       if (unlikely(skb_csum_is_sctp(skb)))
> > > -               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> > > -                       skb_crc32c_csum_help(skb);
> > > +       if (likely(!skb->csum_type))
> > > +               return !!(features & NETIF_F_CSUM_MASK) ? 0 :
> > > +                      skb_checksum_help(skb);
> > >
> > > -       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
> > > +       if (skb_csum_is_sctp(skb)) {
> > > +               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> > > +                      skb_crc32c_csum_help(skb);
> > > +       } else if (skb->csum_type == CSUM_T_IP_GENERIC) {
> > > +               return !!(features & NETIF_F_HW_CSUM) ? 0 :
> > > +                      skb_checksum_help(skb);
> > > +       } else {
> > > +               pr_warn("Wrong csum type: %d\n", skb->csum_type);
> > > +               return 1;
> > > +       }
> >
> > Is the only difference between CSUM_T_IP_GENERIC the fact that we
> > check for NETIF_F_HW_CSUM versus using NETIF_F_CSUM_MASK? If so I
> > don't think adding the new bit is adding all that much value. Instead
> > you could probably just catch this in the testing logic here.
> >
> > You could very easily just fold CSUM_T_IP_GENERIC into CSUM_T_INET,
> > and then in the checks here you split up the checks for
> > NETIF_F_HW_CSUM as follows:
> If so, better not to touch csum_not_inet now. I will drop the patch 1/3.
>
> >
> >  if (skb_csum_is_sctp(skb))
> >     return !!(features & NETIF_F_SCTP_CRC) ? 0 : skb_crc32c_csum_help(skb);
> >
> > if (skb->csum_type) {
> >     pr_warn("Wrong csum type: %d\n", skb->csum_type);
> >     return 1;
> > }
> >
> > if (features & NETIF_F_HW_CSUM)
> >     return 0;
> >
> > if (features & NETIF_F_CSUM_MASK) {
> >     switch (skb->csum_offset) {
> >     case offsetof(struct tcphdr, check):
> >     case offsetof(struct udphdr, check):
> >             return 0;
> >     }
> Question is: is it reliable to check the type by skb->csum_offset?
> What if one day there's another protocol, whose the checksum field
> is on the same offset, which is also using the CSUM_T_IP_GENERIC?

I'd say we are better off crossing that bridge once we get there. For
now we only have a few L4 protocols that are requesting Tx checksum
offload, and I suspect that in many cases they probably won't care
about the actual protocol when it comes to the checksum since the L4
checksum is usually pretty straight forward with it consisting of just
needing to know the start of the transport header and the offset to
place it at based on the protocol.