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

Groups > linux.kernel > #1255121 > unrolled thread

[RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Jarod Wilson <jarod@redhat.com> - 2015-10-24 05:50 +0200
    Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Tom Herbert <tom@herbertland.com> - 2015-10-24 06:50 +0200
    Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-24 08:00 +0200
      Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Michal Kubecek <mkubecek@suse.cz> - 2015-10-26 10:50 +0100
        Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Jarod Wilson <jarod@redhat.com> - 2015-10-30 17:30 +0100
          Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-30 21:10 +0100
      Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Jarod Wilson <jarod@redhat.com> - 2015-10-30 17:40 +0100
        Re: [RFC PATCH net-next] net/core: initial support for stacked dev  feature toggles Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-30 21:20 +0100

#1255121 — [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

There are some netdev features that make little sense to toggle on and
off in a stacked device setup on only one device in the stack. The prime
example is a bonded connection, where it really doesn't make sense to
disable LRO on the master, but not on any of the slaves, nor does it
really make sense to be able to shut LRO off on a slave when its still
enabled on the master.

The strategy here is to add a section near the end of
netdev_fix_features() that looks for upper and lower netdevs, then make
sure certain feature flags match both up and down the stack. At present,
only the LRO flag is included.

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: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 net/core/dev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1225b4b..26f4e2d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6261,9 +6261,57 @@ 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 want = upper->wanted_features & lower->hw_features;
+
+	if (!(upper->wanted_features & NETIF_F_LRO)
+	    && (features & NETIF_F_LRO)) {
+		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
+			   upper->name);
+		features &= ~NETIF_F_LRO;
+	} else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
+		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
+			   upper->name);
+		features |= NETIF_F_LRO;
+	}
+
+	return features;
+}
+
+static void netdev_sync_lower_features(struct net_device *upper,
+	struct net_device *lower, netdev_features_t features)
+{
+	netdev_features_t want = features & lower->hw_features;
+
+	if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
+		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
+			   lower->name);
+		upper->wanted_features &= ~NETIF_F_LRO;
+		lower->wanted_features &= ~NETIF_F_LRO;
+		netdev_update_features(lower);
+		if (unlikely(lower->features & NETIF_F_LRO))
+			netdev_WARN(upper, "failed to disable LRO on %s!\n",
+				    lower->name);
+	} else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
+		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
+			   lower->name);
+		upper->wanted_features |= NETIF_F_LRO;
+		lower->wanted_features |= NETIF_F_LRO;
+		netdev_update_features(lower);
+		if (unlikely(!(lower->features & NETIF_F_LRO)))
+			netdev_WARN(upper, "failed to enable LRO on %s!\n",
+				    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))) {
@@ -6318,6 +6366,15 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		}
 	}
 
+	/* some features should be kept in sync with upper devices */
+	upper = netdev_master_upper_dev_get(dev);
+	if (upper)
+		features = netdev_sync_upper_features(dev, upper, features);
+
+	/* lower devices need some features altered to match upper devices */
+	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]

#1255130 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

On Fri, Oct 23, 2015 at 11:40 PM, Jarod Wilson <jarod@redhat.com> wrote:
> There are some netdev features that make little sense to toggle on and
> off in a stacked device setup on only one device in the stack. The prime
> example is a bonded connection, where it really doesn't make sense to
> disable LRO on the master, but not on any of the slaves, nor does it
> really make sense to be able to shut LRO off on a slave when its still
> enabled on the master.
>
> The strategy here is to add a section near the end of
> netdev_fix_features() that looks for upper and lower netdevs, then make
> sure certain feature flags match both up and down the stack. At present,
> only the LRO flag is included.
>
> 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.
>
I don't see what real problem this is solving. LRO is purely a feature
of physical devices and should be irrelevant to be configured on any
type of virtual device. I think the same thing will be true of RX csum
and other device RX functions (but this is not true for transmit
features). Seems like a better fix might be to disallow setting these
features on the bonding device in the first place, then we don't need
to worry about syncing them amongst slaves-- if a user needs that it's
a simple script.

Tom

