[PATCH 0/6] bonding: only advertise LRO if underlying hardware can LRO

[PATCH 0/6] bonding: only advertise LRO if underlying hardware can LRO

[Jarod Wilson]

At present, you can create a bond, containing only underlying slaves that do
not support LRO, and the bond will happily claim to support LRO, and allow
LRO to be toggled on and off by ethtool. While things actually do function
fine in the scenario, and this is merely cosmetic, its a bit misleading to
users, and its something we can fix.

If we add NETIF_F_LRO to the NETIF_F_ONE_FOR_ALL flags in
netdev_features.h, then netdev_features_increment() will only enable LRO
if 1) its listed in the device's feature mask and 2) if there's actually a
slave present that supports the feature.

However, the bnx2x, ixgbe, netxen, qlcnic and s2io drivers all fail to report
support for LRO in their vlan_features, which requires some minor fixups to
these drivers to keep LRO working in cases where it should have been before
this set. The mellanox mlx5 and cavium liquidio drivers already properly set
the LRO flag in their vlan_features.

Note: I've only tested explicitly with bnx2x, as well as some non-LRO hw,
to confirm that:

1) if all slaves support LRO, the bond enables LRO
2) if some slaves support LRO, the bond enables LRO
3) if no slaves support LRO, the bond disables LRO

This set was generated against net-next master, it applies to 4.2.0-rc6 with
a bit of fuzz.

Jarod Wilson (6):
  net/bonding: enable LRO if one device supports it
  ethernet/bnx2x: advertise LRO support in vlan_features
  ethernet/ixgbe: advertise LRO support in vlan_features
  ethernet/netxen: advertise LRO support in vlan_features
  ethernet/qlcnic: advertise LRO support in vlan_features
  ethernet/s2io: advertise what hw supports in vlan_features

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c     | 3 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c        | 4 +++-
 drivers/net/ethernet/neterion/s2io.c                 | 1 +
 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c | 4 +++-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c     | 4 +++-
 include/linux/netdev_features.h                      | 3 ++-
 6 files changed, 14 insertions(+), 5 deletions(-)

CC: "David S. Miller" <davem@davemloft.net>
CC: Ariel Elior <ariel.elior@qlogic.com>
CC: Manish Chopra <manish.chopra@qlogic.com>
CC: Rajesh Borundia <rajesh.borundia@qlogic.com>
CC: Shahed Shaikh <shahed.shaikh@qlogic.com>
CC: Sony Chacko <sony.chacko@qlogic.com>
CC: Dept-GELinuxNICDev@qlogic.com
CC: Jiri Pirko <jiri@resnulli.us>
CC: Jon Mason <jdmason@kudzu.us>
CC: Scott Feldman <sfeldma@gmail.com>
CC: Tom Herbert <therbert@google.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
-- 
1.8.3.1

[PATCH 1/6] net/bonding: enable LRO if one device supports it

[Jarod Wilson]

Currently, all bonding devices come up, and claim to have LRO support,
which ethtool will let you toggle on and off, even if none of the
underlying hardware devices actually support it. While the bonding driver
takes precautions for slaves that don't support all features, this is at
least a little bit misleading to users.

If we add NETIF_F_LRO to the NETIF_F_ONE_FOR_ALL flags in
netdev_features.h, then netdev_features_increment() will only enable LRO
if 1) its listed in the device's feature mask and 2) if there's actually a
slave present that supports the feature.

Note that this is going to require some follow-up patches, as not all LRO
capable device drivers are currently properly reporting LRO support in
their vlan_features, which is where the bonding driver picks up
device-specific features.

