bonding: restrict up state in 802.3ad mode

From: Zhu Yanjun <[email protected]>

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

Signed-off-by Zhu Yanjun <[email protected]>

---
drivers/net/bonding/bond_main.c | 19 +++++++++++++++++++

1 file changed, 19 insertions(+)

drivers/net/bonding/bond_main.c +8 -0

index 9e0f8a7..0a80fb3 100644
@@ -1991,6 +1991,25 @@ static int bond_miimon_inspect(struct bonding *bond)
 
 		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
+		/* Since some NIC has time span between netif_running and
+		 * getting speed and duples. That is, after a NIC is up (netif_running),
+		 * there is a time span before this NIC is negotiated with speed and duplex.
+		 * During this time span, the slave in 802.3ad is configured without speed
+		 * and duplex. This 802.3ad bonding will not work because it needs slave's speed
+		 * and duplex to generate key field.
+		 * As such, we restrict up in 802.3ad mode to: netif_running && peed != SPEED_UNKNOWN &&
+		 * duplex != DUPLEX_UNKNOWN

drivers/net/bonding/bond_main.c +0 -0

 		switch (slave->link) {
 		case BOND_LINK_UP:
 			if (link_state)

Signature · 1 line

1.7.9.5

From: yzhu1 <[email protected]>

In 802.3ad mode, the speed and duplex is needed. But in some NICs, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.

To make bonding driver robust and compatible with more NICs, it is necessary to delay the up state without speed and duplex in 802.3ad mode.

Signed-off-by yzhu1 <[email protected]>

---
drivers/net/bonding/bond_main.c | 34 ++++++++++++++++++++++++++++++++++

1 file changed, 34 insertions(+)

drivers/net/bonding/bond_main.c +34 -0

index 9e0f8a7..a1d8708 100644
@@ -419,6 +419,35 @@ const char *bond_slave_link_status(s8 link)
 	}
 }
 
+/* This function is to check the speed and duplex of a NIC.
+ * Since the speed and duplex of a slave device are very
+ * important to the bonding in the 802.3ad mode. As such,
+ * it is necessary to check the speed and duplex of a slave
+ * device in 802.3ad mode.
+ *
+ * speed != SPEED_UNKNOWN and duplex == DUPLEX_FULL  :  1
+ *                                           others  :  0
+ */
+static int __check_speed_duplex(struct net_device *netdev)
+{
+	struct ethtool_cmd ecmd;
+	u32 slave_speed = SPEED_UNKNOWN;
+	int res;
+
+	res = __ethtool_get_settings(netdev, &ecmd);
+	if (res < 0)
+		return 0;
+
+	slave_speed = ethtool_cmd_speed(&ecmd);
+	if (slave_speed == 0 || slave_speed == ((__u32) -1))
+		return 0;
+
+	if (DUPLEX_FULL != ecmd.duplex)
+		return 0;
+
+	return 1;
+}
+
 /* if <dev> supports MII link status reporting, check its link status.
 *
 * We either do MII/ETHTOOL ioctls, or check netif_carrier_ok(),

@@ -445,6 +474,11 @@ static int bond_check_dev_link(struct bonding *bond,
 	if (!reporting && !netif_running(slave_dev))
 		return 0;
 
+	/* Check the speed and duplex of the slave device in 802.3ad mode. */
+	if ((BOND_MODE(bond) == BOND_MODE_8023AD) &&
+	   !__check_speed_duplex(slave_dev))
+		return 0;
+
 	if (bond->params.use_carrier)
 		return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
 

Signature · 1 line

1.7.9.5

From: yzhu1 <[email protected]>

In 802.3ad mode, the speed and duplex is needed. But in some NICs, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.

To make bonding driver robust and compatible with more NICs, it is necessary to delay the up state without speed and duplex in 802.3ad mode.

Signed-off-by yzhu1 <[email protected]>

---
  drivers/net/bonding/bond_main.c |   34 ++++++++++++++++++++++++++++++++++
  1 file changed, 34 insertions(+)

drivers/net/bonding/bond_main.c +14 -0

index 9e0f8a7..a1d8708 100644
@@ -419,6 +419,35 @@ const char *bond_slave_link_status(s8 link)
  	}
  }
 
