2017-06-30, 17:50:01 +0200, Jiri Benc wrote:
> On Fri, 30 Jun 2017 15:19:45 +0200, Sabrina Dubroca wrote:
> > This adds a new netdevice feature, so that tunnel offloading can be
> > disabled by the administrator on some netdevices, using the
> > "tunnel-offload" ethtool feature.
> >
> > This feature is set for all devices that provide ndo_udp_tunnel_add.
>
> This patchset looks great, Sabrina! A few comments below.
>
> > --- a/include/linux/netdev_features.h
> > +++ b/include/linux/netdev_features.h
> > @@ -76,6 +76,7 @@ enum {
> > NETIF_F_HW_TC_BIT, /* Offload TC infrastructure */
> > NETIF_F_HW_ESP_BIT, /* Hardware ESP transformation offload
> > */
> > NETIF_F_HW_ESP_TX_CSUM_BIT, /* ESP with TX checksum offload */
> > + NETIF_F_TUNNEL_OFFLOAD_BIT, /* Tunnel offloads */
>
> "Tunnel offload" is very broad. Could we be more specific, e.g. "RSS
> tunnel offload" or such? NETIF_F_HW_TUNNEL_RSS?
Yeah, "tunnel offload" is too broad. But RSS isn't correct here, since
that's not the only thing that devices do with that feature.
Maybe "udp tunnel rx port" instead? Although, as Hannes pointed out
yesterday, we're not really sure that drivers/devices will only use it
for RX purposes either.
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -106,6 +106,7 @@ static const char
> > netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]
> > [NETIF_F_HW_TC_BIT] = "hw-tc-offload",
> > [NETIF_F_HW_ESP_BIT] = "esp-hw-offload",
> > [NETIF_F_HW_ESP_TX_CSUM_BIT] = "esp-tx-csum-hw-offload",
> > + [NETIF_F_TUNNEL_OFFLOAD_BIT] = "tunnel-offload",
> > };
>
> And here, too. "rss-tunnel-offload"?
Yep, I'll update all that when we figure out a reasonable name.
Another thing I was thinking about would be to modify ethtool to show
these UDP tunneling features as a hierarchy (like we already have for
tx-checksumming). Both for this new feature and the existing
tx-udp_tnl-*.
Thanks for the comments,
--
Sabrina
