On Tue, Sep 8, 2020 at 4:32 AM tanhuazhong <tanhuazh...@huawei.com> wrote: > > > > On 2020/9/7 23:35, Willem de Bruijn wrote: > > On Mon, Sep 7, 2020 at 3:38 PM tanhuazhong <tanhuazh...@huawei.com> wrote: > >> > >> > >> > >> On 2020/9/7 17:22, Willem de Bruijn wrote: > >>> On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <k...@kernel.org> 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-tanhuazh...@huawei.com/) > > 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. > > > > > > . > > >