CC: "David S. Miller" <davem@davemloft.net>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Tom Herbert <therbert@google.com>
CC: Scott Feldman <sfeldma@gmail.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 include/linux/netdev_features.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 9672781..6440bf1 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -159,7 +159,8 @@ enum {
  */
 #define NETIF_F_ONE_FOR_ALL	(NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
 				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
-				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
+				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED | \
+				 NETIF_F_LRO)
 
 /*
  * If one device doesn't support one of these features, then disable it
-- 
1.8.3.1

[PATCH 2/6] ethernet/bnx2x: advertise LRO support in vlan_features

[Jarod Wilson]

Without this, the presence of a bnx2x device in a bond will not trigger
LRO support to be enabled at the bond level, even while it is enabled on
the slave itself.

This change becomes necessary when NETIF_F_LRO is added to
netdev_features.h's NETIF_F_ONE_FOR_ALL.

CC: Ariel Elior <ariel.elior@qlogic.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index ad73a60..41dc066 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -13083,7 +13083,8 @@ static int bnx2x_init_dev(struct bnx2x *bp, struct pci_dev *pdev,
 	}
 
 	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA |
+		NETIF_F_LRO;
 
 	/* VF with OLD Hypervisor or old PF do not support filtering */
 	if (IS_PF(bp)) {
-- 
1.8.3.1

[PATCH 3/6] ethernet/ixgbe: advertise LRO support in vlan_features

[Jarod Wilson]

Without this, the presence of a ixgbe device in a bond will not trigger
LRO support to be enabled at the bond level, even while it is enabled on
the slave itself.

This change becomes necessary when NETIF_F_LRO is added to
netdev_features.h's NETIF_F_ONE_FOR_ALL.

CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 3e6a931..0a6e4e1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8659,8 +8659,10 @@ skip_sriov:
 
 	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
 		netdev->hw_features |= NETIF_F_LRO;
-	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
+	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
 		netdev->features |= NETIF_F_LRO;
+		netdev->vlan_features |= NETIF_F_LRO;
+	}
 
 	/* make sure the EEPROM is good */
 	if (hw->eeprom.ops.validate_checksum(hw, NULL) < 0) {
-- 
1.8.3.1

[PATCH 4/6] ethernet/netxen: advertise LRO support in vlan_features

[Jarod Wilson]

Without this, the presence of a netxen device in a bond will not trigger
LRO support to be enabled at the bond level, even while it is enabled on
the slave itself.

This change becomes necessary when NETIF_F_LRO is added to
netdev_features.h's NETIF_F_ONE_FOR_ALL.

CC: Manish Chopra <manish.chopra@qlogic.com>
CC: Sony Chacko <sony.chacko@qlogic.com>
CC: Rajesh Borundia <rajesh.borundia@qlogic.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 6409a06..0fd5ada54 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -1387,8 +1387,10 @@ netxen_setup_netdev(struct netxen_adapter *adapter,
 	if (adapter->capabilities & NX_FW_CAPABILITY_FVLANTX)
 		netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX;
 
-	if (adapter->capabilities & NX_FW_CAPABILITY_HW_LRO)
+	if (adapter->capabilities & NX_FW_CAPABILITY_HW_LRO) {
 		netdev->hw_features |= NETIF_F_LRO;
+		netdev->vlan_features |= NETIF_F_LRO;
+	}
 
 	netdev->features |= netdev->hw_features;
 
-- 
1.8.3.1

[PATCH 5/6] ethernet/qlcnic: advertise LRO support in vlan_features

[Jarod Wilson]

Without this, the presence of a qlcnic device in a bond will not trigger
LRO support to be enabled at the bond level, even while it is enabled on
the slave itself.

This change becomes necessary when NETIF_F_LRO is added to
netdev_features.h's NETIF_F_ONE_FOR_ALL.

CC: Shahed Shaikh <shahed.shaikh@qlogic.com>
CC: Dept-GELinuxNICDev@qlogic.com
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 8b08b20..5a798ab 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -2314,8 +2314,10 @@ qlcnic_setup_netdev(struct qlcnic_adapter *adapter, struct net_device *netdev,
 	if (qlcnic_sriov_vf_check(adapter))
 		netdev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 
-	if (adapter->ahw->capabilities & QLCNIC_FW_CAPABILITY_HW_LRO)
+	if (adapter->ahw->capabilities & QLCNIC_FW_CAPABILITY_HW_LRO) {
 		netdev->features |= NETIF_F_LRO;
+		netdev->vlan_features |= NETIF_F_LRO;
+	}
 
 	if (qlcnic_encap_tx_offload(adapter)) {
 		netdev->features |= NETIF_F_GSO_UDP_TUNNEL;
-- 
1.8.3.1

[PATCH 6/6] ethernet/s2io: advertise what hw supports in vlan_features

[Jarod Wilson]

For some reason, the s2io driver has never filled in vlan_features. If
that's fully intentional, then this patch should be dropped. If its not,
then this patch is necessary to maintain some functionality of slave s2io
devices in a bonding group.

Without this, the presence of a s2io device in a bond will not trigger
LRO support to be enabled at the bond level, even while it is enabled on
the slave itself.

This change becomes necessary when NETIF_F_LRO is added to
netdev_features.h's NETIF_F_ONE_FOR_ALL.

CC: Jon Mason <jdmason@kudzu.us>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/neterion/s2io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index 2d1b942..8bbf540 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c
@@ -7922,6 +7922,7 @@ s2io_init_nic(struct pci_dev *pdev, const struct pci_device_id *pre)
 		NETIF_F_RXCSUM | NETIF_F_LRO;
 	dev->features |= dev->hw_features |
 		NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
+	dev->vlan_features |= dev->hw_features;
 	if (sp->device_type & XFRAME_II_DEVICE) {
 		dev->hw_features |= NETIF_F_UFO;
 		if (ufo)
-- 
1.8.3.1

Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it

[Michal Kubecek]

On Thu, Aug 13, 2015 at 02:02:55PM -0400, Jarod Wilson wrote:
> Currently, all bonding devices come up, and claim to have LRO support,
> which ethtool will let you toggle on and off, even if none of the
> underlying hardware devices actually support it. While the bonding driver
> takes precautions for slaves that don't support all features, this is at
> least a little bit misleading to users.
> 
> If we add NETIF_F_LRO to the NETIF_F_ONE_FOR_ALL flags in
> netdev_features.h, then netdev_features_increment() will only enable LRO
> if 1) its listed in the device's feature mask and 2) if there's actually a
> slave present that supports the feature.
> 
> Note that this is going to require some follow-up patches, as not all LRO
> capable device drivers are currently properly reporting LRO support in
> their vlan_features, which is where the bonding driver picks up
> device-specific features.
> 
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Tom Herbert <therbert@google.com>
> CC: Scott Feldman <sfeldma@gmail.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  include/linux/netdev_features.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 9672781..6440bf1 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -159,7 +159,8 @@ enum {
>   */
>  #define NETIF_F_ONE_FOR_ALL	(NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
>  				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
> -				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
> +				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED | \
> +				 NETIF_F_LRO)
>  
>  /*
>   * If one device doesn't support one of these features, then disable it
> -- 

I don't think this is going to work the way you expect. Assume we have a
non-LRO eth1 and LRO capable eth2. If we enslave eth1 first, bond will
lose NETIF_F_LRO so that while enslaving eth2, bond_enslave() does run

	if (!(bond_dev->features & NETIF_F_LRO))
		dev_disable_lro(slave_dev);

and disable LRO on eth2 even before computing the bond features so that
in the end, all three interfaces end up with disabled LRO. If you add
the slaves in the opposite order, you end up with eth2 and bond having
LRO enabled. IMHO features should not depend on the order in which
slaves are added into the bond.

You would need to remove the code quoted above to make things work the
way you want (or move it after the call to bond_compute_features() which
is effectively the same). But then the result would be even worse:
adding a LRO-capable slave to a bond having dev_disable_lro() called on
it would not disable LRO on that slave, possibly (or rather likely)
causing communication breakage.

I believe NETIF_F_LRO in its original sense should be only considered
for physical devices; even if it's not explicitely said in the commit
message, the logic behind fbe168ba91f7 ("net: generic dev_disable_lro()
stacked device handling") is that for stacked devices like bond or team,
NETIF_F_LRO means "allow slaves to use LRO if they can and want" while
its absence means "disable LRO on all slaves". If you wanted NETIF_F_LRO
for a bond to mean "there is at least one LRO capable slave", you would
need a new flag for the "LRO should be disabled for all lower devices"
state. I don't think it's worth the effort.

                                                         Michal Kubecek

Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it

[Jarod Wilson]

On 2015-08-14 2:56 AM, Michal Kubecek wrote:
> On Thu, Aug 13, 2015 at 02:02:55PM -0400, Jarod Wilson wrote:
>> Currently, all bonding devices come up, and claim to have LRO support,
>> which ethtool will let you toggle on and off, even if none of the
>> underlying hardware devices actually support it. While the bonding driver
>> takes precautions for slaves that don't support all features, this is at
>> least a little bit misleading to users.
>>
>> If we add NETIF_F_LRO to the NETIF_F_ONE_FOR_ALL flags in
>> netdev_features.h, then netdev_features_increment() will only enable LRO
>> if 1) its listed in the device's feature mask and 2) if there's actually a
>> slave present that supports the feature.
>>
>> Note that this is going to require some follow-up patches, as not all LRO
>> capable device drivers are currently properly reporting LRO support in
>> their vlan_features, which is where the bonding driver picks up
>> device-specific features.
>>
>> CC: "David S. Miller" <davem@davemloft.net>
>> CC: Jiri Pirko <jiri@resnulli.us>
>> CC: Tom Herbert <therbert@google.com>
>> CC: Scott Feldman <sfeldma@gmail.com>
>> CC: netdev@vger.kernel.org
>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> ---
>>   include/linux/netdev_features.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>> index 9672781..6440bf1 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -159,7 +159,8 @@ enum {
>>    */
>>   #define NETIF_F_ONE_FOR_ALL	(NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
>>   				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
>> -				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
>> +				 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED | \
>> +				 NETIF_F_LRO)
>>
>>   /*
>>    * If one device doesn't support one of these features, then disable it
>> --
>
> I don't think this is going to work the way you expect. Assume we have a
> non-LRO eth1 and LRO capable eth2. If we enslave eth1 first, bond will
> lose NETIF_F_LRO so that while enslaving eth2, bond_enslave() does run
>
> 	if (!(bond_dev->features & NETIF_F_LRO))
> 		dev_disable_lro(slave_dev);
>
> and disable LRO on eth2 even before computing the bond features so that
> in the end, all three interfaces end up with disabled LRO. If you add
> the slaves in the opposite order, you end up with eth2 and bond having
> LRO enabled. IMHO features should not depend on the order in which
> slaves are added into the bond.

Crap, you're right. Hadn't tried inverting the order of added devices, 
as it didn't occur to me that it would make a difference.

> You would need to remove the code quoted above to make things work the
> way you want (or move it after the call to bond_compute_features() which
> is effectively the same). But then the result would be even worse:
> adding a LRO-capable slave to a bond having dev_disable_lro() called on
> it would not disable LRO on that slave, possibly (or rather likely)
> causing communication breakage.
>
> I believe NETIF_F_LRO in its original sense should be only considered
> for physical devices; even if it's not explicitely said in the commit
> message, the logic behind fbe168ba91f7 ("net: generic dev_disable_lro()
> stacked device handling") is that for stacked devices like bond or team,
> NETIF_F_LRO means "allow slaves to use LRO if they can and want" while
> its absence means "disable LRO on all slaves". If you wanted NETIF_F_LRO
> for a bond to mean "there is at least one LRO capable slave", you would
> need a new flag for the "LRO should be disabled for all lower devices"
> state. I don't think it's worth the effort.

Yeah, my thinking was that it should mean "there's at least one lro 
capable slave". If we just leave things the way they are though, I think 
its confusing on the user side -- it was one of our QE people who 
reported confusion being able to toggle lro on a bond when none of the 
slaves supported it. And there's also the inconsistency among devices 
that support lro in their vlan_features. So I think *something* should 
still be done here to make things clearer and more consistent, but I'll 
have to ponder that next week, since its beyond quitting time on Friday 
already. :)

Oh, last thought: the comment above #define NETIF_F_ONE_FOR_ALL is 
partly to blame for my not thinking harder and trying inverted ordering 
of slave additions:

/*
  * If one device supports one of these features, then enable them
  * for all in netdev_increment_features.
  */

This clearly seems to fall down in the lro case. :)

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it

[Jarod Wilson]

On 2015-08-14 7:41 PM, Jarod Wilson wrote:
> On 2015-08-14 2:56 AM, Michal Kubecek wrote:
>> On Thu, Aug 13, 2015 at 02:02:55PM -0400, Jarod Wilson wrote:
>>> Currently, all bonding devices come up, and claim to have LRO support,
>>> which ethtool will let you toggle on and off, even if none of the
>>> underlying hardware devices actually support it. While the bonding
>>> driver
>>> takes precautions for slaves that don't support all features, this is at
>>> least a little bit misleading to users.
>>>
>>> If we add NETIF_F_LRO to the NETIF_F_ONE_FOR_ALL flags in
>>> netdev_features.h, then netdev_features_increment() will only enable LRO
>>> if 1) its listed in the device's feature mask and 2) if there's
>>> actually a
>>> slave present that supports the feature.
>>>
>>> Note that this is going to require some follow-up patches, as not all
>>> LRO
>>> capable device drivers are currently properly reporting LRO support in
>>> their vlan_features, which is where the bonding driver picks up
>>> device-specific features.
...
>>> diff --git a/include/linux/netdev_features.h
>>> b/include/linux/netdev_features.h
>>> index 9672781..6440bf1 100644
>>> --- a/include/linux/netdev_features.h
>>> +++ b/include/linux/netdev_features.h
>>> @@ -159,7 +159,8 @@ enum {
>>>    */
>>>   #define NETIF_F_ONE_FOR_ALL    (NETIF_F_GSO_SOFTWARE |
>>> NETIF_F_GSO_ROBUST | \
>>>                    NETIF_F_SG | NETIF_F_HIGHDMA |        \
>>> -                 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
>>> +                 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED | \
>>> +                 NETIF_F_LRO)
>>>
>>>   /*
>>>    * If one device doesn't support one of these features, then
>>> disable it
>>> --
>>
>> I don't think this is going to work the way you expect. Assume we have a
>> non-LRO eth1 and LRO capable eth2. If we enslave eth1 first, bond will
>> lose NETIF_F_LRO so that while enslaving eth2, bond_enslave() does run
>>
>>     if (!(bond_dev->features & NETIF_F_LRO))
>>         dev_disable_lro(slave_dev);
>>
>> and disable LRO on eth2 even before computing the bond features so that
>> in the end, all three interfaces end up with disabled LRO. If you add
>> the slaves in the opposite order, you end up with eth2 and bond having
>> LRO enabled. IMHO features should not depend on the order in which
>> slaves are added into the bond.
>
> Crap, you're right. Hadn't tried inverting the order of added devices,
> as it didn't occur to me that it would make a difference.
>
>> You would need to remove the code quoted above to make things work the
>> way you want (or move it after the call to bond_compute_features() which
>> is effectively the same). But then the result would be even worse:
>> adding a LRO-capable slave to a bond having dev_disable_lro() called on
>> it would not disable LRO on that slave, possibly (or rather likely)
>> causing communication breakage.
>>
>> I believe NETIF_F_LRO in its original sense should be only considered
>> for physical devices; even if it's not explicitely said in the commit
>> message, the logic behind fbe168ba91f7 ("net: generic dev_disable_lro()
>> stacked device handling") is that for stacked devices like bond or team,
>> NETIF_F_LRO means "allow slaves to use LRO if they can and want" while
>> its absence means "disable LRO on all slaves". If you wanted NETIF_F_LRO
>> for a bond to mean "there is at least one LRO capable slave", you would
>> need a new flag for the "LRO should be disabled for all lower devices"
>> state. I don't think it's worth the effort.
>
> Yeah, my thinking was that it should mean "there's at least one lro
> capable slave". If we just leave things the way they are though, I think
> its confusing on the user side -- it was one of our QE people who
> reported confusion being able to toggle lro on a bond when none of the
> slaves supported it. And there's also the inconsistency among devices
> that support lro in their vlan_features. So I think *something* should
> still be done here to make things clearer and more consistent, but I'll
> have to ponder that next week, since its beyond quitting time on Friday
> already. :)
>
> Oh, last thought: the comment above #define NETIF_F_ONE_FOR_ALL is
> partly to blame for my not thinking harder and trying inverted ordering
> of slave additions:
>
> /*
>   * If one device supports one of these features, then enable them
>   * for all in netdev_increment_features.
>   */
>
> This clearly seems to fall down in the lro case. :)

Similarly, adding LRO to NETIF_F_ALL_FOR_ALL, even a bond with only 
LRO-capable hardware, I'm seeing the bond wind up without LRO and it 
can't be toggled on. I think the answer here (which to be fair, Nik 
suggested originally to me when I inherited this bug from him) is indeed 
to simply remove LRO from BOND_VLAN_FEATURES. Leave it fixed off for the 
bond itself, which should then turn it off for underlying devices via 
fbe168ba91f7 by default. If the user goes and turns it back on for the 
underlying devices individually, that's their prerogative. They keep 
both halves if it breaks.

Does that sound sane?

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it

[Michal Kubecek]

On Mon, Aug 17, 2015 at 05:07:06PM -0400, Jarod Wilson wrote:
> On 2015-08-14 7:41 PM, Jarod Wilson wrote:
> >
> >Yeah, my thinking was that it should mean "there's at least one lro
> >capable slave". If we just leave things the way they are though, I think
> >its confusing on the user side -- it was one of our QE people who
> >reported confusion being able to toggle lro on a bond when none of the
> >slaves supported it. And there's also the inconsistency among devices
> >that support lro in their vlan_features. So I think *something* should
> >still be done here to make things clearer and more consistent, but I'll
> >have to ponder that next week, since its beyond quitting time on Friday
> >already. :)
> >
> >Oh, last thought: the comment above #define NETIF_F_ONE_FOR_ALL is
> >partly to blame for my not thinking harder and trying inverted ordering
> >of slave additions:
> >
> >/*
> >  * If one device supports one of these features, then enable them
> >  * for all in netdev_increment_features.
> >  */
> >
> >This clearly seems to fall down in the lro case. :)
> 
> Similarly, adding LRO to NETIF_F_ALL_FOR_ALL, even a bond with only
> LRO-capable hardware, I'm seeing the bond wind up without LRO and it
> can't be toggled on.

