NACK/cmnt: [SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack
Stefan Bader
stefan.bader at canonical.com
Mon Jun 4 22:48:14 UTC 2018
On 24.05.2018 09:05, Dan Streetman wrote:
> From: Jarod Wilson <jarod at 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 at dell-per730-01 ~]# ethtool -k bond0 |grep large
> large-receive-offload: on
> [root at dell-per730-01 ~]# ethtool -k p5p1 |grep large
> large-receive-offload: on
> [root at dell-per730-01 ~]# ethtool -K bond0 lro off
> [root at dell-per730-01 ~]# ethtool -k bond0 |grep large
> large-receive-offload: off
> [root at 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 at davemloft.net>
> CC: Eric Dumazet <edumazet at google.com>
> CC: Jay Vosburgh <j.vosburgh at gmail.com>
> CC: Veaceslav Falico <vfalico at gmail.com>
> CC: Andy Gospodarek <gospo at cumulusnetworks.com>
> CC: Jiri Pirko <jiri at resnulli.us>
> CC: Nikolay Aleksandrov <razor at blackwall.org>
> CC: Michal Kubecek <mkubecek at suse.cz>
> CC: Alexander Duyck <alexander.duyck at gmail.com>
> CC: netdev at vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod at redhat.com>
> Signed-off-by: David S. Miller <davem at 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 at 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;
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180604/c412f9a1/attachment.sig>
More information about the kernel-team
mailing list