diff mbox

[RFC,net-next] bonding: Use notifiers for slave link state detection

Message ID 27321.1452216515@famine
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Jay Vosburgh Jan. 8, 2016, 1:28 a.m. UTC 
TEST PATCH

	This patch modifies bonding to utilize notifier callbacks to
detect slave link state changes.  It is intended to be used with miimon
set to zero, and does not support the updelay or downdelay options to
bonding.  It's not as complicated as it looks; most of the change set is
to break out the inner loop of bond_miimon_inspect into its own
function.

	Yanjun, can you test this with miimon=0 and see if it changes
the behavior you're seeing?

	Thanks,

	-J




---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Comments

Tantilov, Emil S Jan. 9, 2016, 1:35 a.m. UTC | #1 
>-----Original Message-----
>From: Jay Vosburgh [mailto:jay.vosburgh@canonical.com]
>Sent: Thursday, January 07, 2016 5:29 PM
>Subject: [RFC PATCH net-next] bonding: Use notifiers for slave link state
>detection
>
>
>	TEST PATCH
>
>	This patch modifies bonding to utilize notifier callbacks to
>detect slave link state changes.  It is intended to be used with miimon
>set to zero, and does not support the updelay or downdelay options to
>bonding.  It's not as complicated as it looks; most of the change set is
>to break out the inner loop of bond_miimon_inspect into its own
>function.

Jay,
 
I managed to do a quick test with this patch and occasionally there is a case where
I see the bonding driver reporting link up for an interface (eth1) that is not up just yet:

[12972.741999] bonding: bond0 is being created...
[12972.761907] bond0: Setting xmit hash policy to layer3+4 (1)
[12972.761990] bond0: Setting MII monitoring interval to 0
[12972.767131] bond0: Setting LACP rate to fast (1)
[12972.767916] IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
[12972.846158] bond0: Adding slave eth0
[12972.950548] pps pps0: new PPS source ptp0
[12972.950555] ixgbe 0000:01:00.0: registered PHC device on eth0
[12973.071750] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
[12973.072023] bond0: Enslaving eth0 as a backup interface with an up link
[12974.122295] bond0: Adding slave eth1
[12974.227639] pps pps1: new PPS source ptp1
[12974.227645] ixgbe 0000:01:00.1: registered PHC device on eth1
[12974.349306] IPv6: ADDRCONF(NETDEV_UP): eth1: link is not ready
[12974.349584] bond0: Enslaving eth1 as a backup interface with an up link
[12982.982797] ixgbe 0000:01:00.0 eth0: NIC Link is Up 10 Gbps, Flow Control: RX/TX
[12983.068437] IPv6: ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
[12983.185720] ixgbe 0000:01:00.0 eth0: NIC Link is Down
[12983.982454] ixgbe 0000:01:00.0 eth0: speed changed to 0 for port eth0
[12983.982539] bond0: link status definitely down for interface eth0, disabling it
[12983.982546] bond0: link status definitely up for interface eth1, 0 Mbps full duplex
[12983.982550] bond0: first active interface up!
[12985.213752] ixgbe 0000:01:00.0 eth0: NIC Link is Up 10 Gbps, Flow Control: RX/TX
[12985.213970] bond0: link status definitely up for interface eth0, 10000 Mbps full duplex
[12985.213975] bond0: link status definitely up for interface eth1, 0 Mbps full duplex
[12989.195157] ixgbe 0000:01:00.1 eth1: NIC Link is Up 10 Gbps, Flow Control: RX/TX

Thanks,
Emil
diff mbox

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index cab99fd..6fe68b1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2012,104 +2012,103 @@  static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
 /*-------------------------------- Monitoring -------------------------------*/
 
 /* called with rcu_read_lock() */