I still believe the very idea of computing bond's NETIF_F_LRO from its
slaves - or, more generally, upper device from lower devices - is wrong.
This makes very good sense for e.g. TSO where lower device is processing
packets passed to it by the lower devices. Then it makes sense to
propagate the information whether the physical device on the bottom can
process those packets (OK, TSO is perhaps not the best example here as
we can always emulate it in software and still gain some performance).

However, with LRO, the situation is exactly the opposite: packets are
produced at the bottom device in the hierarchy and passed up; and
somewhere on the higher level we may hit a reason why LRO should be
disabled (bridged device, IPv6 forwarding enabled, device in a netns
with IPv4 forwarding enabled). This hierarchy may be quite complicated,
I've seen things like a bond with a vlan on top of it and a macvlan on
top of the vlan which was then moved into a different netns (LXC
container). LRO related issues in setups like this were the reason for
529d04895446 and eventually fbe168ba91f7.

I agree that current state is not perfect but I don't think creating an
artificial value of NETIF_F_LRO composed of slave values for a bond (and
probably also a team, for consistency) is going to help. I would see
more sense in rethinking the current concept of deriving all upper
device's features from its lower devices and completing the two classes
we already have:

  - NETIF_F_ONE_FOR_ALL: propagated up, OR-ed
  - NETIF_F_ALL_FOR_ALL: propagated up, AND-ed

