* [PATCH net-next 0/3] net: add support for ip generic checksum offload for gre @ 2021-01-21 8:45 Xin Long 2021-01-21 8:45 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Xin Long 0 siblings, 1 reply; 9+ messages in thread From: Xin Long @ 2021-01-21 8:45 UTC (permalink / raw) To: network dev Cc: Marcelo Ricardo Leitner, Davide Caratti, davem, Jakub Kicinski, Alexander Duyck 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 1/3] net: rename csum_not_inet to csum_type 2021-01-21 8:45 [PATCH net-next 0/3] net: add support for ip generic checksum offload for gre Xin Long @ 2021-01-21 8:45 ` Xin Long 2021-01-21 8:45 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Xin Long 2021-01-22 2:50 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Jakub Kicinski 0 siblings, 2 replies; 9+ messages in thread From: Xin Long @ 2021-01-21 8:45 UTC (permalink / raw) To: network dev Cc: Marcelo Ricardo Leitner, Davide Caratti, davem, Jakub Kicinski, Alexander Duyck 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type 2021-01-21 8:45 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Xin Long @ 2021-01-21 8:45 ` Xin Long 2021-01-21 8:45 ` [PATCH net-next 3/3] ip_gre: add csum offload support for gre header Xin Long 2021-01-21 18:13 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Alexander Duyck 2021-01-22 2:50 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Jakub Kicinski 1 sibling, 2 replies; 9+ messages in thread From: Xin Long @ 2021-01-21 8:45 UTC (permalink / raw) To: network dev Cc: Marcelo Ricardo Leitner, Davide Caratti, davem, Jakub Kicinski, Alexander Duyck 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 3/3] ip_gre: add csum offload support for gre header 2021-01-21 8:45 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Xin Long @ 2021-01-21 8:45 ` Xin Long 2021-01-21 18:13 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Alexander Duyck 1 sibling, 0 replies; 9+ messages in thread From: Xin Long @ 2021-01-21 8:45 UTC (permalink / raw) To: network dev Cc: Marcelo Ricardo Leitner, Davide Caratti, davem, Jakub Kicinski, Alexander Duyck 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type 2021-01-21 8:45 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Xin Long 2021-01-21 8:45 ` [PATCH net-next 3/3] ip_gre: add csum offload support for gre header Xin Long @ 2021-01-21 18:13 ` Alexander Duyck 2021-01-22 3:18 ` Xin Long 1 sibling, 1 reply; 9+ messages in thread From: Alexander Duyck @ 2021-01-21 18:13 UTC (permalink / raw) To: Xin Long Cc: network dev, Marcelo Ricardo Leitner, Davide Caratti, David Miller, Jakub Kicinski 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type 2021-01-21 18:13 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Alexander Duyck @ 2021-01-22 3:18 ` Xin Long 2021-01-22 16:17 ` Alexander Duyck 0 siblings, 1 reply; 9+ messages in thread From: Xin Long @ 2021-01-22 3:18 UTC (permalink / raw) To: Alexander Duyck Cc: network dev, Marcelo Ricardo Leitner, Davide Caratti, David Miller, Jakub Kicinski 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type 2021-01-22 3:18 ` Xin Long @ 2021-01-22 16:17 ` Alexander Duyck 0 siblings, 0 replies; 9+ messages in thread From: Alexander Duyck @ 2021-01-22 16:17 UTC (permalink / raw) To: Xin Long Cc: network dev, Marcelo Ricardo Leitner, Davide Caratti, David Miller, Jakub Kicinski 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] net: rename csum_not_inet to csum_type 2021-01-21 8:45 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Xin Long 2021-01-21 8:45 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Xin Long @ 2021-01-22 2:50 ` Jakub Kicinski 2021-01-22 3:08 ` Xin Long 1 sibling, 1 reply; 9+ messages in thread From: Jakub Kicinski @ 2021-01-22 2:50 UTC (permalink / raw) To: Xin Long Cc: network dev, Marcelo Ricardo Leitner, Davide Caratti, davem, Alexander Duyck 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] net: rename csum_not_inet to csum_type 2021-01-22 2:50 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Jakub Kicinski @ 2021-01-22 3:08 ` Xin Long 0 siblings, 0 replies; 9+ messages in thread From: Xin Long @ 2021-01-22 3:08 UTC (permalink / raw) To: Jakub Kicinski Cc: network dev, Marcelo Ricardo Leitner, Davide Caratti, davem, Alexander Duyck 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-22 16:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-01-21 8:45 [PATCH net-next 0/3] net: add support for ip generic checksum offload for gre Xin Long 2021-01-21 8:45 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Xin Long 2021-01-21 8:45 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Xin Long 2021-01-21 8:45 ` [PATCH net-next 3/3] ip_gre: add csum offload support for gre header Xin Long 2021-01-21 18:13 ` [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type Alexander Duyck 2021-01-22 3:18 ` Xin Long 2021-01-22 16:17 ` Alexander Duyck 2021-01-22 2:50 ` [PATCH net-next 1/3] net: rename csum_not_inet to csum_type Jakub Kicinski 2021-01-22 3:08 ` Xin Long
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).