-static int bond_miimon_inspect(struct bonding *bond)
+static int bond_miimon_inspect_slave(struct bonding *bond, struct slave *slave)
 {
-	int link_state, commit = 0;
-	struct list_head *iter;
-	struct slave *slave;
+	int link_state;
 	bool ignore_updelay;
 
 	ignore_updelay = !rcu_dereference(bond->curr_active_slave);
 
-	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
+	slave->new_link = BOND_LINK_NOCHANGE;
 
-		link_state = bond_check_dev_link(bond, slave->dev, 0);
+	link_state = bond_check_dev_link(bond, slave->dev, 0);
 
-		switch (slave->link) {
-		case BOND_LINK_UP:
-			if (link_state)
-				continue;
+	switch (slave->link) {
+	case BOND_LINK_UP:
+		if (link_state)
+			return 0;
 
-			bond_set_slave_link_state(slave, BOND_LINK_FAIL,
+		bond_set_slave_link_state(slave, BOND_LINK_FAIL,
+					  BOND_SLAVE_NOTIFY_LATER);
+		slave->delay = bond->params.downdelay;
+		if (slave->delay) {
+			netdev_info(bond->dev, "link status down for %sinterface %s, disabling it in %d ms\n",
+				    (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) ?
+				    (bond_is_active_slave(slave) ?
+				     "active " : "backup ") : "",
+				    slave->dev->name,
+				    bond->params.downdelay * bond->params.miimon);
+		}
+		/*FALLTHRU*/
+	case BOND_LINK_FAIL:
+		if (link_state) {
+			/* recovered before downdelay expired */
+			bond_set_slave_link_state(slave, BOND_LINK_UP,
 						  BOND_SLAVE_NOTIFY_LATER);
-			slave->delay = bond->params.downdelay;
-			if (slave->delay) {
-				netdev_info(bond->dev, "link status down for %sinterface %s, disabling it in %d ms\n",
-					    (BOND_MODE(bond) ==
-					     BOND_MODE_ACTIVEBACKUP) ?
-					     (bond_is_active_slave(slave) ?
-					      "active " : "backup ") : "",
-					    slave->dev->name,
-					    bond->params.downdelay * bond->params.miimon);
-			}
-			/*FALLTHRU*/
-		case BOND_LINK_FAIL:
-			if (link_state) {
-				/* recovered before downdelay expired */
-				bond_set_slave_link_state(slave, BOND_LINK_UP,
-							  BOND_SLAVE_NOTIFY_LATER);
-				slave->last_link_up = jiffies;
-				netdev_info(bond->dev, "link status up again after %d ms for interface %s\n",
-					    (bond->params.downdelay - slave->delay) *
-					    bond->params.miimon,
-					    slave->dev->name);
-				continue;
-			}
+			slave->last_link_up = jiffies;
+			netdev_info(bond->dev, "link status up again after %d ms for interface %s\n",
+				    (bond->params.downdelay - slave->delay) *
+				    bond->params.miimon, slave->dev->name);
+			return 0;
+		}
 
-			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_DOWN;
-				commit++;
-				continue;
-			}
+		if (slave->delay <= 0) {
+			slave->new_link = BOND_LINK_DOWN;
+			return 1;
+		}
 
-			slave->delay--;
-			break;
+		slave->delay--;
+		break;
 
-		case BOND_LINK_DOWN:
-			if (!link_state)
-				continue;
+	case BOND_LINK_DOWN:
+		if (!link_state)
+			return 0;
 
-			bond_set_slave_link_state(slave, BOND_LINK_BACK,
-						  BOND_SLAVE_NOTIFY_LATER);
-			slave->delay = bond->params.updelay;
-
-			if (slave->delay) {
-				netdev_info(bond->dev, "link status up for interface %s, enabling it in %d ms\n",
-					    slave->dev->name,
-					    ignore_updelay ? 0 :
-					    bond->params.updelay *
-					    bond->params.miimon);
-			}
-			/*FALLTHRU*/
-		case BOND_LINK_BACK:
-			if (!link_state) {
-				bond_set_slave_link_state(slave,
-							  BOND_LINK_DOWN,
-							  BOND_SLAVE_NOTIFY_LATER);
-				netdev_info(bond->dev, "link status down again after %d ms for interface %s\n",
-					    (bond->params.updelay - slave->delay) *
-					    bond->params.miimon,
-					    slave->dev->name);
+		bond_set_slave_link_state(slave, BOND_LINK_BACK,
+					  BOND_SLAVE_NOTIFY_LATER);
+		slave->delay = bond->params.updelay;
 
-				continue;
-			}
+		if (slave->delay) {
+			netdev_info(bond->dev, "link status up for interface %s, enabling it in %d ms\n",
+				    slave->dev->name, ignore_updelay ? 0 :
+				    bond->params.updelay * bond->params.miimon);
+		}
+		/*FALLTHRU*/
+	case BOND_LINK_BACK:
+		if (!link_state) {
+			bond_set_slave_link_state(slave, BOND_LINK_DOWN,
+						  BOND_SLAVE_NOTIFY_LATER);
+			netdev_info(bond->dev, "link status down again after %d ms for interface %s\n",
+				    (bond->params.updelay - slave->delay) *
+				    bond->params.miimon, slave->dev->name);
 
-			if (ignore_updelay)
-				slave->delay = 0;
+			return 0;
+		}
 
-			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_UP;
-				commit++;
-				ignore_updelay = false;
-				continue;
-			}
+		if (ignore_updelay)
+			slave->delay = 0;
 
-			slave->delay--;
-			break;
+		if (slave->delay <= 0) {
+			slave->new_link = BOND_LINK_UP;
+			return 1;
 		}
+
+		slave->delay--;
+		break;
 	}
 
+	return 0;
+}
+
+static int bond_miimon_inspect(struct bonding *bond)
+{
+	struct list_head *iter;
+	struct slave *slave;
+	int commit = 0;
+
+	bond_for_each_slave_rcu(bond, slave, iter)
+		commit += bond_miimon_inspect_slave(bond, slave);
+
 	return commit;
 }
 
@@ -3016,6 +3015,9 @@  static int bond_slave_netdev_event(unsigned long event,
 			bond_3ad_adapter_speed_duplex_changed(slave);
 		/* Fallthrough */
 	case NETDEV_DOWN:
+		if (bond_miimon_inspect_slave(bond, slave))
+			bond_miimon_commit(bond);
+
 		/* Refresh slave-array if applicable!
 		 * If the setup does not use miimon or arpmon (mode-specific!),
 		 * then these events will not cause the slave-array to be