by two new classes:

  - propagated down, OR-ed
  - propagated down, AND-ed

NETIF_F_LRO should IMHO fall into the last class. I don't have an
example of a flag belonging to third class from the top of my head
but there may also be one (GRO, perhaps?). Such change would, of course,
require careful planning and testing to avoid regressions.

> I think the answer here (which to be fair, Nik suggested originally to
> me when I inherited this bug from him) is indeed to simply remove LRO
> from BOND_VLAN_FEATURES. Leave it fixed off for the bond itself, which
> should then turn it off for underlying devices via fbe168ba91f7 by
> default. If the user goes and turns it back on for the underlying
> devices individually, that's their prerogative. They keep both halves
> if it breaks.
> 
> Does that sound sane?

If I understand the code correctly, this would mean bond would have
NETIF_F_LRO always disabled in its dev->features so that any slave would
end up with disabled LRO after bond_enslave(). While this would be less
of a problem than LRO enabled when it shouldn't, it would cause a
performance penalty in the cases when there is no reason to disable LRO
(end hosts without any virtualization or other bridging or forwarding).

So the distribution init scripts (or management daemons like wicked or
systemd-networkd) would have to handle the LRO reenabling somehow. And
they would have trouble checking whether to do reenable LRO or not as
bond would stop keeping the state information whether dev_disable_lro()
was called for it or not (we must not reenable LRO if it was).

                                                         Michal Kubecek

