Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1260844 > unrolled thread

[PATCH net-next] net/core: generic support for disabling netdev features down stack

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-02 19:00 +0100
    Re: [PATCH net-next] net/core: generic support for disabling netdev  features down stack Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-02 19:10 +0100
      Re: [PATCH net-next] net/core: generic support for disabling netdev  features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-02 23:00 +0100
    [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 04:00 +0100
      Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack David Miller <davem@davemloft.net> - 2015-11-03 05:50 +0100
      Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 11:10 +0100
        Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack Geert Uytterhoeven <geert@linux-m68k.org> - 2015-11-03 15:00 +0100
          Re: [PATCH v2 net-next] net/core: generic support for disabling netdev  features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 15:00 +0100
            Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 15:10 +0100
              Re: [PATCH v2 net-next] net/core: generic support for disabling netdev  features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 17:10 +0100
      [PATCH net-next] net/core: fix for_each_netdev_feature Jarod Wilson <jarod@redhat.com> - 2015-11-03 16:20 +0100
        Re: [PATCH net-next] net/core: fix for_each_netdev_feature Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 16:50 +0100
        Re: [PATCH net-next] net/core: fix for_each_netdev_feature David Miller <davem@davemloft.net> - 2015-11-03 17:40 +0100
      [PATCH net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 21:40 +0100
        Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-03 22:20 +0100
          Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 23:20 +0100
            Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-04 00:10 +0100
        Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 22:30 +0100
        Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Michal Kubecek <mkubecek@suse.cz> - 2015-11-03 23:00 +0100
          Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 23:00 +0100
        [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-04 05:10 +0100
          Re: [PATCH v2 net-next] net/core: ensure features get disabled on  new lower devs David Miller <davem@davemloft.net> - 2015-11-05 04:00 +0100
            Re: [PATCH v2 net-next] net/core: ensure features get disabled on  new lower devs Florian Fainelli <f.fainelli@gmail.com> - 2015-11-13 01:30 +0100
              Re: [PATCH v2 net-next] net/core: ensure features get disabled on  new lower devs Jiri Pirko <jiri@resnulli.us> - 2015-11-13 11:30 +0100
                Re: [PATCH v2 net-next] net/core: ensure features get disabled on new  lower devs Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-13 12:00 +0100
                  Re: [PATCH v2 net-next] net/core: ensure features get disabled on new  lower devs Laura Abbott <labbott@redhat.com> - 2015-11-13 23:40 +0100

Page 1 of 2  [1] 2  Next page →

#1260844 — [PATCH net-next] net/core: generic support for disabling netdev features down stack

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>
---
Note: this replaces "[RFC PATCH net-next] net/core: initial support for
stacked dev feature toggles" for consideration.

 include/linux/netdev_features.h | 11 +++++++++
 net/core/dev.c                  | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 9672781..0f5837a 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -125,6 +125,11 @@ enum {
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
 
+#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 | \
@@ -167,6 +172,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/net/core/dev.c b/net/core/dev.c
index 13f49f8..3a8dbbc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6288,9 +6288,51 @@ 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);
+			upper->wanted_features &= ~feature;
+			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)
 {
+	struct net_device *upper, *lower;
+	struct list_head *iter;
+
 	/* Fix illegal checksum combinations */
 	if ((features & NETIF_F_HW_CSUM) &&
 	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
@@ -6345,6 +6387,16 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		}
 	}
 
+	/* 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);
+
+	/* 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);
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	if (dev->netdev_ops->ndo_busy_poll)
 		features |= NETIF_F_BUSY_POLL;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1260850 — Re: [PATCH net-next] net/core: generic support for disabling netdev features down stack

On 11/02/2015 09:53 AM, Jarod Wilson wrote:
> 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>
> ---
> Note: this replaces "[RFC PATCH net-next] net/core: initial support for
> stacked dev feature toggles" for consideration.
>
>   include/linux/netdev_features.h | 11 +++++++++
>   net/core/dev.c                  | 52 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 63 insertions(+)
>
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 9672781..0f5837a 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -125,6 +125,11 @@ enum {
>   #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
>   #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
>
> +#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 | \
> @@ -167,6 +172,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/net/core/dev.c b/net/core/dev.c
> index 13f49f8..3a8dbbc 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6288,9 +6288,51 @@ 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);
> +			upper->wanted_features &= ~feature;

Isn't this line redundant? The upper device should have already cleared 
the bit from the wanted_features?  That is unless the ndo_fix_features 
call modified it in which case we shouldn't be modifying it ourselves.

> +			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)
>   {
> +	struct net_device *upper, *lower;
> +	struct list_head *iter;
> +
>   	/* Fix illegal checksum combinations */
>   	if ((features & NETIF_F_HW_CSUM) &&
>   	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> @@ -6345,6 +6387,16 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>   		}
>   	}
>
> +	/* 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);
> +
> +	/* 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);
> +

I don't know if this is the right spot for this.  You might want to look 
at placing this after the ndo_set_features call to handle things if 
there wasn't an error.  That way if a lower device for some reason has 
an issue with one of the other settings being changed you don't end up 
in a state where all the lower devices have the feature stripped while 
the upper device still reports it as being enabled.

>   #ifdef CONFIG_NET_RX_BUSY_POLL
>   	if (dev->netdev_ops->ndo_busy_poll)
>   		features |= NETIF_F_BUSY_POLL;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261011 — Re: [PATCH net-next] net/core: generic support for disabling netdev features down stack

Alexander Duyck wrote:
> On 11/02/2015 09:53 AM, Jarod Wilson wrote:
>> 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>
>> ---
>> Note: this replaces "[RFC PATCH net-next] net/core: initial support for
>> stacked dev feature toggles" for consideration.
>>
>> include/linux/netdev_features.h | 11 +++++++++
>> net/core/dev.c | 52 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 63 insertions(+)
>>
>> diff --git a/include/linux/netdev_features.h
>> b/include/linux/netdev_features.h
>> index 9672781..0f5837a 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -125,6 +125,11 @@ enum {
>> #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD)
>> #define NETIF_F_BUSY_POLL __NETIF_F(BUSY_POLL)
>>
>> +#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 | \
>> @@ -167,6 +172,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/net/core/dev.c b/net/core/dev.c
>> index 13f49f8..3a8dbbc 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6288,9 +6288,51 @@ 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);
>> + upper->wanted_features &= ~feature;
>
> Isn't this line redundant? The upper device should have already cleared
> the bit from the wanted_features? That is unless the ndo_fix_features
> call modified it in which case we shouldn't be modifying it ourselves.

With the upper/lower feature sync calls where they currently are, no, 
its actually necessary to prevent flip-flopping flag values.

...
>> @@ -6345,6 +6387,16 @@ static netdev_features_t
>> netdev_fix_features(struct net_device *dev,
>> }
>> }
>>
>> + /* 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);
>> +
>> + /* 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);
>> +
>
> I don't know if this is the right spot for this. You might want to look
> at placing this after the ndo_set_features call to handle things if
> there wasn't an error. That way if a lower device for some reason has an
> issue with one of the other settings being changed you don't end up in a
> state where all the lower devices have the feature stripped while the
> upper device still reports it as being enabled.

I'll give it a go after the .ndo_set_features calls in 
__netdev_update_features(), see what shakes loose. Might even not need 
the extra upper->wanted_features twiddling with it here.


-- 
Jarod Wilson
jarod@redhat.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261169 — [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

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>
---
v2: Move manipulation of lower devices after ndo_set_features so the upper
device gets fully set up first and remove now redundant flag clearing, as
suggested by Alex Duyck. Filtering features on a current lower device
needs to be done prior to ndo_set_features, but both are now in
__netdev_update_features().

 include/linux/netdev_features.h | 11 +++++++++
 net/core/dev.c                  | 50 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 9672781..0f5837a 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -125,6 +125,11 @@ enum {
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
 
+#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 | \
@@ -167,6 +172,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/net/core/dev.c b/net/core/dev.c
index 13f49f8..c4d2b43 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6288,6 +6288,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)
 {
@@ -6357,7 +6395,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();
@@ -6370,6 +6410,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;
 
@@ -6386,6 +6430,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;
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261195 — Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

From: Jarod Wilson <jarod@redhat.com>
Date: Mon,  2 Nov 2015 21:55:59 -0500

> 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.
 ...
> 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.
 ...
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Looks good to me, applied, thanks Jarod.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261358 — Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

On 11/03/2015 03:55 AM, Jarod Wilson wrote:
[snip]
>  
> +#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);
> +
^
This is broken, it will not work for more than a single feature.

>  /* Features valid for ethtool to change */
>  /* = all defined minus driver/device-class-related */
>  #define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
> @@ -167,6 +172,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
> +
[snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261511 — Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
> [snip]
>>
>> +#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);
>> +
> ^
> This is broken, it will not work for more than a single feature.

Indeed it is.

This is used as:

        for_each_netdev_feature(&upper_disables, feature) {
        ...
        }

which expands to:

        int bit;
        for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
                feature = __NETIF_F_BIT(bit);
        {
                ...
        }

Note the assignment to "feature" happens outside the {}-delimited block.
And the block is always executed once.

Interestingly, arm-unknown-linux-gnueabi-gcc 4.9.0 warns about this in
some of my configs, but not in all of them:

net/core/dev.c: In function '__netdev_update_features':
net/core/dev.c:6302:16: warning: 'feature' may be used uninitialized
in this function [-Wmaybe-uninitialized]
    features &= ~feature;
                ^
net/core/dev.c:6295:20: note: 'feature' was declared here
  netdev_features_t feature;
                    ^
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261512 — Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

Geert Uytterhoeven wrote:
> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com>  wrote:
>> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
>> [snip]
>>> +#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);
>>> +
>> ^
>> This is broken, it will not work for more than a single feature.
>
> Indeed it is.
>
> This is used as:
>
>          for_each_netdev_feature(&upper_disables, feature) {
>          ...
>          }
>
> which expands to:
>
>          int bit;
>          for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
>                  feature = __NETIF_F_BIT(bit);
>          {
>                  ...
>          }
>
> Note the assignment to "feature" happens outside the {}-delimited block.
> And the block is always executed once.

Bah, crap, I was still staring at the code not seeing it, thank you for 
the detailed cluebat. I'll fix that up right now.

-- 
Jarod Wilson
jarod@redhat.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261520 — Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

On 11/03/2015 02:57 PM, Jarod Wilson wrote:
> Geert Uytterhoeven wrote:
>> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
>> <nikolay@cumulusnetworks.com>  wrote:
>>> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
>>> [snip]
>>>> +#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);
>>>> +
>>> ^
>>> This is broken, it will not work for more than a single feature.
>>
>> Indeed it is.
>>
>> This is used as:
>>
>>          for_each_netdev_feature(&upper_disables, feature) {
>>          ...
>>          }
>>
>> which expands to:
>>
>>          int bit;
>>          for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
>>                  feature = __NETIF_F_BIT(bit);
>>          {
>>                  ...
>>          }
>>
>> Note the assignment to "feature" happens outside the {}-delimited block.
>> And the block is always executed once.
> 
> Bah, crap, I was still staring at the code not seeing it, thank you for the detailed cluebat. I'll fix that up right now.
> 

Yeah, sorry for not elaborating, I wrote it in a hurry. :-)
Thanks Geert!

By the way since you'll be changing this code, I don't know if it's okay to
declare caller-visible hidden local variables in a macro like this, at the very
least please consider renaming it to something that's much less common, I can see
"bit" being used here and there. IMO either try to find a way to avoid it
altogether or add another argument to the macro so it's explicitly passed.

Cheers,
 Nik


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261667 — Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

Nikolay Aleksandrov wrote:
> On 11/03/2015 02:57 PM, Jarod Wilson wrote:
>> Geert Uytterhoeven wrote:
>>> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
>>> <nikolay@cumulusnetworks.com>   wrote:
>>>> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
>>>> [snip]
>>>>> +#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);
>>>>> +
>>>> ^
>>>> This is broken, it will not work for more than a single feature.
>>> Indeed it is.
>>>
>>> This is used as:
>>>
>>>           for_each_netdev_feature(&upper_disables, feature) {
>>>           ...
>>>           }
>>>
>>> which expands to:
>>>
>>>           int bit;
>>>           for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
>>>                   feature = __NETIF_F_BIT(bit);
>>>           {
>>>                   ...
>>>           }
>>>
>>> Note the assignment to "feature" happens outside the {}-delimited block.
>>> And the block is always executed once.
>> Bah, crap, I was still staring at the code not seeing it, thank you for the detailed cluebat. I'll fix that up right now.
>>
>
> Yeah, sorry for not elaborating, I wrote it in a hurry. :-)
> Thanks Geert!
>
> By the way since you'll be changing this code, I don't know if it's okay to
> declare caller-visible hidden local variables in a macro like this, at the very
> least please consider renaming it to something that's much less common, I can see
> "bit" being used here and there. IMO either try to find a way to avoid it
> altogether or add another argument to the macro so it's explicitly passed.

