Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1207039 > unrolled thread
| Started by | Jarod Wilson <jarod@redhat.com> |
|---|---|
| First post | 2015-08-13 20:10 +0200 |
| Last post | 2015-08-18 09:50 +0200 |
| Articles | 6 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 0/6] bonding: only advertise LRO if underlying hardware can LRO Jarod Wilson <jarod@redhat.com> - 2015-08-13 20:10 +0200
[PATCH 1/6] net/bonding: enable LRO if one device supports it Jarod Wilson <jarod@redhat.com> - 2015-08-13 20:10 +0200
Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it Michal Kubecek <mkubecek@suse.cz> - 2015-08-14 09:00 +0200
Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it Jarod Wilson <jarod@redhat.com> - 2015-08-15 01:50 +0200
Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it Jarod Wilson <jarod@redhat.com> - 2015-08-17 23:10 +0200
Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it Michal Kubecek <mkubecek@suse.cz> - 2015-08-18 09:50 +0200
| From | Jarod Wilson <jarod@redhat.com> |
|---|---|
| Date | 2015-08-13 20:10 +0200 |
| Subject | [PATCH 0/6] bonding: only advertise LRO if underlying hardware can LRO |
| Message-ID | <pX56V-2ug-11@gated-at.bofh.it> |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Jarod Wilson <jarod@redhat.com> |
|---|---|
| Date | 2015-08-13 20:10 +0200 |
| Subject | [PATCH 1/6] net/bonding: enable LRO if one device supports it |
| Message-ID | <pX56Y-2ug-69@gated-at.bofh.it> |
| In reply to | #1207039 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Michal Kubecek <mkubecek@suse.cz> |
|---|---|
| Date | 2015-08-14 09:00 +0200 |
| Subject | Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it |
| Message-ID | <pXh85-2TO-1@gated-at.bofh.it> |
| In reply to | #1207042 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jarod Wilson <jarod@redhat.com> |
|---|---|
| Date | 2015-08-15 01:50 +0200 |
| Subject | Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it |
| Message-ID | <pXwTv-fI-1@gated-at.bofh.it> |
| In reply to | #1207353 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jarod Wilson <jarod@redhat.com> |
|---|---|
| Date | 2015-08-17 23:10 +0200 |
| Subject | Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it |
| Message-ID | <pYzPj-27r-7@gated-at.bofh.it> |
| In reply to | #1207942 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Michal Kubecek <mkubecek@suse.cz> |
|---|---|
| Date | 2015-08-18 09:50 +0200 |
| Subject | Re: [PATCH 1/6] net/bonding: enable LRO if one device supports it |
| Message-ID | <pYJOF-88r-1@gated-at.bofh.it> |
| In reply to | #1208882 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web