Re: [PATCH net-next 0/2] net: two updates related to UDP GSO

[Willem de Bruijn]

On Tue, Sep 8, 2020 at 4:32 AM tanhuazhong <tanhuazhong@xxxxxxxxxx> wrote:
>
>
>
> On 2020/9/7 23:35, Willem de Bruijn wrote:
> > On Mon, Sep 7, 2020 at 3:38 PM tanhuazhong <tanhuazhong@xxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2020/9/7 17:22, Willem de Bruijn wrote:
> >>> On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> >>>>
> >>>> On Sat, 5 Sep 2020 14:11:11 +0800 Huazhong Tan wrote:
> >>>>> There are two updates relates to UDP GSO.
> >>>>> #1 adds a new GSO type for UDPv6
> >>>>> #2 adds check for UDP GSO when csum is disable in netdev_fix_features().
> >>>>>
> >>>>> Changes since RFC V2:
> >>>>> - modifies the timing of setting UDP GSO type when doing UDP GRO in #1.
> >>>>>
> >>>>> Changes since RFC V1:
> >>>>> - updates NETIF_F_GSO_LAST suggested by Willem de Bruijn.
> >>>>>     and add NETIF_F_GSO_UDPV6_L4 feature for each driver who support UDP GSO in #1.
> >>>>>     - add #2 who needs #1.
> >>>>
> >>>> Please CC people who gave you feedback (Willem).
> >>>>
> >>>> I don't feel good about this series. IPv6 is not optional any more.
> >>>> AFAIU you have some issues with csum support in your device? Can you
> >>>> use .ndo_features_check() to handle this?
> >>>>
> >>>> The change in semantics of NETIF_F_GSO_UDP_L4 from "v4 and v6" to
> >>>> "just v4" can trip people over; this is not a new feature people
> >>>> may be depending on the current semantics.
> >>>>
> >>>> Willem, what are your thoughts on this?
> >>>
> >>> If that is the only reason, +1 on fixing it up in the driver's
> >>> ndo_features_check.
> >>>
> >>
> >> Hi, Willem & Jakub.
> >>
> >> This series mainly fixes the feature dependency between hardware
> >> checksum and UDP GSO.
> >> When turn off hardware checksum offload, run 'ethtool -k [devname]'
> >> we can see TSO is off as well, but udp gso still is on.
> >
> > I see. That does not entirely require separate IPv4 and IPv6 flags. It
> > can be disabled if either checksum offload is disabled. I'm not aware
> > of any hardware that only supports checksum offload for one of the two
> > network protocols.
> >
>
> below patch is acceptable? i have sent this patch before
> (https://patchwork.ozlabs.org/project/netdev/patch/1594180136-15912-3-git-send-email-tanhuazhong@xxxxxxxxxx/)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c02bae9..dcb6b35 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9095,6 +9095,12 @@ static netdev_features_t
> netdev_fix_features(struct net_device *dev,
>                 features &= ~NETIF_F_TSO6;
>         }
>
> +       if ((features & NETIF_F_GSO_UDP_L4) && !(features & NETIF_F_HW_CSUM) &&
> +           (!(features & NETIF_F_IP_CSUM) || !(features & NETIF_F_IPV6_CSUM))) {
> +               netdev_dbg(dev, "Dropping UDP GSO features since no CSUM feature.\n");
> +               features &= ~NETIF_F_GSO_UDP_L4;
> +       }
> +
>         /* TSO with IPv4 ID mangling requires IPv4 TSO be enabled */
>         if ((features & NETIF_F_TSO_MANGLEID) && !(features & NETIF_F_TSO))
>                 features &= ~NETIF_F_TSO_MANGLEID;
>
> As Eric Dumazet commented "This would prevent a device providing IPv4
> checksum only (no IPv6 csum support) from sending IPv4 UDP GSO packets ?",
> so i send this series to decouple them. Is there any good ways to
> shuttle this issue? Or as you said there is not device only support
> checksum offload for one of the two network protocols.

As he pointed out

> This could be done in an ndo_fix_features(), or ndo_features_check()
>
> Or maybe we do not care, but this should probably be documented.

I am not aware of devices that only checksum one of the protocols (but
haven't searched for a counter-example).

This sounds fine to me, with a comment to that effect, as Eric suggested.

>
> > Alternatively, the real value of splitting the type is in advertising
> > the features separately through ethtool. That requires additional
> > changes.
> >
>
>
> > .
> >
>