RE: bonding reports interface up with 0 Mbps
From: Tantilov, Emil S <hidden>
Date: 2016-02-04 15:47:36
-----Original Message----- From: Jay Vosburgh [mailto:jay.vosburgh@canonical.com] Sent: Wednesday, February 03, 2016 9:57 PM To: Tantilov, Emil S Cc: netdev@vger.kernel.org; gospo@cumulusnetworks.com; zhuyj; jiri@mellanox.com Subject: Re: bonding reports interface up with 0 Mbps Tantilov, Emil S [off-list ref] wrote:quoted
We are seeing an occasional issue where the bonding driver may reportinterface up with 0 Mbps:quoted
bond0: link status definitely up for interface eth0, 0 Mbps full duplex So far in all the failed traces I have collected this happens on NETDEV_CHANGELOWERSTATE event: <...>-20533 [000] .... 81811.041241: ixgbe_service_task: eth1: NIC Link is Up 10 Gbps, Flow Control: RX/TX <...>-20533 [000] .... 81811.041257: ixgbe_check_vf_rate_limit <-ixgbe_service_task <...>-20533 [000] .... 81811.041272: ixgbe_ping_all_vfs <-ixgbe_service_task kworker/u48:0-7503 [010] .... 81811.041345: ixgbe_get_stats64 <-dev_get_stats kworker/u48:0-7503 [010] .... 81811.041393: bond_netdev_event: eth1: event: 1b kworker/u48:0-7503 [010] .... 81811.041394: bond_netdev_event: eth1: IFF_SLAVE kworker/u48:0-7503 [010] .... 81811.041395: bond_netdev_event: eth1: slave->speed = ffffffff <...>-20533 [000] .... 81811.041407: ixgbe_ptp_overflow_check <-ixgbe_service_task kworker/u48:0-7503 [010] .... 81811.041407: bond_mii_monitor: bond0: link status definitely up for interface eth1, 0 Mbps full duplexFrom looking at the code that prints this, the "full" duplex is probably actually DUPLEX_UNKNOWN, but the netdev_info uses the expression slave->duplex ? "full" : "half", so DUPLEX_UNKNOWN at 0xff would print "full." This is what ixgbe_get_settings returns for speed and duplex if it is called when carrier is off.
But in this case carrier is on and regardless - ixgbe_get_settings() is not called as you can see from the trace, which is why speed and duplex are not set.
quoted
As a proof of concept I added NETDEV_CHANGELOWERSTATE inbond_slave_netdev_event() along with NETDEV_UP/CHANGE:quoted
diff --git a/drivers/net/bonding/bond_main.cb/drivers/net/bonding/bond_main.cquoted
index 56b5605..a9dac4c 100644--- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c@@ -3014,6 +3014,7 @@ static int bond_slave_netdev_event(unsigned longevent,quoted
break; case NETDEV_UP: case NETDEV_CHANGE: + case NETDEV_CHANGELOWERSTATE: bond_update_speed_duplex(slave); if (BOND_MODE(bond) == BOND_MODE_8023AD) bond_3ad_adapter_speed_duplex_changed(slave); With this change I have not seen 0 Mbps reported by the bonding driver(around 12 hour test up to this pointquoted
vs. 2-3 hours otherwise). Although I suppose it could also be some sort ofrace/timing issue with bond_mii_monitor(). This change as a fix seems kind of odd, since CHANGELOWERSTATE is generated by bonding itself. Perhaps the net effect is to add a delay and then update the speed and duplex, masking the actual problem.
I added this case to make sure bond_update_speed_duplex() is called which will get the speed/duplex from ixgbe_get_settings.
Emil, if I recall correctly, the test patch I send that uses the notifiers directly instead of miimon (specify miimon=0 and have bonding respond to the notifiers) handled everything properly, right? If so I
Yes, but I think the code path was also different as I recall the "definitely up" message was not displayed as often.
quoted hunk ↗ jump to hunk
can split that up and submit it properly; it seems more like a feature than a straightforward bug fix, so I'm not sure it's appropriate for net. As a possibly less complex alternative for the miimon > 0 case, could you try the following:diff --git a/drivers/net/bonding/bond_main.cb/drivers/net/bonding/bond_main.c index 56b560558884..ac8921e65f26 100644--- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c@@ -2120,6 +2120,7 @@ static void bond_miimon_commit(struct bonding *bond){ struct list_head *iter; struct slave *slave, *primary; + int link_state; bond_for_each_slave(bond, slave, iter) { switch (slave->new_link) {@@ -2127,6 +2128,10 @@ static void bond_miimon_commit(struct bonding *bond)continue; case BOND_LINK_UP: + link_state = bond_check_dev_link(bond, slave->dev, 0); + if (!link_state) + continue; + bond_update_speed_duplex(slave); bond_set_slave_link_state(slave, BOND_LINK_UP, BOND_SLAVE_NOTIFY_NOW); slave->last_link_up = jiffies; This will make bonding recheck the link state and update the speed and duplex after it acquires RTNL to commit a link change. This probably still has a race, since the change of carrier state in the device is not mutexed by anything bonding can acquire (so it can always change as soon as it's checked).
Sure, I'll give this a try, but I'm not sure this check applies in this case as you can see from the trace link is up and carrier is on. Thanks, Emil