+/* This function is to check the speed and duplex of a NIC.
+ * Since the speed and duplex of a slave device are very
+ * important to the bonding in the 802.3ad mode. As such,
+ * it is necessary to check the speed and duplex of a slave
+ * device in 802.3ad mode.
+ *
+ * speed != SPEED_UNKNOWN and duplex == DUPLEX_FULL  :  1
+ *                                           others  :  0
+ */
+static int __check_speed_duplex(struct net_device *netdev)
+{
+	struct ethtool_cmd ecmd;
+	u32 slave_speed = SPEED_UNKNOWN;
+	int res;

Quoted 7 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet device
can be carrier up but not have the duplex and speed available.

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change() when the duplex and speed are available, or delay calling netif_carrier_on().

Quoted 10 lines · Jay Vosburgh

Quoted 7 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet device
can be carrier up but not have the duplex and speed available.

...

Quoted 7 lines

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change() when the duplex and speed are available, or delay calling netif_carrier_on().

I have encountered this problem (NIC having carrier on before being able to detect speed/duplex and driver not notifying when speed/duplex becomes available) with netxen cards earlier. But it was eventually fixed in the driver by commit 9d01412ae76f ("netxen: Fix link event handling.") so this example rather supports what you said.

Michal Kubecek

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of zhuyj
Sent: Monday, December 28, 2015 1:19 AM
To: Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 25 lines · Michal Kubecek

Quoted 10 lines · Jay Vosburgh

Quoted 7 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet device
can be carrier up but not have the duplex and speed available.

...

Quoted 7 lines

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change() when the duplex and speed are available, or delay calling netif_carrier_on().

I have encountered this problem (NIC having carrier on before being able to detect speed/duplex and driver not notifying when speed/duplex becomes available) with netxen cards earlier. But it was eventually fixed in the driver by commit 9d01412ae76f ("netxen: Fix link event handling.") so this example rather supports what you said.

Quoted 1 line

Michal Kubecek

Thanks a lot.
I checked the commit 9d01412ae76f ("netxen: Fix link event handling."). The symptoms are the same with mine.

The root cause is different. In my problem, the root cause is that LINKS register[] can not provide link_up and link_speed at the same time. There is a time span between link_up and link_speed.

My solution is to force to synchronize link_up and link_speed in ixgbe X540 NIC.

Best Regards!
Zhu Yanjun
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 40 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of zhuyj
Sent: Monday, December 28, 2015 1:19 AM
To: Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 19 lines · Michal Kubecek

Quoted 10 lines · Jay Vosburgh

Quoted 7 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet device
can be carrier up but not have the duplex and speed available.

...

Quoted 7 lines

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change() when the duplex and speed are available, or delay calling netif_carrier_on().

Quoted 7 lines

I have encountered this problem (NIC having carrier on before being able to detect speed/duplex and driver not notifying when speed/duplex becomes available) with netxen cards earlier. But it was eventually fixed in the driver by commit 9d01412ae76f ("netxen: Fix link event handling.") so this example rather supports what you said.

Michal Kubecek

Thanks a lot.
I checked the commit 9d01412ae76f ("netxen: Fix link event handling."). The symptoms are the same with mine.

Quoted 3 lines

The root cause is different. In my problem, the root cause is that LINKS register[] can not provide link_up and link_speed at the same time. There is a time span between link_up and link_speed.

The LINK_UP and LINK_SPEED bits in the LINKS register for ixgbe HW are updated simultaneously. Do you have any proof to show the delay you are referring to as I am sure our HW engineers would like to know about it.

What we have seen in the case of bonding is that with some link partners there may be a rapid link flap (up, down, up) and as result the bonding driver may report the speed as unknown if just so happens that the speed is checked during the period in which the interface is re-negotiating.

Thanks,
Emil

Quoted 8 lines

My solution is to force to synchronize link_up and link_speed in ixgbe X540 NIC.

Best Regards!
Zhu Yanjun
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

-----Original Message-----
From: zhuyj [mailto:[email protected]]
Sent: Tuesday, January 05, 2016 7:05 PM
To: Tantilov, Emil S; Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 2 lines · Emil S

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]

On

Quoted 4 lines

Quoted 4 lines

Behalf Of zhuyj
Sent: Monday, December 28, 2015 1:19 AM
To: Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected];

[email protected];

Quoted 8 lines

Quoted 8 lines

Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 4 lines · Michal Kubecek

Quoted 3 lines · Jay Vosburgh

Quoted 2 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and

duplex.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet device
can be carrier up but not have the duplex and speed available.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

...

Quoted 7 lines

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change() when the duplex and speed are available, or delay calling

netif_carrier_on().

Quoted 1 line

Quoted 1 line

Quoted 1 line

I have encountered this problem (NIC having carrier on before being

able

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

to detect speed/duplex and driver not notifying when speed/duplex becomes available) with netxen cards earlier. But it was eventually fixed in the driver by commit 9d01412ae76f ("netxen: Fix link event handling.") so this example rather supports what you said.

Michal

Kubecek

Quoted 7 lines

Quoted 7 lines

Thanks a lot.
I checked the commit 9d01412ae76f ("netxen: Fix link event handling."). The symptoms are the same with mine.

The root cause is different. In my problem, the root cause is that LINKS register[] can not provide link_up and link_speed at the same time. There is a time span between link_up and link_speed.

Quoted 1 line

The LINK_UP and LINK_SPEED bits in the LINKS register for ixgbe HW are

updated

Quoted 1 line

simultaneously. Do you have any proof to show the delay you are referring

to

Quoted 1 line

as I am sure our HW engineers would like to know about it.

Sorry. I can not reproduce this problem locally. What I have is the feedback from the customer.

Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 100baseT/Full

                           1000baseT/Full
                           10000baseT/Full
   Supported pause frame use: No
   Supports auto-negotiation: Yes
   Advertised link modes:  100baseT/Full
                           1000baseT/Full
                           10000baseT/Full

Advertised pause frame use: No
Advertised auto-negotiation: Yes
Speed: Unknown!
Duplex: Unknown! (255)
Port: Twisted Pair
PHYAD: 0
Transceiver: external
Auto-negotiation: on
MDI-X: Unknown
Supports Wake-on: d
Wake-on: d
Current message level: 0x00000007 (7)
drv probe link
Link detected: yes

Link detected: yes

Quoted 58 lines

-----Original Message-----
From: zhuyj [mailto:[email protected]]
Sent: Tuesday, January 05, 2016 7:05 PM
To: Tantilov, Emil S; Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 2 lines · Emil S

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]

On

Quoted 4 lines

Quoted 4 lines

Behalf Of zhuyj
Sent: Monday, December 28, 2015 1:19 AM
To: Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected];

[email protected];

Quoted 8 lines

Quoted 8 lines

Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 4 lines · Michal Kubecek

Quoted 3 lines · Jay Vosburgh

Quoted 2 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and

duplex.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet device
can be carrier up but not have the duplex and speed available.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

...

Quoted 7 lines

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change() when the duplex and speed are available, or delay calling

netif_carrier_on().

Quoted 1 line

Quoted 1 line

Quoted 1 line

I have encountered this problem (NIC having carrier on before being

able

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

to detect speed/duplex and driver not notifying when speed/duplex becomes available) with netxen cards earlier. But it was eventually fixed in the driver by commit 9d01412ae76f ("netxen: Fix link event handling.") so this example rather supports what you said.

