| Message ID | cover.1611218673.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: add support for ip generic checksum offload for gre | expand |
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);
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);
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.