Just posted a follow-up that removes the macro-internal use of bit and 
doesn't botch up assigning feature. It's not as pretty, but it works 
correctly with multiple feature bits.

-- 
Jarod Wilson
jarod@redhat.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261577 — [PATCH net-next] net/core: fix for_each_netdev_feature

As pointed out by Nikolay and further explained by Geert, the initial
for_each_netdev_feature macro was broken, as feature would get set outside
of the block of code it was intended to run in, thus only ever working for
the first feature bit in the mask. While less pretty this way, this is
tested and confirmed functional with multiple feature bits set in
NETIF_F_UPPER_DISABLES.

[root@dell-per730-01 ~]# ethtool -K bond0 lro off
...
[  242.761394] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
[  243.552178] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
[  244.353978] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
[  245.147420] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71

[root@dell-per730-01 ~]# ethtool -K bond0 gro off
...
[  251.925645] bond0: Disabling feature 0x0000000000004000 on lower dev p5p2.
[  252.713693] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
[  253.499085] bond0: Disabling feature 0x0000000000004000 on lower dev p5p1.
[  254.290922] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71

Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
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: Geert Uytterhoeven <geert@linux-m68k.org>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 include/linux/netdev_features.h | 6 ++----
 net/core/dev.c                  | 8 ++++++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 0f5837a..f0d8734 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -125,10 +125,8 @@ enum {
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
 
-#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);
+#define for_each_netdev_feature(mask_addr, bit)	\
+	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
 
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
diff --git a/net/core/dev.c b/net/core/dev.c
index c4d2b43..8ce3f74 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6293,8 +6293,10 @@ static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
 {
 	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
 	netdev_features_t feature;
+	int feature_bit;
 
-	for_each_netdev_feature(&upper_disables, feature) {
+	for_each_netdev_feature(&upper_disables, feature_bit) {
+		feature = __NETIF_F_BIT(feature_bit);
 		if (!(upper->wanted_features & feature)
 		    && (features & feature)) {
 			netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
@@ -6311,8 +6313,10 @@ static void netdev_sync_lower_features(struct net_device *upper,
 {
 	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
 	netdev_features_t feature;
+	int feature_bit;
 
-	for_each_netdev_feature(&upper_disables, feature) {
+	for_each_netdev_feature(&upper_disables, feature_bit) {
+		feature = __NETIF_F_BIT(feature_bit);
 		if (!(features & feature) && (lower->features & feature)) {
 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
 				   &feature, lower->name);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261637 — Re: [PATCH net-next] net/core: fix for_each_netdev_feature

On 11/03/2015 04:15 PM, Jarod Wilson wrote:
> As pointed out by Nikolay and further explained by Geert, the initial
> for_each_netdev_feature macro was broken, as feature would get set outside
> of the block of code it was intended to run in, thus only ever working for
> the first feature bit in the mask. While less pretty this way, this is
> tested and confirmed functional with multiple feature bits set in
> NETIF_F_UPPER_DISABLES.
> 
> [root@dell-per730-01 ~]# ethtool -K bond0 lro off
> ...
> [  242.761394] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
> [  243.552178] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
> [  244.353978] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
> [  245.147420] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71
> 
> [root@dell-per730-01 ~]# ethtool -K bond0 gro off
> ...
> [  251.925645] bond0: Disabling feature 0x0000000000004000 on lower dev p5p2.
> [  252.713693] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
> [  253.499085] bond0: Disabling feature 0x0000000000004000 on lower dev p5p1.
> [  254.290922] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71
> 
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> 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: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  include/linux/netdev_features.h | 6 ++----
>  net/core/dev.c                  | 8 ++++++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 

Looks good to me especially without the hidden side-effects,

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Thanks,
 Nik

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261704 — Re: [PATCH net-next] net/core: fix for_each_netdev_feature

From: Jarod Wilson <jarod@redhat.com>
Date: Tue,  3 Nov 2015 10:15:59 -0500

> As pointed out by Nikolay and further explained by Geert, the initial
> for_each_netdev_feature macro was broken, as feature would get set outside
> of the block of code it was intended to run in, thus only ever working for
> the first feature bit in the mask. While less pretty this way, this is
> tested and confirmed functional with multiple feature bits set in
> NETIF_F_UPPER_DISABLES.
 ...
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
 ...
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261879 — [PATCH net-next] net/core: ensure features get disabled on new lower devs

With moving netdev_sync_lower_features() after the .ndo_set_features
calls, I neglected to verify that devices added *after* a flag had been
disabled on an upper device were properly added with that flag disabled as
well. This currently happens, because we exit __netdev_update_features()
when we see dev->features == features for the upper dev. We can retain the
optimization of leaving without calling .ndo_set_features with a bit of
tweaking and a goto here.

Changing err to ret was somewhat arbitrary and makes the patch look more
involved, but seems to better fit the altered use.

Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
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>
---
 net/core/dev.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8ce3f74..90e0a62 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6402,7 +6402,7 @@ int __netdev_update_features(struct net_device *dev)
 	struct net_device *upper, *lower;
 	netdev_features_t features;
 	struct list_head *iter;
-	int err = 0;
+	int ret = 0;
 
 	ASSERT_RTNL();
 
@@ -6419,31 +6419,34 @@ int __netdev_update_features(struct net_device *dev)
 		features = netdev_sync_upper_features(dev, upper, features);
 
 	if (dev->features == features)
-		return 0;
+		goto sync_lower;
 
 	netdev_dbg(dev, "Features changed: %pNF -> %pNF\n",
 		&dev->features, &features);
 
 	if (dev->netdev_ops->ndo_set_features)
-		err = dev->netdev_ops->ndo_set_features(dev, features);
+		ret = dev->netdev_ops->ndo_set_features(dev, features);
 
-	if (unlikely(err < 0)) {
+	if (unlikely(ret < 0)) {
 		netdev_err(dev,
 			"set_features() failed (%d); wanted %pNF, left %pNF\n",
-			err, &features, &dev->features);
+			ret, &features, &dev->features);
 		return -1;
 	}
 
+	if (!ret) {
+		dev->features = features;
+		ret = 1;
+	}
+
+sync_lower:
 	/* 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;
-
-	return 1;
+	return ret;
 }
 
 /**
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261901 — Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

On 11/03/2015 12:36 PM, Jarod Wilson wrote:
> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.
>
> Changing err to ret was somewhat arbitrary and makes the patch look more
> involved, but seems to better fit the altered use.
>
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> 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>
> ---
>   net/core/dev.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8ce3f74..90e0a62 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6402,7 +6402,7 @@ int __netdev_update_features(struct net_device *dev)
>   	struct net_device *upper, *lower;
>   	netdev_features_t features;
>   	struct list_head *iter;
> -	int err = 0;
> +	int ret = 0;
>
>   	ASSERT_RTNL();
>
> @@ -6419,31 +6419,34 @@ int __netdev_update_features(struct net_device *dev)
>   		features = netdev_sync_upper_features(dev, upper, features);
>
>   	if (dev->features == features)
> -		return 0;
> +		goto sync_lower;
>
>   	netdev_dbg(dev, "Features changed: %pNF -> %pNF\n",
>   		&dev->features, &features);
>
>   	if (dev->netdev_ops->ndo_set_features)
> -		err = dev->netdev_ops->ndo_set_features(dev, features);
> +		ret = dev->netdev_ops->ndo_set_features(dev, features);
>
> -	if (unlikely(err < 0)) {
> +	if (unlikely(ret < 0)) {
>   		netdev_err(dev,
>   			"set_features() failed (%d); wanted %pNF, left %pNF\n",
> -			err, &features, &dev->features);
> +			ret, &features, &dev->features);
>   		return -1;
>   	}
>
> +	if (!ret) {
> +		dev->features = features;
> +		ret = 1;
> +	}
> +

I would take the "ret = 1;" out of the if statement and let it stay here 
by itself.  Technically anything that traversed this path was returning 
1 previously so we probably want to retain that behavior.

> +sync_lower:
>   	/* 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;

You could just alter the if statement here to check for a non-zero ret 
value since you should have it as either 0 or 1.  It shouldn't have any 
other values.

That way you will have disabled the feature on the lower devices before 
advertising that it has been disabled on the upper device.

> -	return 1;
> +	return ret;
>   }
>
>   /**
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261928 — Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

Alexander Duyck wrote:
> On 11/03/2015 12:36 PM, Jarod Wilson wrote:
>> With moving netdev_sync_lower_features() after the .ndo_set_features
>> calls, I neglected to verify that devices added *after* a flag had been
>> disabled on an upper device were properly added with that flag
>> disabled as
>> well. This currently happens, because we exit __netdev_update_features()
>> when we see dev->features == features for the upper dev. We can retain
>> the
>> optimization of leaving without calling .ndo_set_features with a bit of
>> tweaking and a goto here.
>>
>> Changing err to ret was somewhat arbitrary and makes the patch look more
>> involved, but seems to better fit the altered use.
...
>> + 	if (!ret) {
>> + 		dev->features = features;
>> + 		ret = 1;
>> + 	}
>> +
>
> I would take the "ret = 1;" out of the if statement and let it stay here
> by itself. Technically anything that traversed this path was returning 1
> previously so we probably want to retain that behavior.

Ah, that. I took a look at all the callers of __netdev_update_features, 
and most don't even check return value, the one that does 
(netdev_update_features) only cares if its zero or not zero, so I 
figured it didn't really matter here, but it would indeed return 2 now 
instead of 1, if it got that from ndo_set_features. For consistency's 
sake, I can respin and just always set ret = 1 though.

>> +sync_lower:
>> 	/* 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;
>
> You could just alter the if statement here to check for a non-zero ret
> value since you should have it as either 0 or 1. It shouldn't have any
> other values.
>
> That way you will have disabled the feature on the lower devices before
> advertising that it has been disabled on the upper device.

If this check is down here, the goto will trigger, setting dev->features 
= features, but then, we got there because dev->features == features 
already, so meh. But it would also NOT trigger in the case of 
ndo_set_features returning 0 anymore, because we set ret = 1. Or am I 
missing something or misunderstanding what you're suggesting here?

-- 
Jarod Wilson
jarod@redhat.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261948 — Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

On 11/03/2015 02:11 PM, Jarod Wilson wrote:
> Alexander Duyck wrote:
>> On 11/03/2015 12:36 PM, Jarod Wilson wrote:
>>> With moving netdev_sync_lower_features() after the .ndo_set_features
>>> calls, I neglected to verify that devices added *after* a flag had been
>>> disabled on an upper device were properly added with that flag
>>> disabled as
>>> well. This currently happens, because we exit 
>>> __netdev_update_features()
>>> when we see dev->features == features for the upper dev. We can retain
>>> the
>>> optimization of leaving without calling .ndo_set_features with a bit of
>>> tweaking and a goto here.
>>>
>>> Changing err to ret was somewhat arbitrary and makes the patch look 
>>> more
>>> involved, but seems to better fit the altered use.
> ...
>>> +     if (!ret) {
>>> +         dev->features = features;
>>> +         ret = 1;
>>> +     }
>>> +
>>
>> I would take the "ret = 1;" out of the if statement and let it stay here
>> by itself. Technically anything that traversed this path was returning 1
>> previously so we probably want to retain that behavior.
>
> Ah, that. I took a look at all the callers of 
> __netdev_update_features, and most don't even check return value, the 
> one that does (netdev_update_features) only cares if its zero or not 
> zero, so I figured it didn't really matter here, but it would indeed 
> return 2 now instead of 1, if it got that from ndo_set_features. For 
> consistency's sake, I can respin and just always set ret = 1 though.

One thought I just had would be to make it so that we assign -1 to ret 
and then jump inside the earlier features==features check rather than 
altering the ret value here.  Then we could just use a ternary value at 
the end and just do "return ret < 0 ? 0 : 1;".  That would take care of 
the return values and the features flag you called out below.

>>> +sync_lower:
>>>     /* 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;
>>
>> You could just alter the if statement here to check for a non-zero ret
>> value since you should have it as either 0 or 1. It shouldn't have any
>> other values.
>>
>> That way you will have disabled the feature on the lower devices before
>> advertising that it has been disabled on the upper device.
>
> If this check is down here, the goto will trigger, setting 
> dev->features = features, but then, we got there because dev->features 
> == features already, so meh. But it would also NOT trigger in the case 
> of ndo_set_features returning 0 anymore, because we set ret = 1. Or am 
> I missing something or misunderstanding what you're suggesting here?
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261906 — Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

On 11/03/2015 09:36 PM, Jarod Wilson wrote:
> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.
> 
> Changing err to ret was somewhat arbitrary and makes the patch look more
> involved, but seems to better fit the altered use.
> 
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> 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>
> ---
>  net/core/dev.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 

Thanks for the quick response,

Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Tested-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261921 — Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

On Tue, Nov 03, 2015 at 03:36:57PM -0500, Jarod Wilson wrote:
> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.

I haven't reviewed the patch yet (I'm going to take a look with fresher
mind in the morning) but if you are going to handle this in a generic
way, you may want to remove the LRO specific hacks added to
bond_enslave() and team_port_add() by commit fbe168ba91f7 ("net: generic
dev_disable_lro() stacked device handling").

                                                          Michal Kubecek

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261922 — Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

Michal Kubecek wrote:
> On Tue, Nov 03, 2015 at 03:36:57PM -0500, Jarod Wilson wrote:
>> With moving netdev_sync_lower_features() after the .ndo_set_features
>> calls, I neglected to verify that devices added *after* a flag had been
>> disabled on an upper device were properly added with that flag disabled as
>> well. This currently happens, because we exit __netdev_update_features()
>> when we see dev->features == features for the upper dev. We can retain the
>> optimization of leaving without calling .ndo_set_features with a bit of
>> tweaking and a goto here.
>
> I haven't reviewed the patch yet (I'm going to take a look with fresher
> mind in the morning) but if you are going to handle this in a generic
> way, you may want to remove the LRO specific hacks added to
> bond_enslave() and team_port_add() by commit fbe168ba91f7 ("net: generic
> dev_disable_lro() stacked device handling").

Yeah, that's on tap for a follow-up series. I wanted to get all this in 
first and soaked before pulling those bits out. My current 
haven't-yet-fully-investigated thinking is that dev_disable_lro() can go 
away entirely, not just the hacks added to bond and team.

-- 
Jarod Wilson
jarod@redhat.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web