Michal

Kubecek

Quoted 7 lines

Quoted 7 lines

Thanks a lot.
I checked the commit 9d01412ae76f ("netxen: Fix link event handling."). The symptoms are the same with mine.

The root cause is different. In my problem, the root cause is that LINKS register[] can not provide link_up and link_speed at the same time. There is a time span between link_up and link_speed.

Quoted 7 lines

Quoted 1 line

The LINK_UP and LINK_SPEED bits in the LINKS register for ixgbe HW are

updated

Quoted 1 line

simultaneously. Do you have any proof to show the delay you are referring

to

Quoted 1 line

as I am sure our HW engineers would like to know about it.

Sorry. I can not reproduce this problem locally. What I have is the feedback from the customer.

So you are assuming that there is a delay due to the issue you are seeing?

Quoted 17 lines

Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 100baseT/Full
1000baseT/Full
10000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Advertised link modes: 100baseT/Full
1000baseT/Full
10000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Speed: Unknown!
Duplex: Unknown! (255)
Port: Twisted Pair
PHYAD: 0

Quoted 8 lines

Transceiver: external
Auto-negotiation: on
MDI-X: Unknown
Supports Wake-on: d
Wake-on: d
Current message level: 0x00000007 (7)
drv probe link
Link detected: yes

The speed and the link state here are reported from different sources:

Quoted 2 lines

Link detected: yes

Comes from a netif_carrier_ok() check. This is done via ethtool_op_get_link()

Only the speed is reported through the LINKS register - that is why it is reported as "Unknown" - in other words link_up is false.

This is a trace from the case where the bonding driver reports 0 Mbps:

    kworker/u48:1-27950 [010] ....  6493.084916: ixgbe_service_task: eth1: link_speed = 80, link_up = false
    kworker/u48:1-27950 [011] ....  6493.184894: ixgbe_service_task: eth1: link_speed = 80, link_up = false
    kworker/u48:1-27950 [000] ....  6494.439883: ixgbe_service_task: eth1: link_speed = 80, link_up = true
    kworker/u48:1-27950 [000] ....  6494.464204: ixgbe_service_task: eth1: NIC Link is Up 10 Gbps, Flow Control: RX/TX
      kworker/0:2-1926  [000] ....  6494.464249: ixgbe_get_settings: eth1: link_speed = 80, link_up = false
   NetworkManager-3819  [008] ....  6494.464484: ixgbe_get_settings: eth1: link_speed = 80, link_up = false
    kworker/u48:1-27950 [007] ....  6494.496886: bond_mii_monitor: bond0: link status definitely up for interface eth1, 0 Mbps full duplex
   NetworkManager-3819  [008] ....  6494.496967: ixgbe_get_settings: eth1: link_speed = 80, link_up = false
    kworker/u48:1-27950 [008] ....  6495.288798: ixgbe_service_task: eth1: link_speed = 80, link_up = true
    kworker/u48:1-27950 [008] ....  6495.388806: ixgbe_service_task: eth1: link_speed = 80, link_up = true

As you can see the link is initially established, but then lost and if just so happens that the bonding driver is checking it at that time it will report 0 Mbps.

I will give your patch a try and see if it helps in this situation.

Thanks,
Emil

-----Original Message-----
From: zhuyj [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 7:34 PM
To: Tantilov, Emil S; Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 5 lines · Emil S

Quoted 5 lines

-----Original Message-----
From: zhuyj [mailto:[email protected]]
Sent: Tuesday, January 05, 2016 7:05 PM
To: Tantilov, Emil S; Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected];

[email protected];

Quoted 6 lines

Quoted 6 lines

Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 2 lines · Emil S

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:netdev-

[email protected]]

Quoted 14 lines

Quoted 14 lines

On

Quoted 4 lines

Quoted 4 lines

Behalf Of zhuyj
Sent: Monday, December 28, 2015 1:19 AM
To: Michal Kubecek; Jay Vosburgh
Cc: [email protected]; [email protected];

[email protected];

Quoted 8 lines

Quoted 8 lines

Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 4 lines · Michal Kubecek

Quoted 3 lines · Jay Vosburgh

Quoted 2 lines · <[email protected]> wrote:

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and

Quoted 7 lines

Quoted 7 lines

duplex.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

What device is this? It seems a bit odd that an Ethernet

device

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

can be carrier up but not have the duplex and speed available.

...

Quoted 6 lines

In general, though, bonding expects a speed or duplex change to
be announced via a NETDEV_UPDATE or NETDEV_UP notifier, which would propagate to the 802.3ad logic.

If the device here is going carrier up prior to having speed or
duplex available, then maybe it should call netdev_state_change()

when

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the duplex and speed are available, or delay calling

netif_carrier_on().

Quoted 1 line

Quoted 1 line

Quoted 1 line

I have encountered this problem (NIC having carrier on before being

able

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

to detect speed/duplex and driver not notifying when speed/duplex becomes available) with netxen cards earlier. But it was eventually fixed in the driver by commit 9d01412ae76f ("netxen: Fix link event handling.") so this example rather supports what you said.

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Michal

Kubecek

Quoted 5 lines

Quoted 5 lines

Thanks a lot.
I checked the commit 9d01412ae76f ("netxen: Fix link event handling."). The symptoms are the same with mine.

The root cause is different. In my problem, the root cause is that

LINKS

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

register[] can not provide link_up and link_speed at the same time. There is a time span between link_up and link_speed.

The LINK_UP and LINK_SPEED bits in the LINKS register for ixgbe HW are

updated

Quoted 1 line

simultaneously. Do you have any proof to show the delay you are

referring

Quoted 2 lines

Quoted 2 lines

to

Quoted 1 line

as I am sure our HW engineers would like to know about it.

Quoted 3 lines

Quoted 2 lines

Sorry. I can not reproduce this problem locally. What I have is the feedback from the customer.

So you are assuming that there is a delay due to the issue you are

seeing?

Sure. Before I get the further feedback from the customer, I can not make further conclusion.
My patch is based on the feedback from the customer.

From: Zhu Yanjun <[email protected]>

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.
To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

Signed-off-by Zhu Yanjun <[email protected]>

---
  drivers/net/bonding/bond_main.c |   11 +++++++++++
  1 file changed, 11 insertions(+)

drivers/net/bonding/bond_main.c +11 -0

index 09f8a48..7df8af5 100644
@@ -1991,6 +1991,17 @@ static int bond_miimon_inspect(struct bonding *bond)
 
  		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
+		if ((BMSR_LSTATUS == link_state) &&
+		    (BOND_MODE(bond) == BOND_MODE_8023AD)) {
+			rtnl_lock();
+			bond_update_speed_duplex(slave);
+			rtnl_unlock();
+			if ((slave->speed == SPEED_UNKNOWN) ||
+			    (slave->duplex == DUPLEX_UNKNOWN)) {
+				link_state = 0;
+				netdev_info(bond->dev, "In 802.3ad mode, it is not enough to up without speed and duplex");
+			}
+		}
  		switch (slave->link) {
  		case BOND_LINK_UP:
  			if (link_state)

From: Zhu Yanjun <[email protected]>

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.

To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

Signed-off-by Zhu Yanjun <[email protected]>

---
drivers/net/bonding/bond_main.c | 11 +++++++++++

1 file changed, 11 insertions(+)

drivers/net/bonding/bond_main.c +0 -0

index 09f8a48..7df8af5 100644

drivers/net/bonding/bond_main.c +6 -0

+			if ((slave->speed == SPEED_UNKNOWN) ||
+			    (slave->duplex == DUPLEX_UNKNOWN)) {
+				link_state = 0;
+				netdev_info(bond->dev, "In 802.3ad mode, it is not enough to up without speed and duplex");
+			}
+		}
 		switch (slave->link) {
 		case BOND_LINK_UP:
 			if (link_state)

Signature · 1 line

1.7.9.5

-----Original Message-----
From: Jay Vosburgh [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 10:34 PM
To: [email protected]
Cc: Tantilov, Emil S; [email protected]; [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

Quoted 7 lines · <[email protected]> wrote:

From: Zhu Yanjun <[email protected]>

In 802.3ad mode, the speed and duplex is needed. But in some NIC, there is a time span between NIC up state and getting speed and duplex. As such, sometimes a slave in 802.3ad mode is in up state without speed and duplex. This will make bonding in 802.3ad mode can not work well.

From my reading of Emil's comments in the discussion, I'm not
sure the above is an accurate description of the problem. If I'm understanding correctly, the cause is due to link flaps racing with the bonding monitor workqueue polling the state. Is this correct?

Review

Quoted 9 lines

To make bonding driver be compatible with more NICs, it is necessary to restrict the up state in 802.3ad mode.

Signed-off-by Zhu Yanjun <[email protected]>

---
drivers/net/bonding/bond_main.c | 11 +++++++++++

1 file changed, 11 insertions(+)

(quoted diff) +0 -0

b/drivers/net/bonding/bond_main.c

drivers/net/bonding/bond_main.c static int bond_miimon_inspect(struct bonding

Quoted 4 lines

index 09f8a48..7df8af5 100644

drivers/net/bonding/bond_main.c +0 -0

@@ -1991,6 +1991,17 @@ static int bond_miimon_inspect(struct bonding

*bond)

drivers/net/bonding/bond_main.c static int bond_miimon_inspect(struct bonding *bond) · line 1998

Quoted 8 lines

drivers/net/bonding/bond_main.c +5 -0

 		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
+		if ((BMSR_LSTATUS == link_state) &&
+		    (BOND_MODE(bond) == BOND_MODE_8023AD)) {
+			rtnl_lock();
+			bond_update_speed_duplex(slave);
+			rtnl_unlock();

This will add a round trip on the RTNL mutex for every miimon
interval when the slave is carrier up. At common miimon rates (10 - 50 ms), this will hit RTNL between 20 and 100 times per second. I do not see how this is acceptable.

I believe the proper solution here is to supplant the periodic
miimon polling from bonding with link state detection based on notifiers (As Stephen suggested, not for the first time).

My suggestion is to have bonding set slave link state based on
notifiers if miimon is set to zero, and poll as usual if it is not. This would preserve any backwards compatibility with any device out there that might possibly still be doing netif_carrier_on/off incorrectly or not at all. The only minor complication is synchronizing notifier carrier state detection with the ARP monitor.

This should have been done a long time ago; I'll work something
up tomorrow (it's late here right now) and post a patch for testing.

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

drivers/net/bonding/bond_main.c +80 -79

index cab99fd..6fe68b1 100644
@@ -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
 
 
---

-Jay Vosburgh, [email protected]

From 62c5cf3a4aa944bac0397748d3ce1dd28b358f79 Mon Sep 17 00:00:00 2001

Hi, Jay

Thank for your help.
I made a new patch based on the latest linux kernel. Now it is in the attachment.
When I run "make", the following errors will pop up.

drivers/net/bonding/bond_main.c:1996:3: error: too many arguments to function ‘bond_set_slave_link_state’ include/net/bonding.h:507:20: note: declared here

drivers/net/bonding/bond_main.c +0 -1

-			bond_set_slave_link_state(slave, BOND_LINK_FAIL);

And I can not find notifier callbacks in the patch.

From: Zhu Yanjun <[email protected]>

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.

Signed-off-by Zhu Yanjun <[email protected]>

---
  drivers/net/bonding/bond_main.c |  154 ++++++++++++++++++++-------------------
  1 file changed, 78 insertions(+), 76 deletions(-)

drivers/net/bonding/bond_main.c +78 -76

index 9e0f8a7..9a0e69e 100644
@@ -1977,101 +1977,100 @@ 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;
-
-		link_state = bond_check_dev_link(bond, slave->dev, 0);
+	slave->new_link = BOND_LINK_NOCHANGE;
 
-		switch (slave->link) {
-		case BOND_LINK_UP:
-			if (link_state)
-				continue;
+	link_state = bond_check_dev_link(bond, slave->dev, 0);
 
-			bond_set_slave_link_state(slave, BOND_LINK_FAIL);
-			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);
-				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;
-			}
+	switch (slave->link) {
+	case BOND_LINK_UP:
+		if (link_state)
+			return 0;
 
-			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_DOWN;
-				commit++;
-				continue;
-			}
+		bond_set_slave_link_state(slave, BOND_LINK_FAIL);
+		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);
+			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;
+		}
 
-			slave->delay--;
-			break;
+		if (slave->delay <= 0) {
+			slave->new_link = BOND_LINK_DOWN;
+			return 1;
+		}
 
-		case BOND_LINK_DOWN:
-			if (!link_state)
-				continue;
+		slave->delay--;
+		break;
 
-			bond_set_slave_link_state(slave, BOND_LINK_BACK);
-			slave->delay = bond->params.updelay;
+	case BOND_LINK_DOWN:
+		if (!link_state)
+			return 0;
 
-			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);
-				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);
+		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);
+			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 commit;
+	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;
  }
 
  static void bond_miimon_commit(struct bonding *bond)

