Re: [PATCH net-next 1/5] net: add a common function to compute features from lowers devices

[Sabrina Dubroca <sd@queasysnail.net>]

2025-09-10, 20:08:03 +0300, Ido Schimmel wrote:
> On Wed, Sep 10, 2025 at 04:29:35PM +0200, Sabrina Dubroca wrote:
> > 2025-08-31, 18:35:49 +0300, Ido Schimmel wrote:
> > > On Fri, Aug 29, 2025 at 09:54:26AM +0000, Hangbin Liu wrote:
> > > > Some high level virtual drivers need to compute features from lower
> > > > devices. But each has their own implementations and may lost some
> > > > feature compute. Let's use one common function to compute features
> > > > for kinds of these devices.
> > > > 
> > > > The new helper uses the current bond implementation as the reference
> > > > one, as the latter already handles all the relevant aspects: netdev
> > > > features, TSO limits and dst retention.
> > > > 
> > > > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > > > ---
> > > >  include/linux/netdevice.h | 19 ++++++++++
> > > >  net/core/dev.c            | 79 +++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 98 insertions(+)
> > > > 
> > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > index f3a3b761abfb..42742a47f2c6 100644
> > > > --- a/include/linux/netdevice.h
> > > > +++ b/include/linux/netdevice.h
> > > > @@ -5279,6 +5279,25 @@ int __netdev_update_features(struct net_device *dev);
> > > >  void netdev_update_features(struct net_device *dev);
> > > >  void netdev_change_features(struct net_device *dev);
> > > >  
> > > > +/* netdevice features */
> > > > +#define VIRTUAL_DEV_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> > > > +					 NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \
> > > > +					 NETIF_F_GSO_ENCAP_ALL | \
> > > > +					 NETIF_F_HIGHDMA | NETIF_F_LRO)
> > > > +
> > > > +#define VIRTUAL_DEV_ENC_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> > > > +					 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
> > > > +					 NETIF_F_GSO_PARTIAL)
> > > > +
> > > > +#define VIRTUAL_DEV_MPLS_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> > > > +					 NETIF_F_GSO_SOFTWARE)
> > > > +
> > > > +#define VIRTUAL_DEV_XFRM_FEATURES	(NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
> > > > +					 NETIF_F_GSO_ESP)
> > > > +
> > > > +#define VIRTUAL_DEV_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
> > > > +void netdev_compute_features_from_lowers(struct net_device *dev);
> > > > +
> > > >  void netif_stacked_transfer_operstate(const struct net_device *rootdev,
> > > >  					struct net_device *dev);
> > > >  
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index 1d1650d9ecff..fcad2a9f6b65 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -12577,6 +12577,85 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
> > > >  }
> > > >  EXPORT_SYMBOL(netdev_increment_features);
> > > >  
> > > > +/**
> > > > + *	netdev_compute_features_from_lowers - compute feature from lowers
> > > > + *	@dev: the upper device
> > > > + *
> > > > + *	Recompute the upper device's feature based on all lower devices.
> > > > + */
> > > > +void netdev_compute_features_from_lowers(struct net_device *dev)
> > > > +{
> > > > +	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
> > > > +	netdev_features_t gso_partial_features = VIRTUAL_DEV_GSO_PARTIAL_FEATURES;
> > > > +#ifdef CONFIG_XFRM_OFFLOAD
> > > > +	netdev_features_t xfrm_features  = VIRTUAL_DEV_XFRM_FEATURES;
> > >                                        ^ double space (in other places as well)
> > > 
> > > > +#endif
> > > > +	netdev_features_t mpls_features  = VIRTUAL_DEV_MPLS_FEATURES;
> > > > +	netdev_features_t vlan_features = VIRTUAL_DEV_VLAN_FEATURES;
> > > > +	netdev_features_t enc_features  = VIRTUAL_DEV_ENC_FEATURES;
> > > > +	unsigned short max_hard_header_len = ETH_HLEN;
> > 
> > Going back to this discussion about hard_header_len:
> > 
> > > hard_header_len is not really a feature, so does not sound like it
> > > belongs here. I'm pretty sure it's not needed at all.
> > > 
> > > It was added to the bond driver in 2006 by commit 54ef31371407 ("[PATCH]
> > > bonding: Handle large hard_header_len") citing panics with gianfar on
> > > xmit. In 2009 commit 93c1285c5d92 ("gianfar: reallocate skb when
> > > headroom is not enough for fcb") fixed the gianfar driver to stop
> > > assuming that it has enough room to push its custom header. Further,
> > > commit bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len")
> > > from 2012 fixed this driver to use needed_headroom instead of
> > > hard_header_len.
> > > 
> > > The team driver is also adjusting hard_header_len according to the lower
> > > devices, but it most likely copied it from the bond driver. On the other
> > > hand, the bridge driver does not mess with hard_header_len and no
> > > problems were reported there (that I know of).
> > > 
> > > Might be a good idea to remove this hard_header_len logic from bond and
> > > team and instead set their needed_headroom according to the lower device
> > > with the highest needed_headroom. Paolo added similar logic in bridge
> > > and ovs but the use case is a bit different there.
> > 
> > I'm not convinced removing adapting hard_header_len on bond/team is
> > correct, even with old and broken drivers getting fixed years
> > ago. hard_header_len will be used on the TX path (for some devices
> > like bridge/macvlan via dev_forward_skb() and similar helpers, for IP
> > tunnels setting their MTU, and via LL_RESERVED_SPACE).
> > 
> > So I think we should keep setting hard_header_len to the largest of
> > all lowers.
> 
> It is not clear to me why we are setting hard_header_len to the largest
> of all lowers and not needed_headroom. While bond/team allow
> non-Ethernet lowers (unlike bridge, which is also adjusted to use this
> helper), they do verify that all the lower devices are of the same type.
> Shouldn't devices of the same type have the same hardware header length?

At least not with VLANs. Both basic ethernet and vlan devices are
ARPHRD_ETHER, but the hard_header_len of the vlan device will be
larger if we're not offloading:

    dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;


> On the other hand, needed_headroom can and does vary between devices of
> the same type.

I'm not saying anything about needed_headroom. It sounds like it
should be updated as well.

-- 
Sabrina