[PATCH net-next] team: add ethtool get_link_ksettings

[PATCH net-next] team: add ethtool get_link_ksettings

[Hangbin Liu]

Like bond, add ethtool get_link_ksettings to show the total speed.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/team/team.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 2106045b3e16..5e892ee4c006 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2058,9 +2058,34 @@ static void team_ethtool_get_drvinfo(struct net_device *dev,
 	strlcpy(drvinfo->version, UTS_RELEASE, sizeof(drvinfo->version));
 }
 
+static int team_ethtool_get_link_ksettings(struct net_device *dev,
+					   struct ethtool_link_ksettings *cmd)
+{
+	struct team *team= netdev_priv(dev);
+	unsigned long speed = 0;
+	struct team_port *port;
+
+	cmd->base.duplex = DUPLEX_UNKNOWN;
+	cmd->base.port = PORT_OTHER;
+
+	list_for_each_entry(port, &team->port_list, list) {
+		if (team_port_txable(port)) {
+			if (port->state.speed != SPEED_UNKNOWN)
+				speed += port->state.speed;
+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
+			    port->state.duplex != DUPLEX_UNKNOWN)
+				cmd->base.duplex = port->state.duplex;
+		}
+	}
+	cmd->base.speed = speed ? : SPEED_UNKNOWN;
+
+	return 0;
+}
+
 static const struct ethtool_ops team_ethtool_ops = {
 	.get_drvinfo		= team_ethtool_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
+	.get_link_ksettings	= team_ethtool_get_link_ksettings,
 };
 
 /***********************
-- 
2.19.2

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Mon, 27 May 2019 11:31:10 +0800

> Like bond, add ethtool get_link_ksettings to show the total speed.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Yeah, this mirrors what bonding does.

Jiri, please review.

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[Jiri Pirko]

Mon, May 27, 2019 at 05:31:10AM CEST, liuhangbin@gmail.com wrote:
>Like bond, add ethtool get_link_ksettings to show the total speed.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/team/team.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index 2106045b3e16..5e892ee4c006 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -2058,9 +2058,34 @@ static void team_ethtool_get_drvinfo(struct net_device *dev,
> 	strlcpy(drvinfo->version, UTS_RELEASE, sizeof(drvinfo->version));
> }
> 
>+static int team_ethtool_get_link_ksettings(struct net_device *dev,
>+					   struct ethtool_link_ksettings *cmd)
>+{
>+	struct team *team= netdev_priv(dev);
>+	unsigned long speed = 0;
>+	struct team_port *port;
>+
>+	cmd->base.duplex = DUPLEX_UNKNOWN;
>+	cmd->base.port = PORT_OTHER;
>+
>+	list_for_each_entry(port, &team->port_list, list) {
>+		if (team_port_txable(port)) {
>+			if (port->state.speed != SPEED_UNKNOWN)
>+				speed += port->state.speed;
>+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
>+			    port->state.duplex != DUPLEX_UNKNOWN)
>+				cmd->base.duplex = port->state.duplex;

What is exactly the point of this patch? Why do you need such
information. This is hw-related info. If you simply sum-up all txable
ports, the value is always highly misleading.

For example for hash-based port selection with 2 100Mbit ports,
you will get 200Mbit, but it is not true. It is up to the traffic and
hash function what is the actual TX speed you can get.
On the RX side, this is even more misleading as the actual speed depends
on the other side of the wire.


>+		}
>+	}
>+	cmd->base.speed = speed ? : SPEED_UNKNOWN;
>+
>+	return 0;
>+}
>+
> static const struct ethtool_ops team_ethtool_ops = {
> 	.get_drvinfo		= team_ethtool_get_drvinfo,
> 	.get_link		= ethtool_op_get_link,
>+	.get_link_ksettings	= team_ethtool_get_link_ksettings,
> };
> 
> /***********************
>-- 
>2.19.2
>

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[Hangbin Liu]

On Tue, May 28, 2019 at 11:08:23AM +0200, Jiri Pirko wrote:
> >+static int team_ethtool_get_link_ksettings(struct net_device *dev,
> >+					   struct ethtool_link_ksettings *cmd)
> >+{
> >+	struct team *team= netdev_priv(dev);
> >+	unsigned long speed = 0;
> >+	struct team_port *port;
> >+
> >+	cmd->base.duplex = DUPLEX_UNKNOWN;
> >+	cmd->base.port = PORT_OTHER;
> >+
> >+	list_for_each_entry(port, &team->port_list, list) {
> >+		if (team_port_txable(port)) {
> >+			if (port->state.speed != SPEED_UNKNOWN)
> >+				speed += port->state.speed;
> >+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
> >+			    port->state.duplex != DUPLEX_UNKNOWN)
> >+				cmd->base.duplex = port->state.duplex;
> 
> What is exactly the point of this patch? Why do you need such
> information. This is hw-related info. If you simply sum-up all txable
> ports, the value is always highly misleading.
> 
> For example for hash-based port selection with 2 100Mbit ports,
> you will get 200Mbit, but it is not true. It is up to the traffic and
> hash function what is the actual TX speed you can get.
> On the RX side, this is even more misleading as the actual speed depends
> on the other side of the wire.

The number is the maximum speed in theory. I added it because someone
said bond interface could show total speed while team could not...
The usage is customer could get team link-speed and throughput via SNMP.

Thanks
Hangbin
> 
> 
> >+		}
> >+	}
> >+	cmd->base.speed = speed ? : SPEED_UNKNOWN;
> >+
> >+	return 0;
> >+}
> >+
> > static const struct ethtool_ops team_ethtool_ops = {
> > 	.get_drvinfo		= team_ethtool_get_drvinfo,
> > 	.get_link		= ethtool_op_get_link,
> >+	.get_link_ksettings	= team_ethtool_get_link_ksettings,
> > };
> > 
> > /***********************
> >-- 
> >2.19.2
> >

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[Jiri Pirko]

Tue, May 28, 2019 at 12:02:11PM CEST, liuhangbin@gmail.com wrote:
>On Tue, May 28, 2019 at 11:08:23AM +0200, Jiri Pirko wrote:
>> >+static int team_ethtool_get_link_ksettings(struct net_device *dev,
>> >+					   struct ethtool_link_ksettings *cmd)
>> >+{
>> >+	struct team *team= netdev_priv(dev);
>> >+	unsigned long speed = 0;
>> >+	struct team_port *port;
>> >+
>> >+	cmd->base.duplex = DUPLEX_UNKNOWN;
>> >+	cmd->base.port = PORT_OTHER;
>> >+
>> >+	list_for_each_entry(port, &team->port_list, list) {
>> >+		if (team_port_txable(port)) {
>> >+			if (port->state.speed != SPEED_UNKNOWN)
>> >+				speed += port->state.speed;
>> >+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
>> >+			    port->state.duplex != DUPLEX_UNKNOWN)
>> >+				cmd->base.duplex = port->state.duplex;
>> 
>> What is exactly the point of this patch? Why do you need such
>> information. This is hw-related info. If you simply sum-up all txable
>> ports, the value is always highly misleading.
>> 
>> For example for hash-based port selection with 2 100Mbit ports,
>> you will get 200Mbit, but it is not true. It is up to the traffic and
>> hash function what is the actual TX speed you can get.
>> On the RX side, this is even more misleading as the actual speed depends
>> on the other side of the wire.
>
>The number is the maximum speed in theory. I added it because someone

"in theory" is not what this value should return in my opinion.


>said bond interface could show total speed while team could not...
>The usage is customer could get team link-speed and throughput via SNMP.

Has no meaning though :/


>
>Thanks
>Hangbin
>> 
>> 
>> >+		}
>> >+	}
>> >+	cmd->base.speed = speed ? : SPEED_UNKNOWN;
>> >+
>> >+	return 0;
>> >+}
>> >+
>> > static const struct ethtool_ops team_ethtool_ops = {
>> > 	.get_drvinfo		= team_ethtool_get_drvinfo,
>> > 	.get_link		= ethtool_op_get_link,
>> >+	.get_link_ksettings	= team_ethtool_get_link_ksettings,
>> > };
>> > 
>> > /***********************
>> >-- 
>> >2.19.2
>> >

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[Hangbin Liu]

On Tue, May 28, 2019 at 01:24:31PM +0200, Jiri Pirko wrote:
> Tue, May 28, 2019 at 12:02:11PM CEST, liuhangbin@gmail.com wrote:
> >On Tue, May 28, 2019 at 11:08:23AM +0200, Jiri Pirko wrote:
> >> >+static int team_ethtool_get_link_ksettings(struct net_device *dev,
> >> >+					   struct ethtool_link_ksettings *cmd)
> >> >+{
> >> >+	struct team *team= netdev_priv(dev);
> >> >+	unsigned long speed = 0;
> >> >+	struct team_port *port;
> >> >+
> >> >+	cmd->base.duplex = DUPLEX_UNKNOWN;
> >> >+	cmd->base.port = PORT_OTHER;
> >> >+
> >> >+	list_for_each_entry(port, &team->port_list, list) {
> >> >+		if (team_port_txable(port)) {
> >> >+			if (port->state.speed != SPEED_UNKNOWN)
> >> >+				speed += port->state.speed;
> >> >+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
> >> >+			    port->state.duplex != DUPLEX_UNKNOWN)
> >> >+				cmd->base.duplex = port->state.duplex;
> >> 
> >> What is exactly the point of this patch? Why do you need such
> >> information. This is hw-related info. If you simply sum-up all txable
> >> ports, the value is always highly misleading.
> >> 
> >> For example for hash-based port selection with 2 100Mbit ports,
> >> you will get 200Mbit, but it is not true. It is up to the traffic and
> >> hash function what is the actual TX speed you can get.
> >> On the RX side, this is even more misleading as the actual speed depends
> >> on the other side of the wire.
> >
> >The number is the maximum speed in theory. I added it because someone

Hi Jiri,

Sorry for the late reply.

> 
> "in theory" is not what this value should return in my opinion.

Would you please give some hits about what "in theory" value we should return?

In my understanding, it just shows the maximum in theory speed. Just like a
NIC card shows the speed 1000Mb/s, but the actually max speed could be only
700-900 Mb/s for tcp/udp testing. No need to say if the other side's max speed
is only 100Mb/s, we will get lower speed value.

So I think with ab, rr, lb, random mode, the team speed could be the summary of
total active ports' speed. The only controversial mode may be the broadcast
mode as it just broadcast all the data from all ports. But it do send all the
data. If we ignore the fault tolerance sutff, all the bandwidth are used. The
speed shows the total number of all NICs looks also make sense.

Hope I made it clear and you could got what I mean..
> 
> 
> >said bond interface could show total speed while team could not...
> >The usage is customer could get team link-speed and throughput via SNMP.
> 
> Has no meaning though :/

Anyway, the customer is looking for this feature. Shouldn't we
consider about the requirement?

Thanks
Hangbin

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[Jiri Pirko]

Thu, Jun 13, 2019 at 08:16:49AM CEST, liuhangbin@gmail.com wrote:
>On Tue, May 28, 2019 at 01:24:31PM +0200, Jiri Pirko wrote:
>> Tue, May 28, 2019 at 12:02:11PM CEST, liuhangbin@gmail.com wrote:
>> >On Tue, May 28, 2019 at 11:08:23AM +0200, Jiri Pirko wrote:
>> >> >+static int team_ethtool_get_link_ksettings(struct net_device *dev,
>> >> >+					   struct ethtool_link_ksettings *cmd)
>> >> >+{
>> >> >+	struct team *team= netdev_priv(dev);
>> >> >+	unsigned long speed = 0;
>> >> >+	struct team_port *port;
>> >> >+
>> >> >+	cmd->base.duplex = DUPLEX_UNKNOWN;
>> >> >+	cmd->base.port = PORT_OTHER;
>> >> >+
>> >> >+	list_for_each_entry(port, &team->port_list, list) {
>> >> >+		if (team_port_txable(port)) {
>> >> >+			if (port->state.speed != SPEED_UNKNOWN)
>> >> >+				speed += port->state.speed;
>> >> >+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
>> >> >+			    port->state.duplex != DUPLEX_UNKNOWN)
>> >> >+				cmd->base.duplex = port->state.duplex;
>> >> 
>> >> What is exactly the point of this patch? Why do you need such
>> >> information. This is hw-related info. If you simply sum-up all txable
>> >> ports, the value is always highly misleading.
>> >> 
>> >> For example for hash-based port selection with 2 100Mbit ports,
>> >> you will get 200Mbit, but it is not true. It is up to the traffic and
>> >> hash function what is the actual TX speed you can get.
>> >> On the RX side, this is even more misleading as the actual speed depends
>> >> on the other side of the wire.
>> >
>> >The number is the maximum speed in theory. I added it because someone
>
>Hi Jiri,
>
>Sorry for the late reply.
>
>> 
>> "in theory" is not what this value should return in my opinion.
>
>Would you please give some hits about what "in theory" value we should return?
>
>In my understanding, it just shows the maximum in theory speed. Just like a
>NIC card shows the speed 1000Mb/s, but the actually max speed could be only
>700-900 Mb/s for tcp/udp testing. No need to say if the other side's max speed
>is only 100Mb/s, we will get lower speed value.
>
>So I think with ab, rr, lb, random mode, the team speed could be the summary of
>total active ports' speed. The only controversial mode may be the broadcast
>mode as it just broadcast all the data from all ports. But it do send all the
>data. If we ignore the fault tolerance sutff, all the bandwidth are used. The
>speed shows the total number of all NICs looks also make sense.
>
>Hope I made it clear and you could got what I mean..

Yeah, I was thinking about this in the meantime and I admit this is
probably the best of the wrong approaches. The only correct one would be
to teach the userspace apps to understand the topology and report
parameters according to the runner, utilization, division of the
traffic etc. Not real.

Long story short, I'm okay with your patch. Thanks!



>> 
>> 
>> >said bond interface could show total speed while team could not...
>> >The usage is customer could get team link-speed and throughput via SNMP.
>> 
>> Has no meaning though :/
>
>Anyway, the customer is looking for this feature. Shouldn't we
>consider about the requirement?
>
>Thanks
>Hangbin

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[Jiri Pirko]

Mon, May 27, 2019 at 05:31:10AM CEST, liuhangbin@gmail.com wrote:
>Like bond, add ethtool get_link_ksettings to show the total speed.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next] team: add ethtool get_link_ksettings

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 14 Jun 2019 10:32:25 +0200

> Long story short, I'm okay with your patch. Thanks!

This patch should therefore be reposted.

[PATCHv2 net-next] team: add ethtool get_link_ksettings

[Hangbin Liu]

Like bond, add ethtool get_link_ksettings to show the total speed.

v2: no update, just repost.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/team/team.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b48006e7fa2f..f3422f85f604 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2054,9 +2054,34 @@ static void team_ethtool_get_drvinfo(struct net_device *dev,
 	strlcpy(drvinfo->version, UTS_RELEASE, sizeof(drvinfo->version));
 }
 
+static int team_ethtool_get_link_ksettings(struct net_device *dev,
+					   struct ethtool_link_ksettings *cmd)
+{
+	struct team *team= netdev_priv(dev);
+	unsigned long speed = 0;
+	struct team_port *port;
+
+	cmd->base.duplex = DUPLEX_UNKNOWN;
+	cmd->base.port = PORT_OTHER;
+
+	list_for_each_entry(port, &team->port_list, list) {
+		if (team_port_txable(port)) {
+			if (port->state.speed != SPEED_UNKNOWN)
+				speed += port->state.speed;
+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
+			    port->state.duplex != DUPLEX_UNKNOWN)
+				cmd->base.duplex = port->state.duplex;
+		}
+	}
+	cmd->base.speed = speed ? : SPEED_UNKNOWN;
+
+	return 0;
+}
+
 static const struct ethtool_ops team_ethtool_ops = {
 	.get_drvinfo		= team_ethtool_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
+	.get_link_ksettings	= team_ethtool_get_link_ksettings,
 };
 
 /***********************
-- 
2.19.2

Re: [PATCHv2 net-next] team: add ethtool get_link_ksettings

[Jiri Pirko]

Mon, Jun 17, 2019 at 03:32:55AM CEST, liuhangbin@gmail.com wrote:
>Like bond, add ethtool get_link_ksettings to show the total speed.
>
>v2: no update, just repost.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCHv2 net-next] team: add ethtool get_link_ksettings

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Mon, 17 Jun 2019 09:32:55 +0800

> Like bond, add ethtool get_link_ksettings to show the total speed.
> 
> v2: no update, just repost.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied, thank you.