All of lore.kernel.org
 help / color / mirror / Atom feed
* LRO: creating vlan subports affects parent port's LRO settings
@ 2020-11-20  1:37 Limin Wang
  2020-11-24  0:26 ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Limin Wang @ 2020-11-20  1:37 UTC (permalink / raw)
  To: netdev

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.

This can be easily reproduced on different distros, and independent of
NIC vendors.
Hopefully, this is not a repeat post of a known issue.

Below example is on Ubuntu 18.04 LTS. (Centos-7.6 is slightly
different, but the end result is the same, will attach in the end)
===========================================================================
# Ubuntu 18.04 LTS
root@server1:# uname -a
Linux server1 4.15.0-20-generic #21-Ubuntu SMP Tue Apr 24 06:16:15 UTC
2018 x86_64 x86_64 x86_64 GNU/Linux

# mellanox NIC
root@server1:# /sbin/ethtool -i ens4f0
driver: mlx5_core
version: 5.0-2.1.8

# enable LRO on the NIC
root@server1:# /sbin/ethtool -k ens4f0 | grep large
large-receive-offload: off
root@server1:# /sbin/ethtool -K ens4f0 lro on
root@server1:# /sbin/ethtool -k ens4f0 | grep large
large-receive-offload: on

# create a vlan subport, once subport is up, parent port LRO is disabled
root@server1:# ip link add link ens4f0 name ens4f0.50 type vlan id 50
root@server1:# ifconfig ens4f0.50 up
root@server1:# ethtool -k ens4f0.50 | grep large
large-receive-offload: off [fixed]
root@server1:# ethtool -k ens4f0 | grep large
large-receive-offload: off

# manually enabling LRO on parent port not working any more
root@server1:# /sbin/ethtool -K ens4f0 lro on
Could not change any device features
root@server1:# /sbin/ethtool -K ens4f0.50 lro on
Cannot change large-receive-offload
Could not change any device features
root@server1:# /sbin/ethtool -K ens4f0 lro on
Could not change any device features
root@server1:# ethtool -k ens4f0 | grep large
large-receive-offload: off [requested on]

# Now the only way to re-enable LRO on the parent port is to remove the subport
root@server1:# ip link del ens4f0.50
root@server1:# /sbin/ethtool -k ens4f0 | grep large
large-receive-offload: off [requested on]
root@server1:# /sbin/ethtool -K ens4f0 lro on
root@server1:# ethtool -k ens4f0 | grep large
large-receive-offload: on
===========================================================================

Although LRO may have different implications or issues in practice,
this seems a simple use case expected to work?--enabling LRO on the
physical NIC and also having vlans on the same NIC port.
Note, here both the parent port and the vlan subport are not attached
to any bridge, bond, team or ovs devices, just standalone.

This issue seems not driver or distro related, and lies in the kernel
network stack.
When changing netdev features, (via either userspace ethtool, or other
in-kernel processing), in the end:
__netdev_update_features() does the job and calls
netdev_sync_upper_features() and netdev_sync_lower_features()
both sync functions basically do one thing: make sure
NETIF_F_UPPER_DISABLES is consistently enforced among upper and lower
net devices.
currently NETIF_F_UPPER_DISABLES only includes NETIF_F_LRO

A lot of thoughts must have been given to this logic, and many
situations are considered for upper_devs like bond, team, bridge etc.
However, maybe a possible oversight is vlan_dev, which is an upper_dev
for its parent real_dev?
A vlan_dev is created with LRO unsupported by default, (NETIF_F_LRO
bit not set in hw_features).
As seen "fixed" in
root@server1:# ethtool -k ens4f0.50 | grep large
large-receive-offload: off [fixed]

Therefore, following the code path of upper_sync and lower_sync above,
once a vlan_dev is created, the parent real_dev can no longer set LRO
on.

Honestly, vlan_dev being treated as an upper_dev for the real_dev is a
bit counter-intuitive at the beginning, as people call them vlan
subports.
But, from the perspective that vlan_dev is a virtual device created
out of real_dev, it has somewhat "upper_dev" flavor, similar to
bond/team devices.
Kernel also associates upper_dev with some "master" role, and it makes
perfect sense for bond/team/bridge/ovs.
However, for vlan_dev, it sounds more like a slave dev to real_dev
(some people call real_dev parent port).
A secondary point, upper_dev (bond/team/bridge) typically has > 1
lower_dev, upper:lower normally has 1:N relationship.
For vlan_dev, it has only 1 lower_dev, upper:lower could often be N:1
relationship.

The above upper/lower sync logic probably stems from the "master" role
aspect of upper_dev, just that vlan_dev may not be a good fit for
this.
Probably that is where the confusion is.

Maybe I missed something, but this logic has been there for quite some
time (since v4.4 onwards, didn't try the latest, but tried pre-v4.4
kernels, no such issue under older kernels though).

Feel free to correct me.

Now, two possible solution proposals to fix this (if considered as an issue)
1. when creating/init a vlan_dev, set its hw_feature's NETIF_F_LRO bit
based on its underlying real_dev's hw_feature NETIF_F_LRO bit.
  (maybe not just hw_features, set wanted_feature as well?)