> 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: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  net/core/dev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1225b4b..26f4e2d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6261,9 +6261,57 @@ 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 want = upper->wanted_features & lower->hw_features;
> +
> +       if (!(upper->wanted_features & NETIF_F_LRO)
> +           && (features & NETIF_F_LRO)) {
> +               netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
> +                          upper->name);
> +               features &= ~NETIF_F_LRO;
> +       } else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
> +               netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
> +                          upper->name);
> +               features |= NETIF_F_LRO;
> +       }
> +
> +       return features;
> +}
> +
> +static void netdev_sync_lower_features(struct net_device *upper,
> +       struct net_device *lower, netdev_features_t features)
> +{
> +       netdev_features_t want = features & lower->hw_features;
> +
> +       if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
> +               netdev_info(upper, "Disabling LRO on lower dev %s.\n",
> +                          lower->name);
> +               upper->wanted_features &= ~NETIF_F_LRO;
> +               lower->wanted_features &= ~NETIF_F_LRO;
> +               netdev_update_features(lower);
> +               if (unlikely(lower->features & NETIF_F_LRO))
> +                       netdev_WARN(upper, "failed to disable LRO on %s!\n",
> +                                   lower->name);
> +       } else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
> +               netdev_info(upper, "Enabling LRO on lower dev %s.\n",
> +                          lower->name);
> +               upper->wanted_features |= NETIF_F_LRO;
> +               lower->wanted_features |= NETIF_F_LRO;
> +               netdev_update_features(lower);
> +               if (unlikely(!(lower->features & NETIF_F_LRO)))
> +                       netdev_WARN(upper, "failed to enable LRO on %s!\n",
> +                                   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))) {
> @@ -6318,6 +6366,15 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>                 }
>         }
>
> +       /* some features should be kept in sync with upper devices */
> +       upper = netdev_master_upper_dev_get(dev);
> +       if (upper)
> +               features = netdev_sync_upper_features(dev, upper, features);
> +
> +       /* lower devices need some features altered to match upper devices */
> +       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 netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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]

#1255133 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

On 10/23/2015 08:40 PM, Jarod Wilson wrote:
> There are some netdev features that make little sense to toggle on and
> off in a stacked device setup on only one device in the stack. The prime
> example is a bonded connection, where it really doesn't make sense to
> disable LRO on the master, but not on any of the slaves, nor does it
> really make sense to be able to shut LRO off on a slave when its still
> enabled on the master.
>
> The strategy here is to add a section near the end of
> netdev_fix_features() that looks for upper and lower netdevs, then make
> sure certain feature flags match both up and down the stack. At present,
> only the LRO flag is included.
>
> 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: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>   net/core/dev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1225b4b..26f4e2d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6261,9 +6261,57 @@ 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 want = upper->wanted_features & lower->hw_features;
> +
> +	if (!(upper->wanted_features & NETIF_F_LRO)
> +	    && (features & NETIF_F_LRO)) {
> +		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
> +			   upper->name);
> +		features &= ~NETIF_F_LRO;
> +	} else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
> +		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
> +			   upper->name);
> +		features |= NETIF_F_LRO;
> +	}
> +
> +	return features;
> +}
> +

I'd say to drop the second half of this statement.  LRO is a feature 
that should be enabled explicitly per interface.  If someone enables LRO 
on the master they may only want it on one interface.  The fact is there 
are some implementations of LRO that work better than others so you want 
to give the end user the option to mix and match.

> +static void netdev_sync_lower_features(struct net_device *upper,
> +	struct net_device *lower, netdev_features_t features)
> +{
> +	netdev_features_t want = features & lower->hw_features;
> +
> +	if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
> +		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
> +			   lower->name);
> +		upper->wanted_features &= ~NETIF_F_LRO;
> +		lower->wanted_features &= ~NETIF_F_LRO;
> +		netdev_update_features(lower);
> +		if (unlikely(lower->features & NETIF_F_LRO))
> +			netdev_WARN(upper, "failed to disable LRO on %s!\n",
> +				    lower->name);
> +	} else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
> +		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
> +			   lower->name);
> +		upper->wanted_features |= NETIF_F_LRO;
> +		lower->wanted_features |= NETIF_F_LRO;
> +		netdev_update_features(lower);
> +		if (unlikely(!(lower->features & NETIF_F_LRO)))
> +			netdev_WARN(upper, "failed to enable LRO on %s!\n",
> +				    lower->name);
> +	}
> +}
> +