@@ -2969,6 +2968,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

-----Original Message-----
From: Jay Vosburgh [mailto:[email protected]]
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.

Quoted 1 line

-----Original Message-----

From: Jay Vosburgh [mailto:[email protected]]

Quoted 5 lines

Sent: Thursday, January 07, 2016 5:29 PM
Subject: [RFC PATCH net-next] bonding: Use notifiers for slave link state detection

Quoted 8 lines

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:

[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

Quoted 20 lines · Emil S

Quoted 1 line

-----Original Message-----

From: Jay Vosburgh [mailto:[email protected]]

Quoted 5 lines

Sent: Thursday, January 07, 2016 5:29 PM
Subject: [RFC PATCH net-next] bonding: Use notifiers for slave link state detection

Quoted 8 lines

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:

[...]

Quoted 3 lines

[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

Thanks for testing; the misbehavior is because I cheaped out and
didn't break out the commit function into a "single slave" version. The below patch (against net-next, replacing the original patch) shouldn't generate the erroneous additional link messages any more.

This does generate an RCU warning, although the code actually is
safe (since the notifier callback holds RTNL); I'll sort that out next week.

	-J

drivers/net/bonding/bond_main.c +153 -148

index cab99fd..12dd533 100644
@@ -2012,203 +2012,206 @@ 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 commit;
+	return 0;
  }
 
-static void bond_miimon_commit(struct bonding *bond)
+static int bond_miimon_inspect(struct bonding *bond)
  {
  	struct list_head *iter;
-	struct slave *slave, *primary;
+	struct slave *slave;
+	int commit = 0;
 
-	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
-		case BOND_LINK_NOCHANGE:
-			continue;
+	bond_for_each_slave_rcu(bond, slave, iter)
+		commit += bond_miimon_inspect_slave(bond, slave);
 
-		case BOND_LINK_UP:
-			bond_set_slave_link_state(slave, BOND_LINK_UP,
-						  BOND_SLAVE_NOTIFY_NOW);
-			slave->last_link_up = jiffies;
+	return commit;
+}
 
-			primary = rtnl_dereference(bond->primary_slave);
-			if (BOND_MODE(bond) == BOND_MODE_8023AD) {
-				/* prevent it from being the active one */
-				bond_set_backup_slave(slave);
-			} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
-				/* make it immediately active */
-				bond_set_active_slave(slave);
-			} else if (slave != primary) {
-				/* prevent it from being the active one */
-				bond_set_backup_slave(slave);
-			}
+static void bond_miimon_commit_slave(struct bonding *bond, struct slave *slave)
+{
+	struct slave *primary;
 
-			netdev_info(bond->dev, "link status definitely up for interface %s, %u Mbps %s duplex\n",
-				    slave->dev->name,
-				    slave->speed == SPEED_UNKNOWN ? 0 : slave->speed,
-				    slave->duplex ? "full" : "half");
+	switch (slave->new_link) {
+	case BOND_LINK_NOCHANGE:
+		return;
 
-			/* notify ad that the link status has changed */
-			if (BOND_MODE(bond) == BOND_MODE_8023AD)
-				bond_3ad_handle_link_change(slave, BOND_LINK_UP);
+	case BOND_LINK_UP:
+		bond_set_slave_link_state(slave, BOND_LINK_UP,
+					  BOND_SLAVE_NOTIFY_NOW);
+		slave->last_link_up = jiffies;
 
-			if (bond_is_lb(bond))
-				bond_alb_handle_link_change(bond, slave,
-							    BOND_LINK_UP);
+		primary = rtnl_dereference(bond->primary_slave);
+		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
+			/* prevent it from being the active one */
+			bond_set_backup_slave(slave);
+		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+			/* make it immediately active */
+			bond_set_active_slave(slave);
+		} else if (slave != primary) {
+			/* prevent it from being the active one */
+			bond_set_backup_slave(slave);
+		}
 
-			if (BOND_MODE(bond) == BOND_MODE_XOR)
-				bond_update_slave_arr(bond, NULL);
+		netdev_info(bond->dev, "link status definitely up for interface %s, %u Mbps %s duplex\n",
+			    slave->dev->name,
+			    slave->speed == SPEED_UNKNOWN ? 0 : slave->speed,
+			    slave->duplex ? "full" : "half");
 
-			if (!bond->curr_active_slave || slave == primary)
-				goto do_failover;
+		/* notify ad that the link status has changed */
+		if (BOND_MODE(bond) == BOND_MODE_8023AD)
+			bond_3ad_handle_link_change(slave, BOND_LINK_UP);
 
-			continue;
+		if (bond_is_lb(bond))
+			bond_alb_handle_link_change(bond, slave, BOND_LINK_UP);
 
-		case BOND_LINK_DOWN:
-			if (slave->link_failure_count < UINT_MAX)
-				slave->link_failure_count++;
+		if (BOND_MODE(bond) == BOND_MODE_XOR)
+			bond_update_slave_arr(bond, NULL);
 
-			bond_set_slave_link_state(slave, BOND_LINK_DOWN,
-						  BOND_SLAVE_NOTIFY_NOW);
+		if (!bond->curr_active_slave || slave == primary)
+			goto do_failover;
 
-			if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP ||
-			    BOND_MODE(bond) == BOND_MODE_8023AD)
-				bond_set_slave_inactive_flags(slave,
-							      BOND_SLAVE_NOTIFY_NOW);
+		goto out;
 
