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

[Jarod Wilson <jarod@redhat.com>]

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