RE: [Intel-wired-lan] [PATCH 3/6] ethernet/ixgbe: advertise LRO support in vlan_features

[Singh, Krishneil K]

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Jarod Wilson
Sent: Thursday, August 13, 2015 11:03 AM
To: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Jarod Wilson <jarod@redhat.com>
Subject: [Intel-wired-lan] [PATCH 3/6] ethernet/ixgbe: advertise LRO support in vlan_features

Without this, the presence of a ixgbe device in a bond will not trigger LRO support to be enabled at the bond level, even while it is enabled on the slave itself.

This change becomes necessary when NETIF_F_LRO is added to netdev_features.h's NETIF_F_ONE_FOR_ALL.

CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 3e6a931..0a6e4e1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8659,8 +8659,10 @@ skip_sriov:
 
 	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
 		netdev->hw_features |= NETIF_F_LRO;
-	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
+	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
 		netdev->features |= NETIF_F_LRO;
+		netdev->vlan_features |= NETIF_F_LRO;
+	}
 
 	/* make sure the EEPROM is good */
 	if (hw->eeprom.ops.validate_checksum(hw, NULL) < 0) {
--
1.8.3.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

While Validating this patch we have run in to a call trace if we have forwarding  (net.ipv4.ip_forward = 1) and  LRO enabled on interface prior to creating VLAN interface. With the patch reverted we don't see this failure. 

Validation setup:

sysctl net.ipv4.ip_forward=1
ethtool -K ethX lro on
ip link set ethX up
ip link add link ethX name ethX.10 type vlan id 10.

CALL TRACE:

[582992.985245] ixgbe 0000:83:00.0 eth6: NIC Link is Up 10 Gbps, Flow Control: RX/TX
[582992.985400] IPv6: ADDRCONF(NETDEV_CHANGE): eth6: link becomes ready
[582995.764828] ixgbe 0000:83:00.1 eth7: NIC Link is Up 10 Gbps, Flow Control: RX/TX
[582995.764964] IPv6: ADDRCONF(NETDEV_CHANGE): eth7: link becomes ready
[583027.588991] ixgbe 0000:04:00.0 eth2: NIC Link is Up 10 Gbps, Flow Control: RX/TX
[583044.365523] ------------[ cut here ]------------
[583044.366181] WARNING: CPU: 20 PID: 56879 at net/core/dev.c:1472 dev_disable_lro+0x95/0xa0()
[583044.366711] netdevice: eth2.10
failed to disable LRO!
[583044.367876] Modules linked in: ixgbe ixgb igb e100 mii e1000 e1000e 8021q garp mrp tcp_lp bnep bluetooth rfkill fuse btrfs xor raid6_pq vfat msdos fat ext4 mbcache jbd2 binfmt_misc xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 iptable_filter ip_tables tun bridge stp llc x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul crc32c_intel nfsd ghash_clmulni_intel mei_me aesni_intel mei lrw auth_rpcgss gf128mul shpchp iTCO_wdt ioatdma iTCO_vendor_support glue_helper nfs_acl ablk_helper lockd cryptd i2c_i801 lpc_ich mfd_core ipmi_si sb_edac edac_core grace dm_mirror dm_region_hash ipmi_msghandler pcspkr wmi dm_log dm_mod 
 sunrpc uinput
[583044.371142]  xfs libcrc32c sr_mod cdrom sd_mod mgag200 syscopyarea sysfillrect sysimgblt drm_kms_helper ttm drm ahci libahci libata mdio vxlan firewire_ohci ip6_udp_tunnel firewire_core udp_tunnel ptp i2c_algo_bit pps_core i2c_core crc_itu_t dca [last unloaded: ixgbe]
[583044.372915] CPU: 20 PID: 56879 Comm: ip Tainted: G        W IOE   4.2.0-rc7-Ustream-8-26-15+ #1
[583044.373511] Hardware name: Intel Corporation S2600CO/S2600CO, BIOS SE5C600.86B.01.06.0001.090720121056 09/07/2012
[583044.374126]  0000000000000000 00000000e9a2d4dc ffff8803ce16b5b8 ffffffff8166b4e9
[583044.374752]  0000000000000000 ffff8803ce16b610 ffff8803ce16b5f8 ffffffff8107b06a
[583044.375380]  ffff8803ce16b608 ffff880428041000 ffffffff818dc1eb 0000000000000005
[583044.376033] Call Trace:
[583044.376662]  [<ffffffff8166b4e9>] dump_stack+0x45/0x57
[583044.377290]  [<ffffffff8107b06a>] warn_slowpath_common+0x8a/0xc0
[583044.377950]  [<ffffffff8107b0f5>] warn_slowpath_fmt+0x55/0x70
[583044.378570]  [<ffffffff81561075>] ? netdev_update_features+0x25/0x60
[583044.379218]  [<ffffffff81561145>] dev_disable_lro+0x95/0xa0
[583044.379841]  [<ffffffff815d28dd>] inetdev_init+0x17d/0x230
[583044.380458]  [<ffffffff815d40ef>] inetdev_event+0x37f/0x4f0
[583044.381079]  [<ffffffff8109a3bd>] notifier_call_chain+0x4d/0x80
[583044.381697]  [<ffffffff8109a4e6>] raw_notifier_call_chain+0x16/0x20
[583044.382343]  [<ffffffff81557859>] call_netdevice_notifiers_info+0x39/0x70
[583044.382971]  [<ffffffff8156145e>] register_netdevice+0x2ae/0x430
[583044.383595]  [<ffffffff81557664>] ? dev_get_nest_level+0x64/0xa0
[583044.384226]  [<ffffffffa020fafa>] register_vlan_dev+0xda/0x240 [8021q]
[583044.384884]  [<ffffffffa021141c>] vlan_newlink+0xbc/0xf0 [8021q]
[583044.385516]  [<ffffffff8156feba>] rtnl_newlink+0x6ca/0x880
[583044.386149]  [<ffffffff8156f94c>] ? rtnl_newlink+0x15c/0x880
[583044.386794]  [<ffffffff8156d0e4>] rtnetlink_rcv_msg+0xa4/0x250
[583044.387421]  [<ffffffff8154995d>] ? __alloc_skb+0x8d/0x2b0
[583044.388087]  [<ffffffff8156d040>] ? rtnetlink_rcv+0x40/0x40
[583044.388718]  [<ffffffff815904df>] netlink_rcv_skb+0xaf/0xc0
[583044.389345]  [<ffffffff8156d02c>] rtnetlink_rcv+0x2c/0x40
[583044.389969]  [<ffffffff8158fbd1>] netlink_unicast+0x101/0x1f0
[583044.390584]  [<ffffffff815900c1>] netlink_sendmsg+0x401/0x660
[583044.391212]  [<ffffffff81541138>] sock_sendmsg+0x38/0x50
[583044.391815]  [<ffffffff81541a95>] ___sys_sendmsg+0x275/0x290
[583044.392407]  [<ffffffff8118862b>] ? lru_cache_add_active_or_unevictable+0x2b/0xa0
[583044.392995]  [<ffffffff811a9d8c>] ? handle_mm_fault+0xc0c/0x18d0
[583044.393572]  [<ffffffff81542457>] __sys_sendmsg+0x57/0xa0
[583044.394170]  [<ffffffff815424b2>] SyS_sendmsg+0x12/0x20
[583044.394731]  [<ffffffff8167222e>] entry_SYSCALL_64_fastpath+0x12/0x71
[583044.395290] ---[ end trace 215d0e95d43c1319 ]---