-			netdev_info(bond->dev, "link status definitely down for interface %s, disabling it\n",
-				    slave->dev->name);
+	case BOND_LINK_DOWN:
+		if (slave->link_failure_count < UINT_MAX)
+			slave->link_failure_count++;
 
-			if (BOND_MODE(bond) == BOND_MODE_8023AD)
-				bond_3ad_handle_link_change(slave,
-							    BOND_LINK_DOWN);
+		bond_set_slave_link_state(slave, BOND_LINK_DOWN,
+					  BOND_SLAVE_NOTIFY_NOW);
 
-			if (bond_is_lb(bond))
-				bond_alb_handle_link_change(bond, slave,
-							    BOND_LINK_DOWN);
+		if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP ||
+		    BOND_MODE(bond) == BOND_MODE_8023AD)
+			bond_set_slave_inactive_flags(slave,
+						      BOND_SLAVE_NOTIFY_NOW);
 
-			if (BOND_MODE(bond) == BOND_MODE_XOR)
-				bond_update_slave_arr(bond, NULL);
+		netdev_info(bond->dev, "link status definitely down for interface %s, disabling it\n",
+			    slave->dev->name);
 
-			if (slave == rcu_access_pointer(bond->curr_active_slave))
-				goto do_failover;
+		if (BOND_MODE(bond) == BOND_MODE_8023AD)
+			bond_3ad_handle_link_change(slave, BOND_LINK_DOWN);
 
-			continue;
+		if (bond_is_lb(bond))
+			bond_alb_handle_link_change(bond, slave, BOND_LINK_DOWN);
 
-		default:
-			netdev_err(bond->dev, "invalid new link %d on slave %s\n",
-				   slave->new_link, slave->dev->name);
-			slave->new_link = BOND_LINK_NOCHANGE;
+		if (BOND_MODE(bond) == BOND_MODE_XOR)
+			bond_update_slave_arr(bond, NULL);
 
-			continue;
-		}
+		if (slave == rcu_access_pointer(bond->curr_active_slave))
+			goto do_failover;
 
-do_failover:
-		block_netpoll_tx();
-		bond_select_active_slave(bond);
-		unblock_netpoll_tx();
+		goto out;
+
+	default:
+		netdev_err(bond->dev, "invalid new link %d on slave %s\n",
+			   slave->new_link, slave->dev->name);
+		slave->new_link = BOND_LINK_NOCHANGE;
+
+		goto out;
  	}
 
+do_failover:
+	block_netpoll_tx();
+	bond_select_active_slave(bond);
+	unblock_netpoll_tx();
+
+out:
  	bond_set_carrier(bond);
  }
 
+static void bond_miimon_commit(struct bonding *bond)
+{
+	struct list_head *iter;
+	struct slave *slave;
+
+	bond_for_each_slave(bond, slave, iter)
+		bond_miimon_commit_slave(bond, slave);
+}
+
  /* bond_mii_monitor
   *
   * Really a wrapper that splits the mii monitor into two phases: an

@@ -3016,6 +3019,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_slave(bond, slave);
+
  		/* 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
 
 
---

-Jay Vosburgh, [email protected]

Hi, Jay && Emil

I delved into the source code. This patch is based on notifiers. When a NETDEV_UP notifier is received in bond_slave_netdev_event, in bond_miimon_inspect_slave, bond_check_dev_link is called to detect link_state.

Because of link flap, link_state is sometimes different from NETDEV_UP. That is, though event is NETDEV_UP, sometime link_state is down because of link flap.

In the following patch, if link_state is different from the event, it is unnecessary to make further setup.

drivers/net/bonding/bond_main.c +0 -0

index 12dd533..1b53da0 100644
@@ -2012,7 +2012,8 @@ 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_slave(struct bonding *bond, struct slave *slave)
+static int bond_miimon_inspect_slave(struct bonding *bond, struct slave *slave,

drivers/net/bonding/bond_main.c +12 -0

+                                    unsigned long event)
 {
        int link_state;
        bool ignore_updelay;

@@ -2022,6 +2023,17 @@ static int bond_miimon_inspect_slave(struct 
 bonding *bond, struct slave *slave)
        slave->new_link = BOND_LINK_NOCHANGE;
 
        link_state = bond_check_dev_link(bond, slave->dev, 0);
+       switch (event) {
+       case NETDEV_UP:
+               if (!link_state)
+                       return 0;
+               break;
+
+       case NETDEV_DOWN:
+               if (link_state)
+                       return 0;
+               break;
+       }
 
        switch (slave->link) {

case BOND_LINK_UP:

drivers/net/bonding/bond_main.c +1 -1

@@ -2107,7 +2119,7 @@ static int bond_miimon_inspect(struct bonding 
 *bond)
        int commit = 0;
 
        bond_for_each_slave_rcu(bond, slave, iter)
-               commit += bond_miimon_inspect_slave(bond, slave);
+               commit += bond_miimon_inspect_slave(bond, slave, 0xFF);
 
        return commit;

}

drivers/net/bonding/bond_main.c +1 -1

@@ -3019,7 +3031,7 @@ 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))
+               if (bond_miimon_inspect_slave(bond, slave, event))
                        bond_miimon_commit_slave(bond, slave);
 

                /* Refresh slave-array if applicable!

Best Regards!
Zhu Yanjun

Quoted 390 lines · Jay Vosburgh

Quoted 20 lines · Emil S

Quoted 1 line

-----Original Message-----

From: Jay Vosburgh [mailto:[email protected]]

Quoted 5 lines

Sent: Thursday, January 07, 2016 5:29 PM
Subject: [RFC PATCH net-next] bonding: Use notifiers for slave link state detection

Quoted 8 lines

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:

[...]

Quoted 3 lines

[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

Thanks for testing; the misbehavior is because I cheaped out and
didn't break out the commit function into a "single slave" version. The below patch (against net-next, replacing the original patch) shouldn't generate the erroneous additional link messages any more.

This does generate an RCU warning, although the code actually is
safe (since the notifier callback holds RTNL); I'll sort that out next week.

    -J

drivers/net/bonding/bond_main.c +0 -0

index cab99fd..12dd533 100644
@@ -2012,203 +2012,206 @@ static int bond_slave_info_query(struct 

net_device *bond_dev, struct ifslave *in
/*-------------------------------- Monitoring
-------------------------------*/

    /* called with rcu_read_lock() */

