Date:   Sun, 6 Dec 2020 22:19:56 -0500
From:   Limin Wang <lwang.nbl@...il.com>
To:     Jarod Wilson <jarod@...hat.com>
Cc:     Michal Kubecek <mkubecek@...e.cz>, Netdev <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: LRO: creating vlan subports affects parent port's LRO settings

I might be wrong. One potential issue I found in
netdev_sync_upper_features() is that it depends on the wanted_feature
of upper_dev

if (!(upper->wanted_features & feature)
   && (features & feature)) {
netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
  &feature, upper->name);
features &= ~feature;
}
}
Suppose a new vlan device will have the LRO bit in its features
because lower_dev (real_dev) supports LRO ( assuming with proposed
changes above), if the vlan_dev's wanted_feature doesn't include LRO,
the NETIF_F_LRO may still be dropped due to this.
One could manually use "ethtool -K vlan_dev lro on" to enable LRO in
the subport's wanted_features, but that has to be done on all
vlan_dev's of the same real_dev. (it is not uncommon that a parent
port may have hundreds of vlan subports)
Does that mean the vlan_dev->wanted_feature has to include LRO bit at
creation time to avoid explicitly setting later on for each and every
vlan subports?

On Sun, Dec 6, 2020 at 5:58 PM Jarod Wilson <jarod@...hat.com> wrote:
>
> On Sun, Dec 6, 2020 at 11:49 AM Michal Kubecek <mkubecek@...e.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 <kuba@...nel.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
> jarod@...hat.com
>

Powered by blists - more mailing lists