Same thing here.  If a lower dev has it disabled then leave it disabled. 
  I believe your goal is to make it so that dev_disable_lro() can shut 
down LRO when it is making packets in the data-path unusable.  There is 
no need to make this an all or nothing scenario.  We can let the stack 
slam things down with dev_disable_lro() and then if a user so desires 
they can come back through and enable LRO more selectively if they for 
instance have an interface that can do a smarter job of putting together 
frames that could be routed.

You could probably look at doing something like this for RXCSUM as well. 
  The general idea is that if an upper device has it off then the value 
has to be off.  For example if RXCSUM is off in a upper device and LRO 
is enabled on the lower device there is a good chance that the upper 
device will report checksum errors since most LRO implementations don't 
recalculate the checksum.  If RXCSUM is forced down to the lower device 
hopefully its fix_features will know this and disable LRO on that device 
when the RXCSUM is disabled on it.

>   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))) {
> @@ -6318,6 +6366,15 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>   		}
>   	}
>
> +	/* some features should be kept in sync with upper devices */
> +	upper = netdev_master_upper_dev_get(dev);
> +	if (upper)
> +		features = netdev_sync_upper_features(dev, upper, features);
> +
> +	/* lower devices need some features altered to match upper devices */
> +	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;
>


--
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]

#1255777 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
> >
> >+static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
> >+	struct net_device *upper, netdev_features_t features)
> >+{
> >+	netdev_features_t want = upper->wanted_features & lower->hw_features;
> >+
> >+	if (!(upper->wanted_features & NETIF_F_LRO)
> >+	    && (features & NETIF_F_LRO)) {
> >+		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
> >+			   upper->name);
> >+		features &= ~NETIF_F_LRO;
> >+	} else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
> >+		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
> >+			   upper->name);
> >+		features |= NETIF_F_LRO;
> >+	}
> >+
> >+	return features;
> >+}
> >+
> 
> I'd say to drop the second half of this statement.  LRO is a feature
> that should be enabled explicitly per interface.  If someone enables
> LRO on the master they may only want it on one interface.  The fact
> is there are some implementations of LRO that work better than
> others so you want to give the end user the option to mix and match.

Agreed. IMHO it makes sense to allow setups with LRO disabled on some
slaves and enabled on other.

Also, the logic seems to only consider the 1 upper : N lower scheme
(bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
For these, there is no way to propagate both 0 and 1 down as this would
result in a conflict.

> >+static void netdev_sync_lower_features(struct net_device *upper,
> >+	struct net_device *lower, netdev_features_t features)
> >+{
> >+	netdev_features_t want = features & lower->hw_features;
> >+
> >+	if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
> >+		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
> >+			   lower->name);
> >+		upper->wanted_features &= ~NETIF_F_LRO;
> >+		lower->wanted_features &= ~NETIF_F_LRO;
> >+		netdev_update_features(lower);
> >+		if (unlikely(lower->features & NETIF_F_LRO))
> >+			netdev_WARN(upper, "failed to disable LRO on %s!\n",
> >+				    lower->name);
> >+	} else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
> >+		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
> >+			   lower->name);
> >+		upper->wanted_features |= NETIF_F_LRO;
> >+		lower->wanted_features |= NETIF_F_LRO;
> >+		netdev_update_features(lower);
> >+		if (unlikely(!(lower->features & NETIF_F_LRO)))
> >+			netdev_WARN(upper, "failed to enable LRO on %s!\n",
> >+				    lower->name);
> >+	}
> >+}
> >+
> 
> Same thing here.  If a lower dev has it disabled then leave it
> disabled.  I believe your goal is to make it so that
> dev_disable_lro() can shut down LRO when it is making packets in the
> data-path unusable.

This is already the case since commit fbe168ba91f7 ("net: generic
dev_disable_lro() stacked device handling"). That commit makes sure
dev_disable_lro() is propagated down the stack and also makes sure new
slaves added to a bond/team with LRO disabled have it disabled too.

What it does not do is propagating LRO disabling down if it is disabled
in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
sure if this should be done or not, both options have their pros and
cons. However, I believe enabling LRO shouldn't be propagated down.

                                                         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]

