Re: LRO: creating vlan subports affects parent port's LRO settings

[Limin Wang <lwang.nbl@gmail.com>]

Thanks to Jakub, Michal and Jarod for the time and consideration. I
have been caught up with other stuff, and not spent much time on this
since.

I am not that familiar with the upper/lower sync logic and
implications. But I agree with Michal,  it may not be necessary to
have a blanket exclusion of certain type of devices in those sync
functions.

I was thinking something maybe in vlan_dev.c:

static int vlan_dev_init(struct net_device *dev)
{
struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;

netif_carrier_off(dev);

/* IFF_BROADCAST|IFF_MULTICAST; ??? */
dev->flags  = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI |
 IFF_MASTER | IFF_SLAVE);
dev->state  = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
 (1<<__LINK_STATE_DORMANT))) |
     (1<<__LINK_STATE_PRESENT);

dev->hw_features = NETIF_F_HW_CSUM | NETIF_F_SG |
  NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE |
  NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
  NETIF_F_ALL_FCOE;

here, maybe check if real_dev's NETIF_F_UPPER_DISABLES features bits
are set, and add them to vlan_dev?

On Sun, Dec 6, 2020 at 11:49 AM Michal Kubecek <mkubecek@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 <kuba@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.
>
> Michal