[PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[Michal Kubecek]

Large receive offloading is known to cause problems if received packets
are passed to other host. Therefore the kernel disables it by calling
dev_disable_lro() whenever a network device is enslaved in a bridge or
forwarding is enabled for it (or globally). For virtual devices we need
to disable LRO on the underlying physical device (which is actually
receiving the packets).

Current dev_disable_lro() code handles this propagation for a vlan
(including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
This patch adds LRO disabling propagation for

  - macvlan on top of a vlan or any stacked combination of those
  - bonding
  - teaming

In the bonding and teaming case, it is necessary to disable LRO not only
on slaves when dev_disable_lro() is called but also on any slave (port)
added later.

Michal Kubecek (3):
  net: handle more general stacking in dev_disable_lro()
  team: add helper to check if device is a team master
  net: propagate LRO disabling to bond and team slaves

 drivers/net/bonding/bond_main.c |  3 +++
 drivers/net/team/team.c         |  6 +++++-
 include/linux/netdevice.h       |  7 +++++++
 net/core/dev.c                  | 31 ++++++++++++++++++++++---------
 4 files changed, 37 insertions(+), 10 deletions(-)

-- 
1.8.4.5

[PATCH net-next 1/3] net: handle more general stacking in dev_disable_lro()

[Michal Kubecek]

Current dev_disable_lro() code passing LRO disabling to lower device
handles vlan on top of a macvlan but not the opposite. Repeat the test
until the device is neither vlan nor macvlan.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 net/core/dev.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index bb09b03..ebcd308 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1437,16 +1437,21 @@ EXPORT_SYMBOL(dev_close);
  */
 void dev_disable_lro(struct net_device *dev)
 {
-	/*
-	 * If we're trying to disable lro on a vlan device
-	 * use the underlying physical device instead
-	 */
-	if (is_vlan_dev(dev))
-		dev = vlan_dev_real_dev(dev);
+	struct net_device *prev_dev;
+
+	do {
+		prev_dev = dev;
+
+		/* If we're trying to disable lro on a vlan device
+		 * use the underlying physical device instead
+		 */
+		if (is_vlan_dev(dev))
+			dev = vlan_dev_real_dev(dev);
 
-	/* the same for macvlan devices */
-	if (netif_is_macvlan(dev))
-		dev = macvlan_dev_real_dev(dev);
+		/* the same for macvlan devices */
+		if (netif_is_macvlan(dev))
+			dev = macvlan_dev_real_dev(dev);
+	} while (dev != prev_dev);
 
 	dev->wanted_features &= ~NETIF_F_LRO;
 	netdev_update_features(dev);
-- 
1.8.4.5

[PATCH net-next 2/3] team: add helper to check if device is a team master

[Michal Kubecek]

Add flag IFF_TEAM_MASTER to recognize a team master and helper
netif_is_team_master() to check it.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/team/team.c   | 3 ++-
 include/linux/netdevice.h | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 2368395..e1e2b85 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2043,7 +2043,8 @@ static void team_setup(struct net_device *dev)
 	 * bring us to promisc mode in case a unicast addr is added.
 	 * Let this up to underlay drivers.
 	 */
-	dev->priv_flags |= IFF_UNICAST_FLT | IFF_LIVE_ADDR_CHANGE;
+	dev->priv_flags |= IFF_UNICAST_FLT | IFF_LIVE_ADDR_CHANGE |
+			   IFF_TEAM_MASTER;
 
 	dev->features |= NETIF_F_LLTX;
 	dev->features |= NETIF_F_GRO;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 888d551..1236feb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1203,6 +1203,7 @@ struct net_device_ops {
  * @IFF_LIVE_ADDR_CHANGE: device supports hardware address
  *	change when it's running
  * @IFF_MACVLAN: Macvlan device
+ * @IFF_TEAM_MASTER: device is a team master
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1228,6 +1229,7 @@ enum netdev_priv_flags {
 	IFF_LIVE_ADDR_CHANGE		= 1<<20,
 	IFF_MACVLAN			= 1<<21,
 	IFF_XMIT_DST_RELEASE_PERM	= 1<<22,
+	IFF_TEAM_MASTER			= 1<<23,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -3638,6 +3640,11 @@ static inline bool netif_is_bond_slave(struct net_device *dev)
 	return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_BONDING;
 }
 
+static inline bool netif_is_team_master(struct net_device *dev)
+{
+	return dev->priv_flags & IFF_TEAM_MASTER;
+}
+
 static inline bool netif_supports_nofcs(struct net_device *dev)
 {
 	return dev->priv_flags & IFF_SUPP_NOFCS;
-- 
1.8.4.5

[PATCH net-next 3/3] net: propagate LRO disabling to bond and team slaves

[Michal Kubecek]

As LRO doesn't work correctly if incoming packets are passed to other
hosts, it needs to be disabled if a device is enslaved into a bridge or
forwarding is enabled for it.

For a bond/team master, LRO needs to be disabled for all its slaves as
they are actually receiving the packets. Once LRO is disabled for
a bond/team master, we also need to make sure to disable it for newly
added slaves.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/bonding/bond_main.c | 3 +++
 drivers/net/team/team.c         | 3 +++
 net/core/dev.c                  | 8 ++++++++
 3 files changed, 14 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9b3456..8575fee 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1526,6 +1526,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	}
 #endif
 
+	if (!(bond_dev->features & NETIF_F_LRO))
+		dev_disable_lro(slave_dev);
+
 	res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
 					 new_slave);
 	if (res) {
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index e1e2b85..217b973 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1179,6 +1179,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 		goto err_enable_netpoll;
 	}
 
+	if (!(dev->features & NETIF_F_LRO))
+		dev_disable_lro(port_dev);
+
 	err = netdev_rx_handler_register(port_dev, team_handle_frame,
 					 port);
 	if (err) {
diff --git a/net/core/dev.c b/net/core/dev.c
index ebcd308..b132f44 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1438,6 +1438,8 @@ EXPORT_SYMBOL(dev_close);
 void dev_disable_lro(struct net_device *dev)
 {
 	struct net_device *prev_dev;
+	struct net_device *lower_dev;
+	struct list_head *iter;
 
 	do {
 		prev_dev = dev;
@@ -1458,6 +1460,12 @@ void dev_disable_lro(struct net_device *dev)
 
 	if (unlikely(dev->features & NETIF_F_LRO))
 		netdev_WARN(dev, "failed to disable LRO!\n");
+
+	/* if dev is a bond/team master, disable LRO for all its slaves */
+	if (netif_is_bond_master(dev) || netif_is_team_master(dev)) {
+		netdev_for_each_lower_dev(dev, lower_dev, iter)
+			dev_disable_lro(lower_dev);
+	}
 }
 EXPORT_SYMBOL(dev_disable_lro);
 
-- 
1.8.4.5

Re: [PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[Veaceslav Falico]

On Tue, Nov 11, 2014 at 09:21:30AM +0100, Michal Kubecek wrote:
>Large receive offloading is known to cause problems if received packets
>are passed to other host. Therefore the kernel disables it by calling
>dev_disable_lro() whenever a network device is enslaved in a bridge or
>forwarding is enabled for it (or globally). For virtual devices we need
>to disable LRO on the underlying physical device (which is actually
>receiving the packets).
>
>Current dev_disable_lro() code handles this propagation for a vlan
>(including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
>This patch adds LRO disabling propagation for
>
>  - macvlan on top of a vlan or any stacked combination of those
>  - bonding
>  - teaming

All of these drivers use the netdev_upper and friends, so why not make it
generic with netdev_for_each_all_lower() in dev_disable_lro()?

>
>In the bonding and teaming case, it is necessary to disable LRO not only
>on slaves when dev_disable_lro() is called but also on any slave (port)
>added later.
>
>Michal Kubecek (3):
>  net: handle more general stacking in dev_disable_lro()
>  team: add helper to check if device is a team master
>  net: propagate LRO disabling to bond and team slaves
>
> drivers/net/bonding/bond_main.c |  3 +++
> drivers/net/team/team.c         |  6 +++++-
> include/linux/netdevice.h       |  7 +++++++
> net/core/dev.c                  | 31 ++++++++++++++++++++++---------
> 4 files changed, 37 insertions(+), 10 deletions(-)
>
>-- 
>1.8.4.5
>

Re: [PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[Michal Kubecek]

On Tue, Nov 11, 2014 at 10:05:22AM +0100, Veaceslav Falico wrote:
> On Tue, Nov 11, 2014 at 09:21:30AM +0100, Michal Kubecek wrote:
> >Large receive offloading is known to cause problems if received packets
> >are passed to other host. Therefore the kernel disables it by calling
> >dev_disable_lro() whenever a network device is enslaved in a bridge or
> >forwarding is enabled for it (or globally). For virtual devices we need
> >to disable LRO on the underlying physical device (which is actually
> >receiving the packets).
> >
> >Current dev_disable_lro() code handles this propagation for a vlan
> >(including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
> >This patch adds LRO disabling propagation for
> >
> > - macvlan on top of a vlan or any stacked combination of those
> > - bonding
> > - teaming
> 
> All of these drivers use the netdev_upper and friends, so why not make it
> generic with netdev_for_each_all_lower() in dev_disable_lro()?

I wanted to preserve current approach where for vlan and macvlan, LRO is
disabled on the real device instead of the original one (rather than in
addition to it) as LRO is always disabled on them.

Handling all four uniformly would make the code nicer but would bring
unnecessary overhead of traversing the list and dev_disable_lro()
recursion. On the other hand, this operation is not time critical so it
might be acceptable after all.

                                                         Michal Kubecek

Re: [PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Tue, 11 Nov 2014 10:34:57 +0100

> On Tue, Nov 11, 2014 at 10:05:22AM +0100, Veaceslav Falico wrote:
>> On Tue, Nov 11, 2014 at 09:21:30AM +0100, Michal Kubecek wrote:
>> >Large receive offloading is known to cause problems if received packets
>> >are passed to other host. Therefore the kernel disables it by calling
>> >dev_disable_lro() whenever a network device is enslaved in a bridge or
>> >forwarding is enabled for it (or globally). For virtual devices we need
>> >to disable LRO on the underlying physical device (which is actually
>> >receiving the packets).
>> >
>> >Current dev_disable_lro() code handles this propagation for a vlan
>> >(including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
>> >This patch adds LRO disabling propagation for
>> >
>> > - macvlan on top of a vlan or any stacked combination of those
>> > - bonding
>> > - teaming
>> 
>> All of these drivers use the netdev_upper and friends, so why not make it
>> generic with netdev_for_each_all_lower() in dev_disable_lro()?
> 
> I wanted to preserve current approach where for vlan and macvlan, LRO is
> disabled on the real device instead of the original one (rather than in
> addition to it) as LRO is always disabled on them.
> 
> Handling all four uniformly would make the code nicer but would bring
> unnecessary overhead of traversing the list and dev_disable_lro()
> recursion. On the other hand, this operation is not time critical so it
> might be acceptable after all.

Please do it generically.

Having a special stanza for each layered device type in
dev_disable_lro() is beyond stupid.  Especially when it
can in fact be done cleanly.

THanks.

Re: [PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[Michal Kubecek]

On Tue, Nov 11, 2014 at 09:47:52PM -0500, David Miller wrote:
> 
> Please do it generically.
> 
> Having a special stanza for each layered device type in
> dev_disable_lro() is beyond stupid.  Especially when it
> can in fact be done cleanly.

I gave it some thought and I would like ask one question first:

Does the upper-lower relationship always mean that upper device receives
packets through its lower device(s) so that LRO should always be
disabled for lower devices whenever there are some? Or should it be
limited only to an explicit list of device types (vlan, macvlan, bond,
team)?

                                                         Michal Kubecek

Re: [PATCH net-next 0/3] dev_disable_lro() improvements for stacked devices

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Wed, 12 Nov 2014 14:15:46 +0100

> On Tue, Nov 11, 2014 at 09:47:52PM -0500, David Miller wrote:
>> 
>> Please do it generically.
>> 
>> Having a special stanza for each layered device type in
>> dev_disable_lro() is beyond stupid.  Especially when it
>> can in fact be done cleanly.
> 
> I gave it some thought and I would like ask one question first:
> 
> Does the upper-lower relationship always mean that upper device receives
> packets through its lower device(s) so that LRO should always be
> disabled for lower devices whenever there are some? Or should it be
> limited only to an explicit list of device types (vlan, macvlan, bond,
> team)?

This should be the case, anyone else?

[PATCH net-next v2] net: generic dev_disable_lro() stacked device handling

[Michal Kubecek]

Large receive offloading is known to cause problems if received packets
are passed to other host. Therefore the kernel disables it by calling
dev_disable_lro() whenever a network device is enslaved in a bridge or
forwarding is enabled for it (or globally). For virtual devices we need
to disable LRO on the underlying physical device (which is actually
receiving the packets).

Current dev_disable_lro() code handles this  propagation for a vlan
(including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
It doesn't handle other stacked devices and their combinations, in
particular propagation from a bond to its slaves which often causes
problems in virtualization setups.

As we now have generic data structures describing the upper-lower device
relationship, dev_disable_lro() can be generalized to disable LRO also
for all lower devices (if any) once it is disabled for the device
itself.

For bonding and teaming devices, it is necessary to disable LRO not only
on current slaves at the moment when dev_disable_lro() is called but
also on any slave (port) added later.

v2: use lower device links for all devices (including vlan and macvlan)

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/bonding/bond_main.c |  3 +++
 drivers/net/team/team.c         |  3 +++
 net/core/dev.c                  | 15 +++++----------
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9b3456..8575fee 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1526,6 +1526,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	}
 #endif
 
+	if (!(bond_dev->features & NETIF_F_LRO))
+		dev_disable_lro(slave_dev);
+
 	res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
 					 new_slave);
 	if (res) {
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 2368395..93e2242 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1179,6 +1179,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 		goto err_enable_netpoll;
 	}
 
+	if (!(dev->features & NETIF_F_LRO))
+		dev_disable_lro(port_dev);
+
 	err = netdev_rx_handler_register(port_dev, team_handle_frame,
 					 port);
 	if (err) {
diff --git a/net/core/dev.c b/net/core/dev.c
index bb09b03..1ab168e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1437,22 +1437,17 @@ EXPORT_SYMBOL(dev_close);
  */
 void dev_disable_lro(struct net_device *dev)
 {
-	/*
-	 * If we're trying to disable lro on a vlan device
-	 * use the underlying physical device instead
-	 */
-	if (is_vlan_dev(dev))
-		dev = vlan_dev_real_dev(dev);
-
-	/* the same for macvlan devices */
-	if (netif_is_macvlan(dev))
-		dev = macvlan_dev_real_dev(dev);
+	struct net_device *lower_dev;
+	struct list_head *iter;
 
 	dev->wanted_features &= ~NETIF_F_LRO;
 	netdev_update_features(dev);
 
 	if (unlikely(dev->features & NETIF_F_LRO))
 		netdev_WARN(dev, "failed to disable LRO!\n");
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter)
+		dev_disable_lro(lower_dev);
 }
 EXPORT_SYMBOL(dev_disable_lro);
 
-- 
1.8.4.5

Re: [PATCH net-next v2] net: generic dev_disable_lro() stacked device handling

[Veaceslav Falico]

On Thu, Nov 13, 2014 at 07:54:50AM +0100, Michal Kubecek wrote:
>Large receive offloading is known to cause problems if received packets
>are passed to other host. Therefore the kernel disables it by calling
>dev_disable_lro() whenever a network device is enslaved in a bridge or
>forwarding is enabled for it (or globally). For virtual devices we need
>to disable LRO on the underlying physical device (which is actually
>receiving the packets).
>
>Current dev_disable_lro() code handles this  propagation for a vlan
>(including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
>It doesn't handle other stacked devices and their combinations, in
>particular propagation from a bond to its slaves which often causes
>problems in virtualization setups.
>
>As we now have generic data structures describing the upper-lower device
>relationship, dev_disable_lro() can be generalized to disable LRO also
>for all lower devices (if any) once it is disabled for the device
>itself.
>
>For bonding and teaming devices, it is necessary to disable LRO not only
>on current slaves at the moment when dev_disable_lro() is called but
>also on any slave (port) added later.
>
>v2: use lower device links for all devices (including vlan and macvlan)
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>

Seems ok to me.

Acked-by: Veaceslav Falico <vfalico@gmail.com>

>---
> drivers/net/bonding/bond_main.c |  3 +++
> drivers/net/team/team.c         |  3 +++
> net/core/dev.c                  | 15 +++++----------
> 3 files changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index b9b3456..8575fee 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1526,6 +1526,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 	}
> #endif
>
>+	if (!(bond_dev->features & NETIF_F_LRO))
>+		dev_disable_lro(slave_dev);
>+
> 	res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
> 					 new_slave);
> 	if (res) {
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index 2368395..93e2242 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -1179,6 +1179,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
> 		goto err_enable_netpoll;
> 	}
>
>+	if (!(dev->features & NETIF_F_LRO))
>+		dev_disable_lro(port_dev);
>+
> 	err = netdev_rx_handler_register(port_dev, team_handle_frame,
> 					 port);
> 	if (err) {
>diff --git a/net/core/dev.c b/net/core/dev.c
>index bb09b03..1ab168e 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -1437,22 +1437,17 @@ EXPORT_SYMBOL(dev_close);
>  */
> void dev_disable_lro(struct net_device *dev)
> {
>-	/*
>-	 * If we're trying to disable lro on a vlan device
>-	 * use the underlying physical device instead
>-	 */
>-	if (is_vlan_dev(dev))
>-		dev = vlan_dev_real_dev(dev);
>-
>-	/* the same for macvlan devices */
>-	if (netif_is_macvlan(dev))
>-		dev = macvlan_dev_real_dev(dev);
>+	struct net_device *lower_dev;
>+	struct list_head *iter;
>
> 	dev->wanted_features &= ~NETIF_F_LRO;
> 	netdev_update_features(dev);
>
> 	if (unlikely(dev->features & NETIF_F_LRO))
> 		netdev_WARN(dev, "failed to disable LRO!\n");
>+
>+	netdev_for_each_lower_dev(dev, lower_dev, iter)
>+		dev_disable_lro(lower_dev);
> }
> EXPORT_SYMBOL(dev_disable_lro);
>
>-- 
>1.8.4.5
>

Re: [PATCH net-next v2] net: generic dev_disable_lro() stacked device handling

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Thu, 13 Nov 2014 07:54:50 +0100 (CET)

> Large receive offloading is known to cause problems if received packets
> are passed to other host. Therefore the kernel disables it by calling
> dev_disable_lro() whenever a network device is enslaved in a bridge or
> forwarding is enabled for it (or globally). For virtual devices we need
> to disable LRO on the underlying physical device (which is actually
> receiving the packets).
> 
> Current dev_disable_lro() code handles this  propagation for a vlan
> (including 802.1ad nested vlan), macvlan or a vlan on top of a macvlan.
> It doesn't handle other stacked devices and their combinations, in
> particular propagation from a bond to its slaves which often causes
> problems in virtualization setups.
> 
> As we now have generic data structures describing the upper-lower device
> relationship, dev_disable_lro() can be generalized to disable LRO also
> for all lower devices (if any) once it is disabled for the device
> itself.
> 
> For bonding and teaming devices, it is necessary to disable LRO not only
> on current slaves at the moment when dev_disable_lro() is called but
> also on any slave (port) added later.
> 
> v2: use lower device links for all devices (including vlan and macvlan)
> 
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>

Applied, thanks a lot.