#1259607 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

Michal Kubecek wrote:
> On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
>>> +	struct net_device *upper, netdev_features_t features)
>>> +{
>>> +	netdev_features_t want = upper->wanted_features&  lower->hw_features;
>>> +
>>> +	if (!(upper->wanted_features&  NETIF_F_LRO)
>>> +	&&  (features&  NETIF_F_LRO)) {
>>> +		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
>>> +			   upper->name);
>>> +		features&= ~NETIF_F_LRO;
>>> +	} else if ((want&  NETIF_F_LRO)&&  !(features&  NETIF_F_LRO)) {
>>> +		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
>>> +			   upper->name);
>>> +		features |= NETIF_F_LRO;
>>> +	}
>>> +
>>> +	return features;
>>> +}
>>> +
>> I'd say to drop the second half of this statement.  LRO is a feature
>> that should be enabled explicitly per interface.  If someone enables
>> LRO on the master they may only want it on one interface.  The fact
>> is there are some implementations of LRO that work better than
>> others so you want to give the end user the option to mix and match.
>
> Agreed. IMHO it makes sense to allow setups with LRO disabled on some
> slaves and enabled on other.
>
> Also, the logic seems to only consider the 1 upper : N lower scheme
> (bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
> For these, there is no way to propagate both 0 and 1 down as this would
> result in a conflict.

Okay, so we're thinking do prevent lower devices turning LRO on if the 
upper device has it off. Or rather, if *an* upper device has it off. 
Probably need to rework the bit that calls this function to use 
netdev_for_each_upper_dev{_rcu}() to walk all of adj_list.upper here.

Rather than outright dropping the second bit though, I was thinking 
maybe just drop a note in dmesg along the lines of "hey, you shut off 
LRO, it is still enabled on upper dev foo", to placate end-users.


>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>> +	struct net_device *lower, netdev_features_t features)
>>> +{
>>> +	netdev_features_t want = features&  lower->hw_features;
>>> +
>>> +	if (!(features&  NETIF_F_LRO)&&  (lower->features&  NETIF_F_LRO)) {
>>> +		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>> +			   lower->name);
>>> +		upper->wanted_features&= ~NETIF_F_LRO;
>>> +		lower->wanted_features&= ~NETIF_F_LRO;
>>> +		netdev_update_features(lower);
>>> +		if (unlikely(lower->features&  NETIF_F_LRO))
>>> +			netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>> +				    lower->name);
>>> +	} else if ((want&  NETIF_F_LRO)&&  !(lower->features&  NETIF_F_LRO)) {
>>> +		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>> +			   lower->name);
>>> +		upper->wanted_features |= NETIF_F_LRO;
>>> +		lower->wanted_features |= NETIF_F_LRO;
>>> +		netdev_update_features(lower);
>>> +		if (unlikely(!(lower->features&  NETIF_F_LRO)))
>>> +			netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>> +				    lower->name);
>>> +	}
>>> +}
>>> +
>> Same thing here.  If a lower dev has it disabled then leave it
>> disabled.  I believe your goal is to make it so that
>> dev_disable_lro() can shut down LRO when it is making packets in the
>> data-path unusable.
>
> This is already the case since commit fbe168ba91f7 ("net: generic
> dev_disable_lro() stacked device handling"). That commit makes sure
> dev_disable_lro() is propagated down the stack and also makes sure new
> slaves added to a bond/team with LRO disabled have it disabled too.
>
> What it does not do is propagating LRO disabling down if it is disabled
> in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
> sure if this should be done or not, both options have their pros and
> cons.

Making it work with ethtool was one of my primary goals with this 
change, as it was users prodding things with ethtool that prompted the 
"hey, this doesn't make sense" bug reports.

> However, I believe enabling LRO shouldn't be propagated down.

Hm. Devices that should never have LRO enabled still won't get it 
enabled, so I'm not clear what harm it would cause. I tend to think you 
do want this sync'ing down the stack if set on an upper dev (i.e., 
ethtool -K bond0 lro on), for consistency's sake. You can always come 
back through afterwards and disable things on lower devs individually if 
they're really not wanted, since we're in agreement that we shouldn't 
prevent disabling features on lower devices.

