| Message ID | 20180524160522.18145-1-dan.streetman@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,trusty] net/core: generic support for disabling netdev features down stack | expand |
On 05/24/18 09:05, Dan Streetman wrote: > From: Jarod Wilson <jarod@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1771480 > > There are some netdev features, which when disabled on an upper device, > such as a bonding master or a bridge, must be disabled and cannot be > re-enabled on underlying devices. > > This is a rework of an earlier more heavy-handed appraoch, which simply > disables and prevents re-enabling of netdev features listed in a new > define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper > device that disables a flag in that feature mask, the disabling will > propagate down the stack, and any lower device that has any upper device > with one of those flags disabled should not be able to enable said flag. > > Initially, only LRO is included for proof of concept, and because this > code effectively does the same thing as dev_disable_lro(), though it will > also activate from the ethtool path, which was one of the goals here. > > [root@dell-per730-01 ~]# ethtool -k bond0 |grep large > large-receive-offload: on > [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large > large-receive-offload: on > [root@dell-per730-01 ~]# ethtool -K bond0 lro off > [root@dell-per730-01 ~]# ethtool -k bond0 |grep large > large-receive-offload: off > [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large > large-receive-offload: off > > dmesg dump: > > [ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2. > [ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X IRQs: sp 74 fp[0] 76 ... fp[7] 83 > [ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1. > [ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X IRQs: sp 62 fp[0] 64 ... fp[7] 71 > > This has been successfully tested with bnx2x, qlcnic and netxen network > cards as slaves in a bond interface. Turning LRO on or off on the master > also turns it on or off on each of the slaves, new slaves are added with > LRO in the same state as the master, and LRO can't be toggled on the > slaves. > > Also, this should largely remove the need for dev_disable_lro(), and most, > if not all, of its call sites can be replaced by simply making sure > NETIF_F_LRO isn't included in the relevant device's feature flags. > > Note that this patch is driven by bug reports from users saying it was > confusing that bonds and slaves had different settings for the same > features, and while it won't be 100% in sync if a lower device doesn't > support a feature like LRO, I think this is a good step in the right > direction. > > CC: "David S. Miller" <davem@davemloft.net> > CC: Eric Dumazet <edumazet@google.com> > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Veaceslav Falico <vfalico@gmail.com> > CC: Andy Gospodarek <gospo@cumulusnetworks.com> > CC: Jiri Pirko <jiri@resnulli.us> > CC: Nikolay Aleksandrov <razor@blackwall.org> > CC: Michal Kubecek <mkubecek@suse.cz> > CC: Alexander Duyck <alexander.duyck@gmail.com> > CC: netdev@vger.kernel.org > Signed-off-by: Jarod Wilson <jarod@redhat.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from commit fd867d51f889aec11cca235ebb008578780d052d) > (backported from commit 44a4085538c844e79d6ee6bcf46fabf7c57a9a38) It seems that the above "(backported from...)" line was added by mistake. Thanks, Kleber > [ddstreet: utility funcs backported for use by cherry-pick] > Signed-off-by: Dan Streetman <ddstreet@canonical.com> > > --- > include/linux/netdev_features.h | 11 +++++ > include/linux/netdevice.h | 9 ++++ > net/core/dev.c | 76 +++++++++++++++++++++++++++++++++ > 3 files changed, 96 insertions(+) > > diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h > index b79be82cd02b..64c330e779b7 100644 > --- a/include/linux/netdev_features.h > +++ b/include/linux/netdev_features.h > @@ -121,6 +121,11 @@ enum { > #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX) > #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD) > > +#define for_each_netdev_feature(mask_addr, feature) \ > + int bit; \ > + for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \ > + feature = __NETIF_F_BIT(bit); > + > /* Features valid for ethtool to change */ > /* = all defined minus driver/device-class-related */ > #define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \ > @@ -162,6 +167,12 @@ enum { > */ > #define NETIF_F_ALL_FOR_ALL (NETIF_F_NOCACHE_COPY | NETIF_F_FSO) > > +/* > + * If upper/master device has these features disabled, they must be disabled > + * on all lower/slave devices as well. > + */ > +#define NETIF_F_UPPER_DISABLES NETIF_F_LRO > + > /* changeable features with no special hardware requirements */ > #define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 21ced47b9664..6e56f3af821c 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2927,9 +2927,18 @@ extern int bpf_jit_enable; > > bool netdev_has_upper_dev(struct net_device *dev, struct net_device *upper_dev); > bool netdev_has_any_upper_dev(struct net_device *dev); > +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, > + struct list_head **iter); > struct net_device *netdev_all_upper_get_next_dev_rcu(struct net_device *dev, > struct list_head **iter); > > +/* iterate through upper list, must be called under RCU read lock */ > +#define netdev_for_each_upper_dev_rcu(dev, updev, iter) \ > + for (iter = &(dev)->adj_list.upper, \ > + updev = netdev_upper_get_next_dev_rcu(dev, &(iter)); \ > + updev; \ > + updev = netdev_upper_get_next_dev_rcu(dev, &(iter))) > + > /* iterate through upper list, must be called under RCU read lock */ > #define netdev_for_each_all_upper_dev_rcu(dev, updev, iter) \ > for (iter = &(dev)->all_adj_list.upper, \ > diff --git a/net/core/dev.c b/net/core/dev.c > index 1d37e93ad23c..8f0eb84a5c0f 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4538,6 +4538,32 @@ void *netdev_adjacent_get_private(struct list_head *adj_list) > } > EXPORT_SYMBOL(netdev_adjacent_get_private); > > +/** > + * netdev_upper_get_next_dev_rcu - Get the next dev from upper list > + * @dev: device > + * @iter: list_head ** of the current position > + * > + * Gets the next device from the dev's upper list, starting from iter > + * position. The caller must hold RCU read lock. > + */ > +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, > + struct list_head **iter) > +{ > + struct netdev_adjacent *upper; > + > + WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); > + > + upper = list_entry_rcu((*iter)->next, struct netdev_adjacent, list); > + > + if (&upper->list == &dev->adj_list.upper) > + return NULL; > + > + *iter = &upper->list; > + > + return upper->dev; > +} > +EXPORT_SYMBOL(netdev_upper_get_next_dev_rcu); > + > /** > * netdev_all_upper_get_next_dev_rcu - Get the next dev from upper list > * @dev: device > @@ -5666,6 +5692,44 @@ static void rollback_registered(struct net_device *dev) > list_del(&single); > } > > +static netdev_features_t netdev_sync_upper_features(struct net_device *lower, > + struct net_device *upper, netdev_features_t features) > +{ > + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; > + netdev_features_t feature; > + > + for_each_netdev_feature(&upper_disables, feature) { > + 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; > + } > + } > + > + return features; > +} > + > +static void netdev_sync_lower_features(struct net_device *upper, > + struct net_device *lower, netdev_features_t features) > +{ > + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; > + netdev_features_t feature; > + > + for_each_netdev_feature(&upper_disables, feature) { > + if (!(features & feature) && (lower->features & feature)) { > + netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n", > + &feature, lower->name); > + lower->wanted_features &= ~feature; > + netdev_update_features(lower); > + > + if (unlikely(lower->features & feature)) > + netdev_WARN(upper, "failed to disable %pNF on %s!\n", > + &feature, lower->name); > + } > + } > +} > + > static netdev_features_t netdev_fix_features(struct net_device *dev, > netdev_features_t features) > { > @@ -5728,7 +5792,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, > > int __netdev_update_features(struct net_device *dev) > { > + struct net_device *upper, *lower; > netdev_features_t features; > + struct list_head *iter; > int err = 0; > > ASSERT_RTNL(); > @@ -5741,6 +5807,10 @@ int __netdev_update_features(struct net_device *dev) > /* driver might be less strict about feature dependencies */ > features = netdev_fix_features(dev, features); > > + /* some features can't be enabled if they're off an an upper device */ > + netdev_for_each_upper_dev_rcu(dev, upper, iter) > + features = netdev_sync_upper_features(dev, upper, features); > + > if (dev->features == features) > return 0; > > @@ -5757,6 +5827,12 @@ int __netdev_update_features(struct net_device *dev) > return -1; > } > > + /* 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 (!err) > dev->features = features; > >
On 24.05.2018 09:05, Dan Streetman wrote: > From: Jarod Wilson <jarod@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1771480 > > There are some netdev features, which when disabled on an upper device, > such as a bonding master or a bridge, must be disabled and cannot be > re-enabled on underlying devices. > > This is a rework of an earlier more heavy-handed appraoch, which simply > disables and prevents re-enabling of netdev features listed in a new > define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper > device that disables a flag in that feature mask, the disabling will > propagate down the stack, and any lower device that has any upper device > with one of those flags disabled should not be able to enable said flag. > > Initially, only LRO is included for proof of concept, and because this > code effectively does the same thing as dev_disable_lro(), though it will > also activate from the ethtool path, which was one of the goals here. > > [root@dell-per730-01 ~]# ethtool -k bond0 |grep large > large-receive-offload: on > [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large > large-receive-offload: on > [root@dell-per730-01 ~]# ethtool -K bond0 lro off > [root@dell-per730-01 ~]# ethtool -k bond0 |grep large > large-receive-offload: off > [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large > large-receive-offload: off > > dmesg dump: > > [ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2. > [ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X IRQs: sp 74 fp[0] 76 ... fp[7] 83 > [ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1. > [ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X IRQs: sp 62 fp[0] 64 ... fp[7] 71 > > This has been successfully tested with bnx2x, qlcnic and netxen network > cards as slaves in a bond interface. Turning LRO on or off on the master > also turns it on or off on each of the slaves, new slaves are added with > LRO in the same state as the master, and LRO can't be toggled on the > slaves. > > Also, this should largely remove the need for dev_disable_lro(), and most, > if not all, of its call sites can be replaced by simply making sure > NETIF_F_LRO isn't included in the relevant device's feature flags. > > Note that this patch is driven by bug reports from users saying it was > confusing that bonds and slaves had different settings for the same > features, and while it won't be 100% in sync if a lower device doesn't > support a feature like LRO, I think this is a good step in the right > direction. > > CC: "David S. Miller" <davem@davemloft.net> > CC: Eric Dumazet <edumazet@google.com> > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Veaceslav Falico <vfalico@gmail.com> > CC: Andy Gospodarek <gospo@cumulusnetworks.com> > CC: Jiri Pirko <jiri@resnulli.us> > CC: Nikolay Aleksandrov <razor@blackwall.org> > CC: Michal Kubecek <mkubecek@suse.cz> > CC: Alexander Duyck <alexander.duyck@gmail.com> > CC: netdev@vger.kernel.org > Signed-off-by: Jarod Wilson <jarod@redhat.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from commit fd867d51f889aec11cca235ebb008578780d052d) > (backported from commit 44a4085538c844e79d6ee6bcf46fabf7c57a9a38) I think in this case the better approach is to actually split these two things. Probably make the partial backport an UBUNTU: SAUCE: Backport bla.. This is a partial backport of helper functions introduced in ... to be used by ... (backported from 44a4085538c844e79d6ee6bcf46fabf7c57a9a38) And then the cherry-pick as is. -Stefan > [ddstreet: utility funcs backported for use by cherry-pick] > Signed-off-by: Dan Streetman <ddstreet@canonical.com> > > --- > include/linux/netdev_features.h | 11 +++++ > include/linux/netdevice.h | 9 ++++ > net/core/dev.c | 76 +++++++++++++++++++++++++++++++++ > 3 files changed, 96 insertions(+) > > diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h > index b79be82cd02b..64c330e779b7 100644 > --- a/include/linux/netdev_features.h > +++ b/include/linux/netdev_features.h > @@ -121,6 +121,11 @@ enum { > #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX) > #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD) > > +#define for_each_netdev_feature(mask_addr, feature) \ > + int bit; \ > + for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \ > + feature = __NETIF_F_BIT(bit); > + > /* Features valid for ethtool to change */ > /* = all defined minus driver/device-class-related */ > #define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \ > @@ -162,6 +167,12 @@ enum { > */ > #define NETIF_F_ALL_FOR_ALL (NETIF_F_NOCACHE_COPY | NETIF_F_FSO) > > +/* > + * If upper/master device has these features disabled, they must be disabled > + * on all lower/slave devices as well. > + */ > +#define NETIF_F_UPPER_DISABLES NETIF_F_LRO > + > /* changeable features with no special hardware requirements */ > #define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 21ced47b9664..6e56f3af821c 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2927,9 +2927,18 @@ extern int bpf_jit_enable; > > bool netdev_has_upper_dev(struct net_device *dev, struct net_device *upper_dev); > bool netdev_has_any_upper_dev(struct net_device *dev); > +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, > + struct list_head **iter); > struct net_device *netdev_all_upper_get_next_dev_rcu(struct net_device *dev, > struct list_head **iter); > > +/* iterate through upper list, must be called under RCU read lock */ > +#define netdev_for_each_upper_dev_rcu(dev, updev, iter) \ > + for (iter = &(dev)->adj_list.upper, \ > + updev = netdev_upper_get_next_dev_rcu(dev, &(iter)); \ > + updev; \ > + updev = netdev_upper_get_next_dev_rcu(dev, &(iter))) > + > /* iterate through upper list, must be called under RCU read lock */ > #define netdev_for_each_all_upper_dev_rcu(dev, updev, iter) \ > for (iter = &(dev)->all_adj_list.upper, \ > diff --git a/net/core/dev.c b/net/core/dev.c > index 1d37e93ad23c..8f0eb84a5c0f 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4538,6 +4538,32 @@ void *netdev_adjacent_get_private(struct list_head *adj_list) > } > EXPORT_SYMBOL(netdev_adjacent_get_private); > > +/** > + * netdev_upper_get_next_dev_rcu - Get the next dev from upper list > + * @dev: device > + * @iter: list_head ** of the current position > + * > + * Gets the next device from the dev's upper list, starting from iter > + * position. The caller must hold RCU read lock. > + */ > +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, > + struct list_head **iter) > +{ > + struct netdev_adjacent *upper; > + > + WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); > + > + upper = list_entry_rcu((*iter)->next, struct netdev_adjacent, list); > + > + if (&upper->list == &dev->adj_list.upper) > + return NULL; > + > + *iter = &upper->list; > + > + return upper->dev; > +} > +EXPORT_SYMBOL(netdev_upper_get_next_dev_rcu); > + > /** > * netdev_all_upper_get_next_dev_rcu - Get the next dev from upper list > * @dev: device > @@ -5666,6 +5692,44 @@ static void rollback_registered(struct net_device *dev) > list_del(&single); > } > > +static netdev_features_t netdev_sync_upper_features(struct net_device *lower, > + struct net_device *upper, netdev_features_t features) > +{ > + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; > + netdev_features_t feature; > + > + for_each_netdev_feature(&upper_disables, feature) { > + 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; > + } > + } > + > + return features; > +} > + > +static void netdev_sync_lower_features(struct net_device *upper, > + struct net_device *lower, netdev_features_t features) > +{ > + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; > + netdev_features_t feature; > + > + for_each_netdev_feature(&upper_disables, feature) { > + if (!(features & feature) && (lower->features & feature)) { > + netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n", > + &feature, lower->name); > + lower->wanted_features &= ~feature; > + netdev_update_features(lower); > + > + if (unlikely(lower->features & feature)) > + netdev_WARN(upper, "failed to disable %pNF on %s!\n", > + &feature, lower->name); > + } > + } > +} > + > static netdev_features_t netdev_fix_features(struct net_device *dev, > netdev_features_t features) > { > @@ -5728,7 +5792,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, > > int __netdev_update_features(struct net_device *dev) > { > + struct net_device *upper, *lower; > netdev_features_t features; > + struct list_head *iter; > int err = 0; > > ASSERT_RTNL(); > @@ -5741,6 +5807,10 @@ int __netdev_update_features(struct net_device *dev) > /* driver might be less strict about feature dependencies */ > features = netdev_fix_features(dev, features); > > + /* some features can't be enabled if they're off an an upper device */ > + netdev_for_each_upper_dev_rcu(dev, upper, iter) > + features = netdev_sync_upper_features(dev, upper, features); > + > if (dev->features == features) > return 0; > > @@ -5757,6 +5827,12 @@ int __netdev_update_features(struct net_device *dev) > return -1; > } > > + /* 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 (!err) > dev->features = features; > >
On Mon, Jun 4, 2018 at 6:48 PM, Stefan Bader <stefan.bader@canonical.com> wrote: > On 24.05.2018 09:05, Dan Streetman wrote: >> From: Jarod Wilson <jarod@redhat.com> >> >> BugLink: https://bugs.launchpad.net/bugs/1771480 >> >> There are some netdev features, which when disabled on an upper device, >> such as a bonding master or a bridge, must be disabled and cannot be >> re-enabled on underlying devices. >> >> This is a rework of an earlier more heavy-handed appraoch, which simply >> disables and prevents re-enabling of netdev features listed in a new >> define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper >> device that disables a flag in that feature mask, the disabling will >> propagate down the stack, and any lower device that has any upper device >> with one of those flags disabled should not be able to enable said flag. >> >> Initially, only LRO is included for proof of concept, and because this >> code effectively does the same thing as dev_disable_lro(), though it will >> also activate from the ethtool path, which was one of the goals here. >> >> [root@dell-per730-01 ~]# ethtool -k bond0 |grep large >> large-receive-offload: on >> [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large >> large-receive-offload: on >> [root@dell-per730-01 ~]# ethtool -K bond0 lro off >> [root@dell-per730-01 ~]# ethtool -k bond0 |grep large >> large-receive-offload: off >> [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large >> large-receive-offload: off >> >> dmesg dump: >> >> [ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2. >> [ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X IRQs: sp 74 fp[0] 76 ... fp[7] 83 >> [ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1. >> [ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X IRQs: sp 62 fp[0] 64 ... fp[7] 71 >> >> This has been successfully tested with bnx2x, qlcnic and netxen network >> cards as slaves in a bond interface. Turning LRO on or off on the master >> also turns it on or off on each of the slaves, new slaves are added with >> LRO in the same state as the master, and LRO can't be toggled on the >> slaves. >> >> Also, this should largely remove the need for dev_disable_lro(), and most, >> if not all, of its call sites can be replaced by simply making sure >> NETIF_F_LRO isn't included in the relevant device's feature flags. >> >> Note that this patch is driven by bug reports from users saying it was >> confusing that bonds and slaves had different settings for the same >> features, and while it won't be 100% in sync if a lower device doesn't >> support a feature like LRO, I think this is a good step in the right >> direction. >> >> CC: "David S. Miller" <davem@davemloft.net> >> CC: Eric Dumazet <edumazet@google.com> >> CC: Jay Vosburgh <j.vosburgh@gmail.com> >> CC: Veaceslav Falico <vfalico@gmail.com> >> CC: Andy Gospodarek <gospo@cumulusnetworks.com> >> CC: Jiri Pirko <jiri@resnulli.us> >> CC: Nikolay Aleksandrov <razor@blackwall.org> >> CC: Michal Kubecek <mkubecek@suse.cz> >> CC: Alexander Duyck <alexander.duyck@gmail.com> >> CC: netdev@vger.kernel.org >> Signed-off-by: Jarod Wilson <jarod@redhat.com> >> Signed-off-by: David S. Miller <davem@davemloft.net> >> (cherry-picked from commit fd867d51f889aec11cca235ebb008578780d052d) >> (backported from commit 44a4085538c844e79d6ee6bcf46fabf7c57a9a38) > > I think in this case the better approach is to actually split these two things. > Probably make the partial backport an > > UBUNTU: SAUCE: Backport bla.. > > This is a partial backport of helper functions introduced in > ... > to be used by ... > > (backported from 44a4085538c844e79d6ee6bcf46fabf7c57a9a38) > > And then the cherry-pick as is. ack, i'll resend v2 as a patch series, thnx. > > -Stefan > >> [ddstreet: utility funcs backported for use by cherry-pick] >> Signed-off-by: Dan Streetman <ddstreet@canonical.com> >> >> --- >> include/linux/netdev_features.h | 11 +++++ >> include/linux/netdevice.h | 9 ++++ >> net/core/dev.c | 76 +++++++++++++++++++++++++++++++++ >> 3 files changed, 96 insertions(+) >> >> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h >> index b79be82cd02b..64c330e779b7 100644 >> --- a/include/linux/netdev_features.h >> +++ b/include/linux/netdev_features.h >> @@ -121,6 +121,11 @@ enum { >> #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX) >> #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD) >> >> +#define for_each_netdev_feature(mask_addr, feature) \ >> + int bit; \ >> + for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \ >> + feature = __NETIF_F_BIT(bit); >> + >> /* Features valid for ethtool to change */ >> /* = all defined minus driver/device-class-related */ >> #define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \ >> @@ -162,6 +167,12 @@ enum { >> */ >> #define NETIF_F_ALL_FOR_ALL (NETIF_F_NOCACHE_COPY | NETIF_F_FSO) >> >> +/* >> + * If upper/master device has these features disabled, they must be disabled >> + * on all lower/slave devices as well. >> + */ >> +#define NETIF_F_UPPER_DISABLES NETIF_F_LRO >> + >> /* changeable features with no special hardware requirements */ >> #define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 21ced47b9664..6e56f3af821c 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -2927,9 +2927,18 @@ extern int bpf_jit_enable; >> >> bool netdev_has_upper_dev(struct net_device *dev, struct net_device *upper_dev); >> bool netdev_has_any_upper_dev(struct net_device *dev); >> +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, >> + struct list_head **iter); >> struct net_device *netdev_all_upper_get_next_dev_rcu(struct net_device *dev, >> struct list_head **iter); >> >> +/* iterate through upper list, must be called under RCU read lock */ >> +#define netdev_for_each_upper_dev_rcu(dev, updev, iter) \ >> + for (iter = &(dev)->adj_list.upper, \ >> + updev = netdev_upper_get_next_dev_rcu(dev, &(iter)); \ >> + updev; \ >> + updev = netdev_upper_get_next_dev_rcu(dev, &(iter))) >> + >> /* iterate through upper list, must be called under RCU read lock */ >> #define netdev_for_each_all_upper_dev_rcu(dev, updev, iter) \ >> for (iter = &(dev)->all_adj_list.upper, \ >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 1d37e93ad23c..8f0eb84a5c0f 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -4538,6 +4538,32 @@ void *netdev_adjacent_get_private(struct list_head *adj_list) >> } >> EXPORT_SYMBOL(netdev_adjacent_get_private); >> >> +/** >> + * netdev_upper_get_next_dev_rcu - Get the next dev from upper list >> + * @dev: device >> + * @iter: list_head ** of the current position >> + * >> + * Gets the next device from the dev's upper list, starting from iter >> + * position. The caller must hold RCU read lock. >> + */ >> +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, >> + struct list_head **iter) >> +{ >> + struct netdev_adjacent *upper; >> + >> + WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); >> + >> + upper = list_entry_rcu((*iter)->next, struct netdev_adjacent, list); >> + >> + if (&upper->list == &dev->adj_list.upper) >> + return NULL; >> + >> + *iter = &upper->list; >> + >> + return upper->dev; >> +} >> +EXPORT_SYMBOL(netdev_upper_get_next_dev_rcu); >> + >> /** >> * netdev_all_upper_get_next_dev_rcu - Get the next dev from upper list >> * @dev: device >> @@ -5666,6 +5692,44 @@ static void rollback_registered(struct net_device *dev) >> list_del(&single); >> } >> >> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower, >> + struct net_device *upper, netdev_features_t features) >> +{ >> + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; >> + netdev_features_t feature; >> + >> + for_each_netdev_feature(&upper_disables, feature) { >> + 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; >> + } >> + } >> + >> + return features; >> +} >> + >> +static void netdev_sync_lower_features(struct net_device *upper, >> + struct net_device *lower, netdev_features_t features) >> +{ >> + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; >> + netdev_features_t feature; >> + >> + for_each_netdev_feature(&upper_disables, feature) { >> + if (!(features & feature) && (lower->features & feature)) { >> + netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n", >> + &feature, lower->name); >> + lower->wanted_features &= ~feature; >> + netdev_update_features(lower); >> + >> + if (unlikely(lower->features & feature)) >> + netdev_WARN(upper, "failed to disable %pNF on %s!\n", >> + &feature, lower->name); >> + } >> + } >> +} >> + >> static netdev_features_t netdev_fix_features(struct net_device *dev, >> netdev_features_t features) >> { >> @@ -5728,7 +5792,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, >> >> int __netdev_update_features(struct net_device *dev) >> { >> + struct net_device *upper, *lower; >> netdev_features_t features; >> + struct list_head *iter; >> int err = 0; >> >> ASSERT_RTNL(); >> @@ -5741,6 +5807,10 @@ int __netdev_update_features(struct net_device *dev) >> /* driver might be less strict about feature dependencies */ >> features = netdev_fix_features(dev, features); >> >> + /* some features can't be enabled if they're off an an upper device */ >> + netdev_for_each_upper_dev_rcu(dev, upper, iter) >> + features = netdev_sync_upper_features(dev, upper, features); >> + >> if (dev->features == features) >> return 0; >> >> @@ -5757,6 +5827,12 @@ int __netdev_update_features(struct net_device *dev) >> return -1; >> } >> >> + /* 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 (!err) >> dev->features = features; >> >> > > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team >
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h index b79be82cd02b..64c330e779b7 100644 --- a/include/linux/netdev_features.h +++ b/include/linux/netdev_features.h @@ -121,6 +121,11 @@ enum { #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX) #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD) +#define for_each_netdev_feature(mask_addr, feature) \ + int bit; \ + for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \ + feature = __NETIF_F_BIT(bit); + /* Features valid for ethtool to change */ /* = all defined minus driver/device-class-related */ #define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \ @@ -162,6 +167,12 @@ enum { */ #define NETIF_F_ALL_FOR_ALL (NETIF_F_NOCACHE_COPY | NETIF_F_FSO) +/* + * If upper/master device has these features disabled, they must be disabled + * on all lower/slave devices as well. + */ +#define NETIF_F_UPPER_DISABLES NETIF_F_LRO + /* changeable features with no special hardware requirements */ #define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 21ced47b9664..6e56f3af821c 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2927,9 +2927,18 @@ extern int bpf_jit_enable; bool netdev_has_upper_dev(struct net_device *dev, struct net_device *upper_dev); bool netdev_has_any_upper_dev(struct net_device *dev); +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, + struct list_head **iter); struct net_device *netdev_all_upper_get_next_dev_rcu(struct net_device *dev, struct list_head **iter); +/* iterate through upper list, must be called under RCU read lock */ +#define netdev_for_each_upper_dev_rcu(dev, updev, iter) \ + for (iter = &(dev)->adj_list.upper, \ + updev = netdev_upper_get_next_dev_rcu(dev, &(iter)); \ + updev; \ + updev = netdev_upper_get_next_dev_rcu(dev, &(iter))) + /* iterate through upper list, must be called under RCU read lock */ #define netdev_for_each_all_upper_dev_rcu(dev, updev, iter) \ for (iter = &(dev)->all_adj_list.upper, \ diff --git a/net/core/dev.c b/net/core/dev.c index 1d37e93ad23c..8f0eb84a5c0f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4538,6 +4538,32 @@ void *netdev_adjacent_get_private(struct list_head *adj_list) } EXPORT_SYMBOL(netdev_adjacent_get_private); +/** + * netdev_upper_get_next_dev_rcu - Get the next dev from upper list + * @dev: device + * @iter: list_head ** of the current position + * + * Gets the next device from the dev's upper list, starting from iter + * position. The caller must hold RCU read lock. + */ +struct net_device *netdev_upper_get_next_dev_rcu(struct net_device *dev, + struct list_head **iter) +{ + struct netdev_adjacent *upper; + + WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); + + upper = list_entry_rcu((*iter)->next, struct netdev_adjacent, list); + + if (&upper->list == &dev->adj_list.upper) + return NULL; + + *iter = &upper->list; + + return upper->dev; +} +EXPORT_SYMBOL(netdev_upper_get_next_dev_rcu); + /** * netdev_all_upper_get_next_dev_rcu - Get the next dev from upper list * @dev: device @@ -5666,6 +5692,44 @@ static void rollback_registered(struct net_device *dev) list_del(&single); } +static netdev_features_t netdev_sync_upper_features(struct net_device *lower, + struct net_device *upper, netdev_features_t features) +{ + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; + netdev_features_t feature; + + for_each_netdev_feature(&upper_disables, feature) { + 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; + } + } + + return features; +} + +static void netdev_sync_lower_features(struct net_device *upper, + struct net_device *lower, netdev_features_t features) +{ + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES; + netdev_features_t feature; + + for_each_netdev_feature(&upper_disables, feature) { + if (!(features & feature) && (lower->features & feature)) { + netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n", + &feature, lower->name); + lower->wanted_features &= ~feature; + netdev_update_features(lower); + + if (unlikely(lower->features & feature)) + netdev_WARN(upper, "failed to disable %pNF on %s!\n", + &feature, lower->name); + } + } +} + static netdev_features_t netdev_fix_features(struct net_device *dev, netdev_features_t features) { @@ -5728,7 +5792,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev, int __netdev_update_features(struct net_device *dev) { + struct net_device *upper, *lower; netdev_features_t features; + struct list_head *iter; int err = 0; ASSERT_RTNL(); @@ -5741,6 +5807,10 @@ int __netdev_update_features(struct net_device *dev) /* driver might be less strict about feature dependencies */ features = netdev_fix_features(dev, features); + /* some features can't be enabled if they're off an an upper device */ + netdev_for_each_upper_dev_rcu(dev, upper, iter) + features = netdev_sync_upper_features(dev, upper, features); + if (dev->features == features) return 0; @@ -5757,6 +5827,12 @@ int __netdev_update_features(struct net_device *dev) return -1; } + /* 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 (!err) dev->features = features;