2. in netdev_sync_upper_features() and netdev_sync_lower_features()
exclude those upper_dev that is also a vlan_dev

Thanks for the attention.
Limin

p.s. another example of Centos-7.6 with VMXNET3 port
===========================================================================
# CentOS Linux release 7.6.1810 (Core)
root@esxi-server]# uname -a
Linux esxi-server 3.10.0-957.27.2.el7.x86_64 #1 SMP Mon Jul 29
17:46:05 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

# VMXNET3 NIC
[root@esxi-server]# ethtool -i ens224
driver: vmxnet3
version: 1.4.14.0-k-NAPI

# LRO enabled on the NIC
[root@esxi-server]# ethtool -k ens224 | grep large
large-receive-offload: on

# create a vlan subport, NIC LRO still on
[root@esxi-server]# ip link add link ens224 name ens224.50 type vlan id 50
[root@esxi-server]# ifconfig ens224.50 up
[root@esxi-server]# ethtool -k ens224 | grep large
large-receive-offload: on
[root@esxi-server]# ethtool -k ens224.50 | grep large
large-receive-offload: off [fixed]

# now turn LRO off, and after that, LRO cannot be turned on any longer
[root@esxi-server]# ethtool -K ens224 lro off
[root@esxi-server]# ethtool -k ens224 | grep large
large-receive-offload: off
[root@esxi-server]# ethtool -k ens224.50 | grep large
large-receive-offload: off [fixed]
[root@esxi-server]# ethtool -K ens224 lro on
Could not change any device features
[root@esxi-server]# ethtool -k ens224 | grep large
large-receive-offload: off [requested on]
[root@esxi-server]# ethtool -k ens224.50 | grep large
large-receive-offload: off [fixed]

# Now the only way to re-enable LRO on the parent port is to remove the subport
[root@esxi-server]# ip link del ens224.50
[root@esxi-server]# ethtool -k ens224 | grep large
large-receive-offload: off [requested on]
[root@esxi-server]# ethtool -K ens224 lro on
[root@esxi-server]# ethtool -k ens224 | grep large
large-receive-offload: on
===========================================================================

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LRO: creating vlan subports affects parent port's LRO settings
  2020-11-20  1:37 LRO: creating vlan subports affects parent port's LRO settings Limin Wang
@ 2020-11-24  0:26 ` Jakub Kicinski
  2020-12-06  0:04   ` Jarod Wilson
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2020-11-24  0:26 UTC (permalink / raw)
  To: Limin Wang; +Cc: netdev

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LRO: creating vlan subports affects parent port's LRO settings
  2020-11-24  0:26 ` Jakub Kicinski
@ 2020-12-06  0:04   ` Jarod Wilson
  2020-12-06 16:49     ` Michal Kubecek
  0 siblings, 1 reply; 7+ messages in thread
From: Jarod Wilson @ 2020-12-06  0:04 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: Limin Wang, Netdev

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?

-- 
Jarod Wilson
jarod@redhat.com


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: LRO: creating vlan subports affects parent port's LRO settings
  2020-12-06  0:04   ` Jarod Wilson
@ 2020-12-06 16:49     ` Michal Kubecek
  2020-12-06 22:58       ` Jarod Wilson
  2020-12-07  3:04       ` Limin Wang
  0 siblings, 2 replies; 7+ messages in thread
From: Michal Kubecek @ 2020-12-06 16:49 UTC (permalink / raw)
  To: netdev; +Cc: Jarod Wilson, Jakub Kicinski, Limin Wang

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LRO: creating vlan subports affects parent port's LRO settings
  2020-12-06 16:49     ` Michal Kubecek
@ 2020-12-06 22:58       ` Jarod Wilson
  2020-12-07  3:19         ` Limin Wang
  2020-12-07  3:04       ` Limin Wang
  1 sibling, 1 reply; 7+ messages in thread
From: Jarod Wilson @ 2020-12-06 22:58 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: Netdev, Jakub Kicinski, Limin Wang

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.

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@redhat.com


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LRO: creating vlan subports affects parent port's LRO settings
  2020-12-06 16:49     ` Michal Kubecek
  2020-12-06 22:58       ` Jarod Wilson
@ 2020-12-07  3:04       ` Limin Wang
  1 sibling, 0 replies; 7+ messages in thread
From: Limin Wang @ 2020-12-07  3:04 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: netdev, Jarod Wilson, Jakub Kicinski

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LRO: creating vlan subports affects parent port's LRO settings
  2020-12-06 22:58       ` Jarod Wilson
@ 2020-12-07  3:19         ` Limin Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Limin Wang @ 2020-12-07  3:19 UTC (permalink / raw)
  To: Jarod Wilson; +Cc: Michal Kubecek, Netdev, Jakub Kicinski

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@redhat.com> wrote:
>
> 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.
>
> 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@redhat.com
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-12-07  3:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20  1:37 LRO: creating vlan subports affects parent port's LRO settings Limin Wang
2020-11-24  0:26 ` Jakub Kicinski
2020-12-06  0:04   ` Jarod Wilson
2020-12-06 16:49     ` Michal Kubecek
2020-12-06 22:58       ` Jarod Wilson
2020-12-07  3:19         ` Limin Wang
2020-12-07  3:04       ` Limin Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.