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
ja...@redhat.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
