| Message ID | 1456269234-5731-1-git-send-email-dan.streetman@canonical.com |
|---|---|
| State | New |
| Headers | show |
I guess its not going to happen as normal stable since David Miller is no longer looking at those versions. Patch seems reasonable. Just a bit more background info somewhere (bug report) and hints about any testing done would be nice. -Stefan On 24.02.2016 00:13, Dan Streetman wrote: > BugLink: http://bugs.launchpad.net/bugs/1547680 > > 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> > Acked-by: Veaceslav Falico <vfalico@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from fbe168ba91f7c327856f205699404284c2f09e36 upstream) > Signed-off-by: Dan Streetman <dan.streetman@canonical.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 d0f58e4..6eb47aa 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1584,6 +1584,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 f30a88b..03bac60 100644 > --- a/drivers/net/team/team.c > +++ b/drivers/net/team/team.c > @@ -1180,6 +1180,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev) > goto err_set_upper_link; > } > > + 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 31f1f68..e758293 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -1423,22 +1423,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); > >
On Wed, Feb 24, 2016 at 11:23 AM, Stefan Bader <stefan.bader@canonical.com> wrote: > I guess its not going to happen as normal stable since David Miller is no longer > looking at those versions. Patch seems reasonable. Just a bit more background > info somewhere (bug report) and hints about any testing done would be nice. I added some more detail to the bug; thanks! > > -Stefan > > > On 24.02.2016 00:13, Dan Streetman wrote: >> BugLink: http://bugs.launchpad.net/bugs/1547680 >> >> 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> >> Acked-by: Veaceslav Falico <vfalico@gmail.com> >> Signed-off-by: David S. Miller <davem@davemloft.net> >> (cherry-picked from fbe168ba91f7c327856f205699404284c2f09e36 upstream) >> Signed-off-by: Dan Streetman <dan.streetman@canonical.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 d0f58e4..6eb47aa 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1584,6 +1584,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 f30a88b..03bac60 100644 >> --- a/drivers/net/team/team.c >> +++ b/drivers/net/team/team.c >> @@ -1180,6 +1180,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev) >> goto err_set_upper_link; >> } >> >> + 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 31f1f68..e758293 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -1423,22 +1423,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); >> >> > >
On Tue, Feb 23, 2016 at 06:13:54PM -0500, Dan Streetman wrote: > BugLink: http://bugs.launchpad.net/bugs/1547680 > > 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> > Acked-by: Veaceslav Falico <vfalico@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry-picked from fbe168ba91f7c327856f205699404284c2f09e36 upstream) > Signed-off-by: Dan Streetman <dan.streetman@canonical.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 d0f58e4..6eb47aa 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1584,6 +1584,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 f30a88b..03bac60 100644 > --- a/drivers/net/team/team.c > +++ b/drivers/net/team/team.c > @@ -1180,6 +1180,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev) > goto err_set_upper_link; > } > > + 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 31f1f68..e758293 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -1423,22 +1423,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); Looks to use interfaces which at least existed at the time. The code change looks reasonable to the naked eye. The bug shows some testing and the bug itself should be easily verifiable. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
Applied [Trusty/Utopic]: commit 4afcc3fefddfeaf391e80343f9290bc775652d4f Author: Michal Kubeček <mkubecek@suse.cz> Date: Thu Nov 13 07:54:50 2014 +0100 net: generic dev_disable_lro() stacked device handling
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index d0f58e4..6eb47aa 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1584,6 +1584,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 f30a88b..03bac60 100644 --- a/drivers/net/team/team.c +++ b/drivers/net/team/team.c @@ -1180,6 +1180,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev) goto err_set_upper_link; } + 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 31f1f68..e758293 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1423,22 +1423,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);