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

[Alexander Duyck <alexander.duyck@gmail.com>]

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.