On Fri, Jan 22, 2021 at 2:13 AM Alexander Duyck
<alexander.du...@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 12:46 AM Xin Long <lucien....@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....@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);