drivers/net/bonding/bond_main.c +1 -1

-static int bond_miimon_inspect(struct bonding *bond)
+static int bond_miimon_inspect_slave(struct bonding *bond, struct 

slave *slave)
{

drivers/net/bonding/bond_main.c +12 -7

-    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",

drivers/net/bonding/bond_main.c +11 -3

+                    (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",

drivers/net/bonding/bond_main.c +0 -15

-                        (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",

drivers/net/bonding/bond_main.c +2 -5

-                        (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",

drivers/net/bonding/bond_main.c +13 -12

+                    (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",

drivers/net/bonding/bond_main.c +0 -12

-                        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",

drivers/net/bonding/bond_main.c +5 -4

-                        (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",

drivers/net/bonding/bond_main.c +9 -0

+                    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",

drivers/net/bonding/bond_main.c +21 -24

+                    (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 commit;
+    return 0;
  }
  -static void bond_miimon_commit(struct bonding *bond)
+static int bond_miimon_inspect(struct bonding *bond)
  {
      struct list_head *iter;
-    struct slave *slave, *primary;
+    struct slave *slave;
+    int commit = 0;
  -    bond_for_each_slave(bond, slave, iter) {
-        switch (slave->new_link) {
-        case BOND_LINK_NOCHANGE:
-            continue;
+    bond_for_each_slave_rcu(bond, slave, iter)
+        commit += bond_miimon_inspect_slave(bond, slave);
  -        case BOND_LINK_UP:
-            bond_set_slave_link_state(slave, BOND_LINK_UP,
-                          BOND_SLAVE_NOTIFY_NOW);
-            slave->last_link_up = jiffies;
+    return commit;
+}
  -            primary = rtnl_dereference(bond->primary_slave);
-            if (BOND_MODE(bond) == BOND_MODE_8023AD) {
-                /* prevent it from being the active one */
-                bond_set_backup_slave(slave);
-            } else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
-                /* make it immediately active */
-                bond_set_active_slave(slave);
-            } else if (slave != primary) {
-                /* prevent it from being the active one */
-                bond_set_backup_slave(slave);
-            }
+static void bond_miimon_commit_slave(struct bonding *bond, struct 

slave *slave)

drivers/net/bonding/bond_main.c +2 -0

+{
+    struct slave *primary;
  -            netdev_info(bond->dev, "link status definitely up for 

interface %s, %u Mbps %s duplex\n",

drivers/net/bonding/bond_main.c +19 -8

-                    slave->dev->name,
-                    slave->speed == SPEED_UNKNOWN ? 0 : slave->speed,
-                    slave->duplex ? "full" : "half");
+    switch (slave->new_link) {
+    case BOND_LINK_NOCHANGE:
+        return;
  -            /* notify ad that the link status has changed */
-            if (BOND_MODE(bond) == BOND_MODE_8023AD)
-                bond_3ad_handle_link_change(slave, BOND_LINK_UP);
+    case BOND_LINK_UP:
+        bond_set_slave_link_state(slave, BOND_LINK_UP,
+                      BOND_SLAVE_NOTIFY_NOW);
+        slave->last_link_up = jiffies;
  -            if (bond_is_lb(bond))
-                bond_alb_handle_link_change(bond, slave,
-                                BOND_LINK_UP);
+        primary = rtnl_dereference(bond->primary_slave);
+        if (BOND_MODE(bond) == BOND_MODE_8023AD) {
+            /* prevent it from being the active one */
+            bond_set_backup_slave(slave);
+        } else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
+            /* make it immediately active */
+            bond_set_active_slave(slave);
+        } else if (slave != primary) {
+            /* prevent it from being the active one */
+            bond_set_backup_slave(slave);
+        }
  -            if (BOND_MODE(bond) == BOND_MODE_XOR)
-                bond_update_slave_arr(bond, NULL);
+        netdev_info(bond->dev, "link status definitely up for 

interface %s, %u Mbps %s duplex\n",

drivers/net/bonding/bond_main.c +13 -7

+                slave->dev->name,
+                slave->speed == SPEED_UNKNOWN ? 0 : slave->speed,
+                slave->duplex ? "full" : "half");
  -            if (!bond->curr_active_slave || slave == primary)
-                goto do_failover;
+        /* notify ad that the link status has changed */
+        if (BOND_MODE(bond) == BOND_MODE_8023AD)
+            bond_3ad_handle_link_change(slave, BOND_LINK_UP);
  -            continue;
+        if (bond_is_lb(bond))
+            bond_alb_handle_link_change(bond, slave, BOND_LINK_UP);
  -        case BOND_LINK_DOWN:
-            if (slave->link_failure_count < UINT_MAX)
-                slave->link_failure_count++;
+        if (BOND_MODE(bond) == BOND_MODE_XOR)
+            bond_update_slave_arr(bond, NULL);
  -            bond_set_slave_link_state(slave, BOND_LINK_DOWN,
-                          BOND_SLAVE_NOTIFY_NOW);
+        if (!bond->curr_active_slave || slave == primary)
+            goto do_failover;
  -            if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP ||
-                BOND_MODE(bond) == BOND_MODE_8023AD)
-                bond_set_slave_inactive_flags(slave,
-                                  BOND_SLAVE_NOTIFY_NOW);
+        goto out;
  -            netdev_info(bond->dev, "link status definitely down 

for interface %s, disabling it\n",

drivers/net/bonding/bond_main.c +10 -6

-                    slave->dev->name);
+    case BOND_LINK_DOWN:
+        if (slave->link_failure_count < UINT_MAX)
+            slave->link_failure_count++;
  -            if (BOND_MODE(bond) == BOND_MODE_8023AD)
-                bond_3ad_handle_link_change(slave,
-                                BOND_LINK_DOWN);
+        bond_set_slave_link_state(slave, BOND_LINK_DOWN,
+                      BOND_SLAVE_NOTIFY_NOW);
  -            if (bond_is_lb(bond))
-                bond_alb_handle_link_change(bond, slave,
-                                BOND_LINK_DOWN);
+        if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP ||
+            BOND_MODE(bond) == BOND_MODE_8023AD)
+            bond_set_slave_inactive_flags(slave,
+                              BOND_SLAVE_NOTIFY_NOW);
  -            if (BOND_MODE(bond) == BOND_MODE_XOR)
-                bond_update_slave_arr(bond, NULL);
+        netdev_info(bond->dev, "link status definitely down for 

interface %s, disabling it\n",

drivers/net/bonding/bond_main.c +33 -8

+                slave->dev->name);
  -            if (slave == rcu_access_pointer(bond->curr_active_slave))
-                goto do_failover;
+        if (BOND_MODE(bond) == BOND_MODE_8023AD)
+            bond_3ad_handle_link_change(slave, BOND_LINK_DOWN);
  -            continue;
+        if (bond_is_lb(bond))
+            bond_alb_handle_link_change(bond, slave, BOND_LINK_DOWN);
  -        default:
-            netdev_err(bond->dev, "invalid new link %d on slave %s\n",
-                   slave->new_link, slave->dev->name);
-            slave->new_link = BOND_LINK_NOCHANGE;
+        if (BOND_MODE(bond) == BOND_MODE_XOR)
+            bond_update_slave_arr(bond, NULL);
  -            continue;
-        }
+        if (slave == rcu_access_pointer(bond->curr_active_slave))
+            goto do_failover;
  -do_failover:
-        block_netpoll_tx();
-        bond_select_active_slave(bond);
-        unblock_netpoll_tx();
+        goto out;
+
+    default:
+        netdev_err(bond->dev, "invalid new link %d on slave %s\n",
+               slave->new_link, slave->dev->name);
+        slave->new_link = BOND_LINK_NOCHANGE;
+
+        goto out;
      }
  +do_failover:
+    block_netpoll_tx();
+    bond_select_active_slave(bond);
+    unblock_netpoll_tx();
+
+out:
      bond_set_carrier(bond);
  }
  +static void bond_miimon_commit(struct bonding *bond)
+{
+    struct list_head *iter;
+    struct slave *slave;
+
+    bond_for_each_slave(bond, slave, iter)
+        bond_miimon_commit_slave(bond, slave);
+}
+
  /* bond_mii_monitor
   *
   * Really a wrapper that splits the mii monitor into two phases: an

@@ -3016,6 +3019,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_slave(bond, slave);
+
          /* 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

---
-Jay Vosburgh, [email protected]

-----Original Message-----
From: Jay Vosburgh [mailto:[email protected]]
Sent: Friday, January 08, 2016 6:20 PM
To: Tantilov, Emil S
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Shteinbock, Boris (Wind River)
Subject: Re: [RFC PATCH net-next] bonding: Use notifiers for slave link state detection

Quoted 21 lines · Emil S

Quoted 1 line

-----Original Message-----

From: Jay Vosburgh [mailto:[email protected]]

Quoted 5 lines

Sent: Thursday, January 07, 2016 5:29 PM
Subject: [RFC PATCH net-next] bonding: Use notifiers for slave link state detection

Quoted 8 lines

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:

[...]

Quoted 1 line

[12985.213752] ixgbe 0000:01:00.0 eth0: NIC Link is Up 10 Gbps, Flow

Control: RX/TX

Quoted 1 line

[12985.213970] bond0: link status definitely up for interface eth0, 10000

Mbps full duplex

Quoted 1 line

[12985.213975] bond0: link status definitely up for interface eth1, 0 Mbps

full duplex

Thanks for testing; the misbehavior is because I cheaped out and
didn't break out the commit function into a "single slave" version. The below patch (against net-next, replacing the original patch) shouldn't generate the erroneous additional link messages any more.

This does generate an RCU warning, although the code actually is
safe (since the notifier callback holds RTNL); I'll sort that out next week.

	-J