netdev - Re: [PATCH net-next] team: add ethtool get_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190614083225.GE2242@nanopsycho>
Date:   Fri, 14 Jun 2019 10:32:25 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net-next] team: add ethtool get_link_ksettings

Thu, Jun 13, 2019 at 08:16:49AM CEST, liuhangbin@...il.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@...il.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