-- 
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]

#1259762 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

On 10/30/2015 09:25 AM, Jarod Wilson wrote:
> Michal Kubecek wrote:
>> On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
>>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>>> +static netdev_features_t netdev_sync_upper_features(struct
>>>> net_device *lower,
>>>> +    struct net_device *upper, netdev_features_t features)
>>>> +{
>>>> +    netdev_features_t want = upper->wanted_features&
>>>> lower->hw_features;
>>>> +
>>>> +    if (!(upper->wanted_features&  NETIF_F_LRO)
>>>> +    &&  (features&  NETIF_F_LRO)) {
>>>> +        netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
>>>> +               upper->name);
>>>> +        features&= ~NETIF_F_LRO;
>>>> +    } else if ((want&  NETIF_F_LRO)&&  !(features&  NETIF_F_LRO)) {
>>>> +        netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
>>>> +               upper->name);
>>>> +        features |= NETIF_F_LRO;
>>>> +    }
>>>> +
>>>> +    return features;
>>>> +}
>>>> +
>>> I'd say to drop the second half of this statement.  LRO is a feature
>>> that should be enabled explicitly per interface.  If someone enables
>>> LRO on the master they may only want it on one interface.  The fact
>>> is there are some implementations of LRO that work better than
>>> others so you want to give the end user the option to mix and match.
>>
>> Agreed. IMHO it makes sense to allow setups with LRO disabled on some
>> slaves and enabled on other.
>>
>> Also, the logic seems to only consider the 1 upper : N lower scheme
>> (bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
>> For these, there is no way to propagate both 0 and 1 down as this would
>> result in a conflict.
>
> Okay, so we're thinking do prevent lower devices turning LRO on if the
> upper device has it off. Or rather, if *an* upper device has it off.
> Probably need to rework the bit that calls this function to use
> netdev_for_each_upper_dev{_rcu}() to walk all of adj_list.upper here.

Right.  This part sounds fine.

> Rather than outright dropping the second bit though, I was thinking
> maybe just drop a note in dmesg along the lines of "hey, you shut off
> LRO, it is still enabled on upper dev foo", to placate end-users.

I would rather not see it.  It would be mostly noise.  It is perfectly 
valid to have LRO advertised on an upper device, but not supported on a 
lower one.  It basically just means that the path will allow LRO frames 
through, it doesn't guarantee that we are going to provide them.

>>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>>> +    struct net_device *lower, netdev_features_t features)
>>>> +{
>>>> +    netdev_features_t want = features&  lower->hw_features;
>>>> +
>>>> +    if (!(features&  NETIF_F_LRO)&&  (lower->features&
>>>> NETIF_F_LRO)) {
>>>> +        netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>>> +               lower->name);
>>>> +        upper->wanted_features&= ~NETIF_F_LRO;
>>>> +        lower->wanted_features&= ~NETIF_F_LRO;
>>>> +        netdev_update_features(lower);
>>>> +        if (unlikely(lower->features&  NETIF_F_LRO))
>>>> +            netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>>> +                    lower->name);
>>>> +    } else if ((want&  NETIF_F_LRO)&&  !(lower->features&
>>>> NETIF_F_LRO)) {
>>>> +        netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>>> +               lower->name);
>>>> +        upper->wanted_features |= NETIF_F_LRO;
>>>> +        lower->wanted_features |= NETIF_F_LRO;
>>>> +        netdev_update_features(lower);
>>>> +        if (unlikely(!(lower->features&  NETIF_F_LRO)))
>>>> +            netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>>> +                    lower->name);
>>>> +    }
>>>> +}
>>>> +
>>> Same thing here.  If a lower dev has it disabled then leave it
>>> disabled.  I believe your goal is to make it so that
>>> dev_disable_lro() can shut down LRO when it is making packets in the
>>> data-path unusable.
>>
>> This is already the case since commit fbe168ba91f7 ("net: generic
>> dev_disable_lro() stacked device handling"). That commit makes sure
>> dev_disable_lro() is propagated down the stack and also makes sure new
>> slaves added to a bond/team with LRO disabled have it disabled too.
>>
>> What it does not do is propagating LRO disabling down if it is disabled
>> in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
>> sure if this should be done or not, both options have their pros and
>> cons.
>
> Making it work with ethtool was one of my primary goals with this
> change, as it was users prodding things with ethtool that prompted the
> "hey, this doesn't make sense" bug reports.

I'd say make it work like dev_disable_lro already does.  Disabling LRO 
propagates down, enabling LRO only enables it on the specific device.

The way to think of it is as a warning flag.  With LRO enabled this 
device may report frames larger than MTU to the stack and will mangle 
checksums.  Without LRO all of the frames received should be restricted 
to MTU.  That is why you have to force the disabling down to all lower 
devices, and why you cannot enable it if an upper device has it disabled.

>> However, I believe enabling LRO shouldn't be propagated down.
>
> Hm. Devices that should never have LRO enabled still won't get it
> enabled, so I'm not clear what harm it would cause.I tend to think you

How do you define "devices that should never have LRO enabled"?  The 
fact is LRO is very messy in terms of the way it functions.  Different 
drivers handle it different ways.  Usually it results in the Rx checksum 
being mangled, it provides frames larger than MTU, and uses fraglist 
instead of frags on some drivers.

> do want this sync'ing down the stack if set on an upper dev (i.e.,
> ethtool -K bond0 lro on), for consistency's sake. You can always come
> back through afterwards and disable things on lower devs individually if
> they're really not wanted, since we're in agreement that we shouldn't
> prevent disabling features on lower devices.

