| 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
| ||
|
Message-ID: <27035.1362606373@death.nxdomain>
Date: Wed, 06 Mar 2013 13:46:13 -0800
From: Jay Vosburgh <fubar@...ibm.com>
To: Andy Gospodarek <andy@...yhouse.net>
cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next] bond: add support to read speed and duplex via ethtool
Andy Gospodarek <andy@...yhouse.net> wrote:
>On Wed, Mar 06, 2013 at 11:25:12AM -0800, Jay Vosburgh wrote:
>> Andy Gospodarek <andy@...yhouse.net> wrote:
>>
>> >This patch adds support for the get_settings ethtool op to the bonding
>> >driver. This was motivated by users who wanted to get the speed of the
>> >bond and compare that against throughput to understand utilization.
>> >The behavior before this patch was added was problematic when computing
>> >line utilization after trying to get link-speed and throughput via SNMP.
>> >
>> >The general plan for computing link-speed was as follows:
>> >
>> >Mode Formula
>> >---- -------
>> >active-backup speed of current active slave
>> >broadcast speed of first slave with known speed
>> >all other modes aggregate speed of all slaves with known speed
>>
>> I'll just point out that the balance-tlb mode is asymmetric; it
>> uses all slaves for transmission, but only one slave for reception.
>> Ethtool only has a single speed for both directions, so this is probably
>> the best choice, but it should still be noted.
>
>Thanks for pointing that out. I have a feeling there will be a v2, so
>I'll try and update the changelog to reflect that. For the record, this
>same limitation exists when connecting to most switches and using
>round-robin, so I didn't feel the need to differentiate possibly
>asymmetric speeds.
>
>> >Output from ethtool looks like this for a round-robin bond:
>> >
>> >Settings for bond0:
>> > Supported ports: [ ]
>> > Supported link modes: Not reported
>> > Supported pause frame use: No
>> > Supports auto-negotiation: No
>> > Advertised link modes: Not reported
>> > Advertised pause frame use: No
>> > Advertised auto-negotiation: No
>> > Speed: 11000Mb/s
>> > Duplex: Full
>> > Port: Twisted Pair
>> > PHYAD: 0
>> > Transceiver: internal
>> > Auto-negotiation: off
>> > MDI-X: Unknown
>> > Link detected: yes
>> >
>> >I tested this and verified it works as expected. A test was also done
>> >on a version backported to an older kernel and it worked well there.
>> >
>> >Signed-off-by: Andy Gospodarek <andy@...yhouse.net>
>> >---
>> > drivers/net/bonding/bond_main.c | 47 +++++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 47 insertions(+)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index 7bd068a..6e70ff0 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -4224,6 +4224,52 @@ void bond_set_mode_ops(struct bonding *bond, int mode)
>> > }
>> > }
>> >
>> >+static int bond_ethtool_get_settings(struct net_device *bond_dev,
>> >+ struct ethtool_cmd *ecmd)
>> >+{
>> >+ struct bonding *bond = netdev_priv(bond_dev);
>> >+ struct slave *slave;
>> >+ int i;
>> >+ unsigned long speed = 0;
>> >+
>> >+ ecmd->speed = SPEED_UNKNOWN;
>> >+ ecmd->duplex = DUPLEX_UNKNOWN;
>> >+
>> >+ read_lock(&bond->lock);
>> >+ switch (bond->params.mode) {
>> >+ case BOND_MODE_ACTIVEBACKUP:
>> >+ read_lock(&bond->curr_slave_lock);
>> >+ if (bond->curr_active_slave &&
>> >+ bond->curr_active_slave->speed != SPEED_UNKNOWN) {
>> >+ ecmd->speed = bond->curr_active_slave->speed;
>> >+ ecmd->duplex = bond->curr_active_slave->duplex;
>> >+ }
>> >+ read_unlock(&bond->curr_slave_lock);
>> >+ break;
>> >+ case BOND_MODE_BROADCAST:
>> >+ bond_for_each_slave(bond, slave, i) {
>> >+ if (slave->speed != SPEED_UNKNOWN) {
>> >+ ecmd->speed = slave->speed;
>> >+ ecmd->duplex = slave->duplex;
>> >+ break;
>> >+ }
>> >+ }
>> >+ break;
>>
>> Does anybody really use broadcast mode? Not that I'm saying
>> this is incorrect, I'm just wondering in general.
>>
>
>I don't imagine they do, but wanted to add something for it since it
>would not reallyu fall into the default case well.
>
>> >+ default:
>> >+ bond_for_each_slave(bond, slave, i) {
>> >+ if (slave->speed != SPEED_UNKNOWN) {
>> >+ speed += slave->speed;
>> >+ }
>> >+ if (ecmd->duplex == DUPLEX_UNKNOWN &&
>> >+ slave->duplex != DUPLEX_UNKNOWN)
>> >+ ecmd->duplex = slave->duplex;
>>
>> Should the calculations skip slaves that are not BOND_LINK_UP?
>> If the ARP monitor is running, some slaves may be carrier up (and have
>> slave->speed set), but are not actually in use by the bond, at least for
>> transmission.
>>
>
>That would be fine with me. If you would like I can add that for a v2.
>It would produce a more honest estimate of what the maximum throughput
>would be at that point in time.
Yes, I think so; it's going to be an estimate for any of the
load balance modes, but it ought to be as close as is reasonable to what
kind of throughput would be expected.
I also think it might be odd if it were possible for a bond to
simultaneously show as carrier down, but speed as something very high.
-J
>> >+ }
>> >+ ecmd->speed = speed;
>> >+ }
>> >+ read_unlock(&bond->lock);
>> >+ return 0;
>> >+}
>> >+
>> > static void bond_ethtool_get_drvinfo(struct net_device *bond_dev,
>> > struct ethtool_drvinfo *drvinfo)
>> > {
>> >@@ -4235,6 +4281,7 @@ static void bond_ethtool_get_drvinfo(struct net_device *bond_dev,
>> >
>> > static const struct ethtool_ops bond_ethtool_ops = {
>> > .get_drvinfo = bond_ethtool_get_drvinfo,
>> >+ .get_settings = bond_ethtool_get_settings,
>> > .get_link = ethtool_op_get_link,
>> > };
>> >
>> >--
>> >1.7.11.7
>>
>> ---
>> -Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists