[PATCH] batman-adv: Revert "disable ethtool link speed detection when auto negotiation off"
The commit d60b8fc69ef2 ("batman-adv: disable ethtool link speed detection when auto negotiation off") disabled the usage of ethtool's link_ksetting when auto negotation was enabled due to invalid values when used with tun/tap virtual net_devices. According to the patch, automatic measurements should be used for these kind of interfaces.
But there are major flaws with this argumentation:
* automatic measurements are not implemented * auto negotiation has nothing to do with the validity of the retrieved values
The first point has to be fixed by a longer patch series. The "validity" part of the second point must be addressed in the same patch series by dropping the usage of ethtool's link_ksetting (thus always doing automatic measurements over ethernet).
Drop the patch again to have more default values for various net_device types/configurations. The user can still overwrite them using the batadv_hardif's BATADV_ATTR_THROUGHPUT_OVERRIDE.
Reported-by: Matthias Schiffer mschiffer@universe-factory.net Signed-off-by: Sven Eckelmann sven@narfation.org --- compat-include/linux/ethtool.h | 2 -- net/batman-adv/bat_v_elp.c | 15 +-------------- 2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/compat-include/linux/ethtool.h b/compat-include/linux/ethtool.h index e1f39c33..8dcbe02c 100644 --- a/compat-include/linux/ethtool.h +++ b/compat-include/linux/ethtool.h @@ -21,7 +21,6 @@ struct batadv_ethtool_link_ksettings { struct { __u32 speed; __u8 duplex; - __u8 autoneg; } base; };
@@ -42,7 +41,6 @@ batadv_ethtool_get_link_ksettings(struct net_device *dev, return ret;
link_ksettings->base.duplex = cmd.duplex; - link_ksettings->base.autoneg = cmd.autoneg; link_ksettings->base.speed = ethtool_cmd_speed(&cmd);
return 0; diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c index 2614a9ca..a39af0ee 100644 --- a/net/batman-adv/bat_v_elp.c +++ b/net/batman-adv/bat_v_elp.c @@ -120,20 +120,7 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) rtnl_lock(); ret = __ethtool_get_link_ksettings(hard_iface->net_dev, &link_settings); rtnl_unlock(); - - /* Virtual interface drivers such as tun / tap interfaces, VLAN, etc - * tend to initialize the interface throughput with some value for the - * sake of having a throughput number to export via ethtool. This - * exported throughput leaves batman-adv to conclude the interface - * throughput is genuine (reflecting reality), thus no measurements - * are necessary. - * - * Based on the observation that those interface types also tend to set - * the link auto-negotiation to 'off', batman-adv shall check this - * setting to differentiate between genuine link throughput information - * and placeholders installed by virtual interfaces. - */ - if (ret == 0 && link_settings.base.autoneg == AUTONEG_ENABLE) { + if (ret == 0) { /* link characteristics might change over time */ if (link_settings.base.duplex == DUPLEX_FULL) hard_iface->bat_v.flags |= BATADV_FULL_DUPLEX;
On Monday, 25 November 2019 17:46:50 HKT Sven Eckelmann wrote:
The commit d60b8fc69ef2 ("batman-adv: disable ethtool link speed detection when auto negotiation off") disabled the usage of ethtool's link_ksetting when auto negotation was enabled due to invalid values when used with tun/tap virtual net_devices. According to the patch, automatic measurements should be used for these kind of interfaces.
But there are major flaws with this argumentation:
- automatic measurements are not implemented
- auto negotiation has nothing to do with the validity of the retrieved values
The first point has to be fixed by a longer patch series. The "validity" part of the second point must be addressed in the same patch series by dropping the usage of ethtool's link_ksetting (thus always doing automatic measurements over ethernet).
Drop the patch again to have more default values for various net_device types/configurations. The user can still overwrite them using the batadv_hardif's BATADV_ATTR_THROUGHPUT_OVERRIDE.
I am not quite clear on how reverting this patch will get us better default values. In the case reported by Matthias the autoneg detection was working as intended by this very patch you are reverting. As Antonio had originally outlined:
The problem with autonegotiation disabled is that the advertised speed is likely to be a random number set by default by the driver.
This patch was the main reason why Matthias (or Gluon users) even realized that there was an issue with certain Ethernet ports & BATMAN V. Without the patch BATMAN V may have created routing loops and Gluon users would complain about those instead.
There is no disagreement that the situation needs improving but why is reverting this autoneg patch the best course of action ?
Cheers, Marek
On Monday, 25 November 2019 11:02:17 CET Marek Lindner wrote: [...]
Drop the patch again to have more default values for various net_device types/configurations. The user can still overwrite them using the batadv_hardif's BATADV_ATTR_THROUGHPUT_OVERRIDE.
I am not quite clear on how reverting this patch will get us better default values. In the case reported by Matthias the autoneg detection was working as intended by this very patch you are reverting. As Antonio had originally outlined:
The problem with autonegotiation disabled is that the advertised speed is likely to be a random number set by default by the driver.
It is also not reliable with auto negotiation. And disabled auto negotiation is also used rather often in complete valid setups with fixed links.
And I never said "better" - I said "more". And the other cases can be solved the same way as described in the original patch - override them.
This patch was the main reason why Matthias (or Gluon users) even realized that there was an issue with certain Ethernet ports & BATMAN V. Without the patch BATMAN V may have created routing loops and Gluon users would complain about those instead.
Routing loops? Are you sure? If this is the case than we have a major flaw in B.A.T.M.A.N. V and should think about disabling it again. This should never happen - because bad measurements can always happen. And who says that it is not a common case (even without ethernet).... maybe because it will be rather normal in the wild due to the various intepretation what get_expected_throughput should represent. Every non-minstrel based driver seems to have its own idea of what get_expected_throughput should return.
The only thing I am aware of are the invalid looking throughput values. But as outlined before, ethtool's link_ksetting are a rather bad source for neighbor specific throughput measurements. But it is the only thing which we have without a neighbor specific measurement for neighbors.
There is no disagreement that the situation needs improving but why is reverting this autoneg patch the best course of action ?
"auto negotiation has nothing to do with the validity of the retrieved values"
And gluon users/developers noticed that a lot of ethtool's link_ksetting were not used because of this patch.
Another option would be to completely drop the usage of ethtool's link_ksettings without having automatic measurements. But I think no one will be happy with it.
Kind regards, Sven
b.a.t.m.a.n@lists.open-mesh.org
-
Marek Lindner -
Sven Eckelmann