Think of it this way.  Lets say I have a NIC that I know is problematic 
when LRO is enabled, it might cause a kernel panic due to an skb 
overrun.  So I have a bond with it and some other NIC which can run with 
LRO enabled without issues.  How do I enable LRO on the other device 
without causing a kernel panic, and without tearing apart the existing 
bond?  With the approach you have described I can't because I have to 
enable it at the bond and doing so will enable it on the NIC with the 
faulty implementation.

This is why we cannot enable LRO unless all upper devices support it, 
and why we should propagate disabling LRO down to all lower devices. 
Trying to force it on for a lower device just because the upper device 
supports it is a bad idea because there are multiple LRO implementations 
and they all behave very differently.

- Alex

--
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]

#1259613 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

Alexander Duyck wrote:
> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>> There are some netdev features that make little sense to toggle on and
>> off in a stacked device setup on only one device in the stack. The prime
>> example is a bonded connection, where it really doesn't make sense to
>> disable LRO on the master, but not on any of the slaves, nor does it
>> really make sense to be able to shut LRO off on a slave when its still
>> enabled on the master.
>>
>> The strategy here is to add a section near the end of
>> netdev_fix_features() that looks for upper and lower netdevs, then make
>> sure certain feature flags match both up and down the stack. At present,
>> only the LRO flag is included.
...
>> +static void netdev_sync_lower_features(struct net_device *upper,
>> + struct net_device *lower, netdev_features_t features)
>> +{
>> + netdev_features_t want = features & lower->hw_features;
>> +
>> + if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
>> + netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>> + lower->name);
>> + upper->wanted_features &= ~NETIF_F_LRO;
>> + lower->wanted_features &= ~NETIF_F_LRO;
>> + netdev_update_features(lower);
>> + if (unlikely(lower->features & NETIF_F_LRO))
>> + netdev_WARN(upper, "failed to disable LRO on %s!\n",
>> + lower->name);
>> + } else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
>> + netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>> + lower->name);
>> + upper->wanted_features |= NETIF_F_LRO;
>> + lower->wanted_features |= NETIF_F_LRO;
>> + netdev_update_features(lower);
>> + if (unlikely(!(lower->features & NETIF_F_LRO)))
>> + netdev_WARN(upper, "failed to enable LRO on %s!\n",
>> + lower->name);
>> + }
>> +}
>> +
>
> Same thing here. If a lower dev has it disabled then leave it disabled.
> I believe your goal is to make it so that dev_disable_lro() can shut
> down LRO when it is making packets in the data-path unusable. There is
> no need to make this an all or nothing scenario. We can let the stack
> slam things down with dev_disable_lro() and then if a user so desires
> they can come back through and enable LRO more selectively if they for
> instance have an interface that can do a smarter job of putting together
> frames that could be routed.
>
> You could probably look at doing something like this for RXCSUM as well.
> The general idea is that if an upper device has it off then the value
> has to be off. For example if RXCSUM is off in a upper device and LRO is
> enabled on the lower device there is a good chance that the upper device
> will report checksum errors since most LRO implementations don't
> recalculate the checksum. If RXCSUM is forced down to the lower device
> hopefully its fix_features will know this and disable LRO on that device
> when the RXCSUM is disabled on it.

