On Sun, Dec 6, 2020 at 11:49 AM Michal Kubecek <mkube...@suse.cz> wrote:
>
> On Sat, Dec 05, 2020 at 07:04:06PM -0500, Jarod Wilson wrote:
> > On Mon, Nov 23, 2020 at 7:27 PM Jakub Kicinski <k...@kernel.org> wrote:
> > >
> > > On Thu, 19 Nov 2020 20:37:27 -0500 Limin Wang wrote:
> > > > Under relatively recent kernels (v4.4+), creating a vlan subport on a
> > > > LRO supported parent NIC may turn LRO off on the parent port and
> > > > further render its LRO feature practically unchangeable.
> > >
> > > That does sound like an oversight in commit fd867d51f889 ("net/core:
> > > generic support for disabling netdev features down stack").
> > >
> > > Are you able to create a patch to fix this?
> >
> > Something like this, perhaps? Completely untested copy-pasta'd
> > theoretical patch:
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 8588ade790cb..a5ce372e02ba 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9605,8 +9605,10 @@ int __netdev_update_features(struct net_device *dev)
> > features = netdev_fix_features(dev, features);
> >
> > /* some features can't be enabled if they're off on an upper device
> > */
> > - netdev_for_each_upper_dev_rcu(dev, upper, iter)
> > - features = netdev_sync_upper_features(dev, upper, features);
> > + netdev_for_each_upper_dev_rcu(dev, upper, iter) {
> > + if (netif_is_lag_master(upper) ||
> > netif_is_bridge_master(upper))
> > + features = netdev_sync_upper_features(dev,
> > upper, features);
> > + }
> >
> > if (dev->features == features)
> > goto sync_lower;
> > @@ -9633,8 +9635,10 @@ int __netdev_update_features(struct net_device *dev)
> > /* some features must be disabled on lower devices when disabled
> > * on an upper device (think: bonding master or bridge)
> > */
> > - netdev_for_each_lower_dev(dev, lower, iter)
> > - netdev_sync_lower_features(dev, lower, features);
> > + if (netif_is_lag_master(dev) || netif_is_bridge_master(dev)) {
> > + netdev_for_each_lower_dev(dev, lower, iter)
> > + netdev_sync_lower_features(dev, lower, features);
> > + }
> >
> > if (!err) {
> > netdev_features_t diff = features ^ dev->features;
> >
> > I'm not sure what all other upper devices this excludes besides just
> > vlan ports though, so perhaps safer add upper device types to not do
> > feature sync on than to choose which ones to do them on?
>
> I'm not sure excluding devices from feature sync is the right way,
> whether it's an explicit list types or default. The logic still makes
> sense to me. Couldn't we address the issue by either setting features in
> NETIF_F_UPPER_DISABLES) by default for a new vlan (and probably macvlan)
> device? Or perhaps inheriting their values from the lower device.
Yeah, I think you're right, excluding devices entirely from sync is a
bad idea, it should be only certain features that don't get sync'd for
devices that say they don't want them (i.e., vlan devs and macvlan
devs). I'll do a bit more reading of the code and ponder. I'm not
familiar with the intricacies of NETIF_F_UPPER_DISABLES just yet.
--
Jarod Wilson
ja...@redhat.com
