[SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack

Dan Streetman
From: Jarod Wilson <[hidden email]>

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" <[hidden email]>
CC: Eric Dumazet <[hidden email]>
CC: Jay Vosburgh <[hidden email]>
CC: Veaceslav Falico <[hidden email]>
CC: Andy Gospodarek <[hidden email]>
CC: Jiri Pirko <[hidden email]>
CC: Nikolay Aleksandrov <[hidden email]>
CC: Michal Kubecek <[hidden email]>
CC: Alexander Duyck <[hidden email]>
CC: [hidden email]
Signed-off-by: Jarod Wilson <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry-picked from commit fd867d51f889aec11cca235ebb008578780d052d)
(backported from commit 44a4085538c844e79d6ee6bcf46fabf7c57a9a38)
[ddstreet: utility funcs backported for use by cherry-pick]
Signed-off-by: Dan Streetman <[hidden email]>

---
 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;
 
--
2.17.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack

Kleber Souza
On 05/24/18 09:05, Dan Streetman wrote:

> From: Jarod Wilson <[hidden email]>
>
> 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" <[hidden email]>
> CC: Eric Dumazet <[hidden email]>
> CC: Jay Vosburgh <[hidden email]>
> CC: Veaceslav Falico <[hidden email]>
> CC: Andy Gospodarek <[hidden email]>
> CC: Jiri Pirko <[hidden email]>
> CC: Nikolay Aleksandrov <[hidden email]>
> CC: Michal Kubecek <[hidden email]>
> CC: Alexander Duyck <[hidden email]>
> CC: [hidden email]
> Signed-off-by: Jarod Wilson <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (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 <[hidden email]>
>
> ---
>  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
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK/cmnt: [SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack

Stefan Bader-2
In reply to this post by Dan Streetman
On 24.05.2018 09:05, Dan Streetman wrote:

> From: Jarod Wilson <[hidden email]>
>
> 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" <[hidden email]>
> CC: Eric Dumazet <[hidden email]>
> CC: Jay Vosburgh <[hidden email]>
> CC: Veaceslav Falico <[hidden email]>
> CC: Andy Gospodarek <[hidden email]>
> CC: Jiri Pirko <[hidden email]>
> CC: Nikolay Aleksandrov <[hidden email]>
> CC: Michal Kubecek <[hidden email]>
> CC: Alexander Duyck <[hidden email]>
> CC: [hidden email]
> Signed-off-by: Jarod Wilson <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (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 <[hidden email]>
>
> ---
>  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
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: NACK/cmnt: [SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack

Dan Streetman
On Mon, Jun 4, 2018 at 6:48 PM, Stefan Bader <[hidden email]> wrote:

> On 24.05.2018 09:05, Dan Streetman wrote:
>> From: Jarod Wilson <[hidden email]>
>>
>> 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" <[hidden email]>
>> CC: Eric Dumazet <[hidden email]>
>> CC: Jay Vosburgh <[hidden email]>
>> CC: Veaceslav Falico <[hidden email]>
>> CC: Andy Gospodarek <[hidden email]>
>> CC: Jiri Pirko <[hidden email]>
>> CC: Nikolay Aleksandrov <[hidden email]>
>> CC: Michal Kubecek <[hidden email]>
>> CC: Alexander Duyck <[hidden email]>
>> CC: [hidden email]
>> Signed-off-by: Jarod Wilson <[hidden email]>
>> Signed-off-by: David S. Miller <[hidden email]>
>> (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 <[hidden email]>
>>
>> ---
>>  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
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team