NACK/cmnt: [SRU][trusty][PATCH] net/core: generic support for disabling netdev features down stack
Dan Streetman
dan.streetman at canonical.com
Tue Jun 5 11:53:49 UTC 2018
On Mon, Jun 4, 2018 at 6:48 PM, Stefan Bader <stefan.bader at canonical.com> wrote:
> 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.
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 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;
>>
>>
>
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
More information about the kernel-team
mailing list