Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1259607
| From | Jarod Wilson <jarod@redhat.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles |
| Date | 2015-10-30 17:30 +0100 |
| Message-ID | <qpkIW-84i-7@gated-at.bofh.it> (permalink) |
| References | <qmY09-59l-3@gated-at.bofh.it> <qn01X-89V-3@gated-at.bofh.it> <qnMzE-6zx-11@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Michal Kubecek wrote:
> On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
>>> + struct net_device *upper, netdev_features_t features)
>>> +{
>>> + netdev_features_t want = upper->wanted_features& lower->hw_features;
>>> +
>>> + if (!(upper->wanted_features& NETIF_F_LRO)
>>> + && (features& NETIF_F_LRO)) {
>>> + netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
>>> + upper->name);
>>> + features&= ~NETIF_F_LRO;
>>> + } else if ((want& NETIF_F_LRO)&& !(features& NETIF_F_LRO)) {
>>> + netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
>>> + upper->name);
>>> + features |= NETIF_F_LRO;
>>> + }
>>> +
>>> + return features;
>>> +}
>>> +
>> I'd say to drop the second half of this statement. LRO is a feature
>> that should be enabled explicitly per interface. If someone enables
>> LRO on the master they may only want it on one interface. The fact
>> is there are some implementations of LRO that work better than
>> others so you want to give the end user the option to mix and match.
>
> Agreed. IMHO it makes sense to allow setups with LRO disabled on some
> slaves and enabled on other.
>
> Also, the logic seems to only consider the 1 upper : N lower scheme
> (bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
> For these, there is no way to propagate both 0 and 1 down as this would
> result in a conflict.
Okay, so we're thinking do prevent lower devices turning LRO on if the
upper device has it off. Or rather, if *an* upper device has it off.
Probably need to rework the bit that calls this function to use
netdev_for_each_upper_dev{_rcu}() to walk all of adj_list.upper here.
Rather than outright dropping the second bit though, I was thinking
maybe just drop a note in dmesg along the lines of "hey, you shut off
LRO, it is still enabled on upper dev foo", to placate end-users.
>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>> + struct net_device *lower, netdev_features_t features)
>>> +{
>>> + netdev_features_t want = features& lower->hw_features;
>>> +
>>> + if (!(features& NETIF_F_LRO)&& (lower->features& NETIF_F_LRO)) {
>>> + netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>> + lower->name);
>>> + upper->wanted_features&= ~NETIF_F_LRO;
>>> + lower->wanted_features&= ~NETIF_F_LRO;
>>> + netdev_update_features(lower);
>>> + if (unlikely(lower->features& NETIF_F_LRO))
>>> + netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>> + lower->name);
>>> + } else if ((want& NETIF_F_LRO)&& !(lower->features& NETIF_F_LRO)) {
>>> + netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>> + lower->name);
>>> + upper->wanted_features |= NETIF_F_LRO;
>>> + lower->wanted_features |= NETIF_F_LRO;
>>> + netdev_update_features(lower);
>>> + if (unlikely(!(lower->features& NETIF_F_LRO)))
>>> + netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>> + lower->name);
>>> + }
>>> +}
>>> +
>> Same thing here. If a lower dev has it disabled then leave it
>> disabled. I believe your goal is to make it so that
>> dev_disable_lro() can shut down LRO when it is making packets in the
>> data-path unusable.
>
> This is already the case since commit fbe168ba91f7 ("net: generic
> dev_disable_lro() stacked device handling"). That commit makes sure
> dev_disable_lro() is propagated down the stack and also makes sure new
> slaves added to a bond/team with LRO disabled have it disabled too.
>
> What it does not do is propagating LRO disabling down if it is disabled
> in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
> sure if this should be done or not, both options have their pros and
> cons.
Making it work with ethtool was one of my primary goals with this
change, as it was users prodding things with ethtool that prompted the
"hey, this doesn't make sense" bug reports.
> However, I believe enabling LRO shouldn't be propagated down.
Hm. Devices that should never have LRO enabled still won't get it
enabled, so I'm not clear what harm it would cause. I tend to think you
do want this sync'ing down the stack if set on an upper dev (i.e.,
ethtool -K bond0 lro on), for consistency's sake. You can always come
back through afterwards and disable things on lower devs individually if
they're really not wanted, since we're in agreement that we shouldn't
prevent disabling features on lower devices.
--
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/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Jarod Wilson <jarod@redhat.com> - 2015-10-24 05:50 +0200
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Tom Herbert <tom@herbertland.com> - 2015-10-24 06:50 +0200
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-24 08:00 +0200
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Michal Kubecek <mkubecek@suse.cz> - 2015-10-26 10:50 +0100
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Jarod Wilson <jarod@redhat.com> - 2015-10-30 17:30 +0100
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-30 21:10 +0100
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Jarod Wilson <jarod@redhat.com> - 2015-11-02 18:40 +0100
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Jarod Wilson <jarod@redhat.com> - 2015-10-30 17:40 +0100
Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-30 21:20 +0100
[PATCH net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-02 19:00 +0100
Re: [PATCH net-next] net/core: generic support for disabling netdev features down stack Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-02 19:10 +0100
Re: [PATCH net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-02 23:00 +0100
[PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 04:00 +0100
Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack David Miller <davem@davemloft.net> - 2015-11-03 05:50 +0100
Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 11:10 +0100
Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Geert Uytterhoeven <geert@linux-m68k.org> - 2015-11-03 15:00 +0100
Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 15:00 +0100
Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 15:10 +0100
Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 17:10 +0100
[PATCH net-next] net/core: fix for_each_netdev_feature Jarod Wilson <jarod@redhat.com> - 2015-11-03 16:20 +0100
Re: [PATCH net-next] net/core: fix for_each_netdev_feature Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 16:50 +0100
Re: [PATCH net-next] net/core: fix for_each_netdev_feature David Miller <davem@davemloft.net> - 2015-11-03 17:40 +0100
[PATCH net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 21:40 +0100
Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-03 22:20 +0100
Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 23:20 +0100
Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-04 00:10 +0100
Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 22:30 +0100
Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs Michal Kubecek <mkubecek@suse.cz> - 2015-11-03 23:00 +0100
Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 23:00 +0100
[PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-04 05:10 +0100
Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs David Miller <davem@davemloft.net> - 2015-11-05 04:00 +0100
Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Florian Fainelli <f.fainelli@gmail.com> - 2015-11-13 01:30 +0100
Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Jiri Pirko <jiri@resnulli.us> - 2015-11-13 11:30 +0100
Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-13 12:00 +0100
Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Laura Abbott <labbott@redhat.com> - 2015-11-13 23:40 +0100
csiph-web