Yeah, I was thinking there might be more flags to treat the same way, 
just wanted to hammer out the plausibility of doing it at all first. I 
can add RXCSUM to v2, or just wait until there's something that people 
might consider merge-worthy before worrying about additional flags. From 
what I've seen, most device's fix_features are reasonably intelligent 
about allowing/disallowing certain flag combos, so this does look pretty 
safe at a glance, and if a specific device tips over, it probably needs 
to be fixed in the device's driver anyway.

-- 
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]

#1259768 — Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

On 10/30/2015 09:35 AM, Jarod Wilson wrote:
> Alexander Duyck wrote:
>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>> There are some netdev features that make little sense to toggle on and
>>> off in a stacked device setup on only one device in the stack. The prime
>>> example is a bonded connection, where it really doesn't make sense to
>>> disable LRO on the master, but not on any of the slaves, nor does it
>>> really make sense to be able to shut LRO off on a slave when its still
>>> enabled on the master.
>>>
>>> The strategy here is to add a section near the end of
>>> netdev_fix_features() that looks for upper and lower netdevs, then make
>>> sure certain feature flags match both up and down the stack. At present,
>>> only the LRO flag is included.
> ...
>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>> + struct net_device *lower, netdev_features_t features)
>>> +{
>>> + netdev_features_t want = features & lower->hw_features;
>>> +
>>> + if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
>>> + netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>> + lower->name);
>>> + upper->wanted_features &= ~NETIF_F_LRO;
>>> + lower->wanted_features &= ~NETIF_F_LRO;
>>> + netdev_update_features(lower);
>>> + if (unlikely(lower->features & NETIF_F_LRO))
>>> + netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>> + lower->name);
>>> + } else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
>>> + netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>> + lower->name);
>>> + upper->wanted_features |= NETIF_F_LRO;
>>> + lower->wanted_features |= NETIF_F_LRO;
>>> + netdev_update_features(lower);
>>> + if (unlikely(!(lower->features & NETIF_F_LRO)))
>>> + netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>> + lower->name);
>>> + }
>>> +}
>>> +
>>
>> Same thing here. If a lower dev has it disabled then leave it disabled.
>> I believe your goal is to make it so that dev_disable_lro() can shut
>> down LRO when it is making packets in the data-path unusable. There is
>> no need to make this an all or nothing scenario. We can let the stack
>> slam things down with dev_disable_lro() and then if a user so desires
>> they can come back through and enable LRO more selectively if they for
>> instance have an interface that can do a smarter job of putting together
>> frames that could be routed.
>>
>> You could probably look at doing something like this for RXCSUM as well.
>> The general idea is that if an upper device has it off then the value
>> has to be off. For example if RXCSUM is off in a upper device and LRO is
>> enabled on the lower device there is a good chance that the upper device
>> will report checksum errors since most LRO implementations don't
>> recalculate the checksum. If RXCSUM is forced down to the lower device
>> hopefully its fix_features will know this and disable LRO on that device
>> when the RXCSUM is disabled on it.
>
> Yeah, I was thinking there might be more flags to treat the same way,
> just wanted to hammer out the plausibility of doing it at all first. I
> can add RXCSUM to v2, or just wait until there's something that people
> might consider merge-worthy before worrying about additional flags. From
> what I've seen, most device's fix_features are reasonably intelligent
> about allowing/disallowing certain flag combos, so this does look pretty
> safe at a glance, and if a specific device tips over, it probably needs
> to be fixed in the device's driver anyway.

If nothing else you might start looking at working with a mask of bits 
that function like this.  You could probably start with GRO, LRO, and 
RXCSUM and work your way up from there.  If they aren't set on the upper 
devices you cannot enable them, and if they are cleared then they must 
be cleared on all lower devices.

- Alex
--
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web