| Message ID | 1599286273-26553-1-git-send-email-tanhuazhong@huawei.com |
|---|---|
| Headers | show |
| Series | net: two updates related to UDP GSO | expand |
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?
On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <kuba@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.
On 2020/9/7 17:22, Willem de Bruijn wrote: > On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <kuba@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. [root@localhost ~]# ethtool -K eth0 tx off Actual changes: tx-checksumming: off tx-checksum-ipv4: off tx-checksum-ipv6: off tx-checksum-sctp: off tcp-segmentation-offload: off tx-tcp-segmentation: off [requested on] tx-tcp-ecn-segmentation: off [requested on] tx-tcp6-segmentation: off [requested on] [root@localhost ~]# ethtool -k eth0 Features for eth0: rx-checksumming: on tx-checksumming: off tx-checksum-ipv4: off tx-checksum-ip-generic: off [fixed] tx-checksum-ipv6: off tx-checksum-fcoe-crc: off [fixed] tx-checksum-sctp: off ... tcp-segmentation-offload: off tx-tcp-segmentation: off [requested on] tx-tcp-ecn-segmentation: off [requested on] tx-tcp-mangleid-segmentation: off tx-tcp6-segmentation: off [requested on] udp-fragmentation-offload: off generic-segmentation-offload: on generic-receive-offload: on ... tx-udp-segmentation: on ... .ndo_feature_check seems unnecessary. Because the stack has already do this check. in __ip_append_data() below if branch will not run if hardware checksum offload is off. if (transhdrlen && length + fragheaderlen <= mtu && rt->dst.dev->features & (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM) && (!(flags & MSG_MORE) || cork->gso_size) && (!exthdrlen || (rt->dst.dev->features & NETIF_F_HW_ESP_TX_CSUM))) csummode = CHECKSUM_PARTIAL; ... so skb->ip_summed set as CHECKSUM_NONE, then in udp_send_skb() if (cork->gso_size) { ... if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite || dst_xfrm(skb_dst(skb))) { kfree_skb(skb); return -EIO; } the packet who needs udp gso will return ERROR. For this kind of problem how could we fix it? Thanks. Huazhong. > . >
On Mon, Sep 7, 2020 at 3:38 PM tanhuazhong <tanhuazhong@huawei.com> wrote: > > > > On 2020/9/7 17:22, Willem de Bruijn wrote: > > On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <kuba@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. Alternatively, the real value of splitting the type is in advertising the features separately through ethtool. That requires additional changes.
On 2020/9/7 23:35, Willem de Bruijn wrote: > On Mon, Sep 7, 2020 at 3:38 PM tanhuazhong <tanhuazhong@huawei.com> wrote: >> >> >> >> On 2020/9/7 17:22, Willem de Bruijn wrote: >>> On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <kuba@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-tanhuazhong@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. > Alternatively, the real value of splitting the type is in advertising > the features separately through ethtool. That requires additional > changes. > > . >
On Tue, Sep 8, 2020 at 4:32 AM tanhuazhong <tanhuazhong@huawei.com> wrote: > > > > On 2020/9/7 23:35, Willem de Bruijn wrote: > > On Mon, Sep 7, 2020 at 3:38 PM tanhuazhong <tanhuazhong@huawei.com> wrote: > >> > >> > >> > >> On 2020/9/7 17:22, Willem de Bruijn wrote: > >>> On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski <kuba@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-tanhuazhong@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. > > > > > > . > > >