[PATCH] bonding/802.3ad: fix slave initialization states race

[PATCH] bonding/802.3ad: fix slave initialization states race

[Aleksei Zakharov]

Once a while, one of 802.3ad slaves fails to initialize and hangs in
BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
link initialization transition states") checks slave->last_link_up. But
link can still hang in weird state.
After physical link comes up it sends first two LACPDU messages and
doesn't work properly after that. It doesn't send or receive LACPDU.
Once it happens, the only message in dmesg is:
bond1: link status up again after 0 ms for interface eth2

This behavior can be reproduced (not every time):
1. Set slave link down
2. Wait for 1-3 seconds
3. Set slave link up

The fix is to check slave->link before setting it to BOND_LINK_FAIL or
BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
BOND_LINK_DOWN.

Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
transition states")
Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 931d9d935686..a28776d8f33f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		if (bond_update_speed_duplex(slave) &&
 		    BOND_MODE(bond) == BOND_MODE_8023AD) {
-			if (slave->last_link_up)
+			if (slave->link == BOND_LINK_UP)
 				slave->link = BOND_LINK_FAIL;
 			else
 				slave->link = BOND_LINK_DOWN;
-- 
2.17.1

Re: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Aleksei Zakharov <zaharov@selectel.ru> wrote:

>Once a while, one of 802.3ad slaves fails to initialize and hangs in
>BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
>link initialization transition states") checks slave->last_link_up. But
>link can still hang in weird state.
>After physical link comes up it sends first two LACPDU messages and
>doesn't work properly after that. It doesn't send or receive LACPDU.
>Once it happens, the only message in dmesg is:
>bond1: link status up again after 0 ms for interface eth2

	I believe this message indicates that the slave entered
BOND_LINK_FAIL state, but downdelay was not set.  The _FAIL state is
really for managing the downdelay expiration, and a slave should not be
in that state (outside of a brief transition entirely within
bond_miimon_inspect) if downdelay is 0.

>This behavior can be reproduced (not every time):
>1. Set slave link down
>2. Wait for 1-3 seconds
>3. Set slave link up
>
>The fix is to check slave->link before setting it to BOND_LINK_FAIL or
>BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
>BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
>BOND_LINK_DOWN.
>
>Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
>transition states")
>Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
>---
> drivers/net/bonding/bond_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 931d9d935686..a28776d8f33f 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
> 		 */
> 		if (bond_update_speed_duplex(slave) &&
> 		    BOND_MODE(bond) == BOND_MODE_8023AD) {
>-			if (slave->last_link_up)
>+			if (slave->link == BOND_LINK_UP)
> 				slave->link = BOND_LINK_FAIL;
> 			else
> 				slave->link = BOND_LINK_DOWN;

	Is the core problem here that slaves are reporting link up, but
returning invalid values for speed and/or duplex?  If so, what network
device are you testing with that is exhibiting this behavior?

	If I'm not mistaken, there have been several iterations of
hackery on this block of code to work around this same problem, and each
time there's some corner case that still doesn't work.

	As Davem asked last time around, is the real problem that device
drivers report carrier up but supply invalid speed and duplex state?

	-J

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

Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Алексей Захаров]

> >Once a while, one of 802.3ad slaves fails to initialize and hangs in
> >BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
> >link initialization transition states") checks slave->last_link_up. But
> >link can still hang in weird state.
> >After physical link comes up it sends first two LACPDU messages and
> >doesn't work properly after that. It doesn't send or receive LACPDU.
> >Once it happens, the only message in dmesg is:
> >bond1: link status up again after 0 ms for interface eth2
>
>         I believe this message indicates that the slave entered
> BOND_LINK_FAIL state, but downdelay was not set.  The _FAIL state is
> really for managing the downdelay expiration, and a slave should not be
> in that state (outside of a brief transition entirely within
> bond_miimon_inspect) if downdelay is 0.
That's true, downdelay was set to 0, we only use updelay 500.
Does it mean, that the bonding driver shouldn't set slave to FAIL
state in this case?

>
> >This behavior can be reproduced (not every time):
> >1. Set slave link down
> >2. Wait for 1-3 seconds
> >3. Set slave link up
> >
> >The fix is to check slave->link before setting it to BOND_LINK_FAIL or
> >BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
> >BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
> >BOND_LINK_DOWN.
> >
> >Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
> >transition states")
> >Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
> >---
> > drivers/net/bonding/bond_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 931d9d935686..a28776d8f33f 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
> >                */
> >               if (bond_update_speed_duplex(slave) &&
> >                   BOND_MODE(bond) == BOND_MODE_8023AD) {
> >-                      if (slave->last_link_up)
> >+                      if (slave->link == BOND_LINK_UP)
> >                               slave->link = BOND_LINK_FAIL;
> >                       else
> >                               slave->link = BOND_LINK_DOWN;
>
>         Is the core problem here that slaves are reporting link up, but
> returning invalid values for speed and/or duplex?  If so, what network
> device are you testing with that is exhibiting this behavior?
That's true, because link becomes FAIL right in this block of code.
We use Mellanox ConnectX-3 Pro nic.

>
>         If I'm not mistaken, there have been several iterations of
> hackery on this block of code to work around this same problem, and each
> time there's some corner case that still doesn't work.
As i can see, commit 4d2c0cda0744 ("bonding: speed/duplex update at
NETDEV_UP event")
introduced BOND_LINK_DOWN state if update speed/duplex failed.

Commit ea53abfab960 ("bonding/802.3ad: fix link_failure_count tracking")
changed DOWN state to FAIL.

Commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
transition states")
implemented different new state for different current states, but it
was based on slave->last_link_up.
In our case slave->last_link_up !=0 when this code runs. But, slave is
not in UP state at the moment. It becomes
FAIL and hangs in this state.
So, it looks like checking if slave is in UP mode is more appropriate
here. At least it works in our case.

There was one more commit 12185dfe4436 ("bonding: Force slave speed
check after link state recovery for 802.3ad")
but it doesn't help in our case.

>
>         As Davem asked last time around, is the real problem that device
> drivers report carrier up but supply invalid speed and duplex state?
Probably, but I'm not quite sure right now. We didn't face this issue
before 4d2c0cda0744 and ea53abfab960
commits.

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



--
Best Regards,
Aleksei Zakharov
System administrator

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Алексей Захаров wrote:

>> >Once a while, one of 802.3ad slaves fails to initialize and hangs in
>> >BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
>> >link initialization transition states") checks slave->last_link_up. But
>> >link can still hang in weird state.
>> >After physical link comes up it sends first two LACPDU messages and
>> >doesn't work properly after that. It doesn't send or receive LACPDU.
>> >Once it happens, the only message in dmesg is:
>> >bond1: link status up again after 0 ms for interface eth2
>>
>>         I believe this message indicates that the slave entered
>> BOND_LINK_FAIL state, but downdelay was not set.  The _FAIL state is
>> really for managing the downdelay expiration, and a slave should not be
>> in that state (outside of a brief transition entirely within
>> bond_miimon_inspect) if downdelay is 0.
>That's true, downdelay was set to 0, we only use updelay 500.
>Does it mean, that the bonding driver shouldn't set slave to FAIL
>state in this case?

	It really shouldn't change the slave->link outside of the
monitoring functions at all, because there are side effects that are not
happening (user space notifications, updelay / downdelay, etc).

>> >This behavior can be reproduced (not every time):
>> >1. Set slave link down
>> >2. Wait for 1-3 seconds
>> >3. Set slave link up
>> >
>> >The fix is to check slave->link before setting it to BOND_LINK_FAIL or
>> >BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
>> >BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
>> >BOND_LINK_DOWN.
>> >
>> >Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
>> >transition states")
>> >Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
>> >---
>> > drivers/net/bonding/bond_main.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index 931d9d935686..a28776d8f33f 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
>> >                */
>> >               if (bond_update_speed_duplex(slave) &&
>> >                   BOND_MODE(bond) == BOND_MODE_8023AD) {
>> >-                      if (slave->last_link_up)
>> >+                      if (slave->link == BOND_LINK_UP)
>> >                               slave->link = BOND_LINK_FAIL;
>> >                       else
>> >                               slave->link = BOND_LINK_DOWN;
>>
>>         Is the core problem here that slaves are reporting link up, but
>> returning invalid values for speed and/or duplex?  If so, what network
>> device are you testing with that is exhibiting this behavior?
>That's true, because link becomes FAIL right in this block of code.
>We use Mellanox ConnectX-3 Pro nic.
>
>>
>>         If I'm not mistaken, there have been several iterations of
>> hackery on this block of code to work around this same problem, and each
>> time there's some corner case that still doesn't work.
>As i can see, commit 4d2c0cda0744 ("bonding: speed/duplex update at
>NETDEV_UP event")
>introduced BOND_LINK_DOWN state if update speed/duplex failed.
>
>Commit ea53abfab960 ("bonding/802.3ad: fix link_failure_count tracking")
>changed DOWN state to FAIL.
>
>Commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
>transition states")
>implemented different new state for different current states, but it
>was based on slave->last_link_up.
>In our case slave->last_link_up !=0 when this code runs. But, slave is
>not in UP state at the moment. It becomes
>FAIL and hangs in this state.
>So, it looks like checking if slave is in UP mode is more appropriate
>here. At least it works in our case.
>
>There was one more commit 12185dfe4436 ("bonding: Force slave speed
>check after link state recovery for 802.3ad")
>but it doesn't help in our case.
>
>>
>>         As Davem asked last time around, is the real problem that device
>> drivers report carrier up but supply invalid speed and duplex state?
>Probably, but I'm not quite sure right now. We didn't face this issue
>before 4d2c0cda0744 and ea53abfab960
>commits.

	My concern here is that we keep adding special cases to this
code apparently without really understanding the root cause of the
failures.  4d2c0cda0744 asserts that there is a problem that drivers are
not supplying speed and duplex information at NETDEV_UP time, but is not
specific as to the details (hardware information).  Before we add
another change, I would like to understand what the actual underlying
cause of the failure is, and if yours is somehow different from what
4d2c0cda0744 or ea53abfab960 were fixing (or trying to fix).

	Would it be possible for you to instrument the code here to dump
out the duplex/speed failure information and carrier state of the slave
device at this point when it fails in your testing?  Something like the
following (which I have not compile tested):

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 931d9d935686..758af8c2b9e1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -378,15 +378,22 @@ static int bond_update_speed_duplex(struct slave *slave)
 	slave->duplex = DUPLEX_UNKNOWN;
 
 	res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
-	if (res < 0)
+	if (res < 0) {
+		pr_err("DBG ksettings res %d slave %s\n", res, slave_dev->name);
 		return 1;
-	if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
+	}
+	if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1)) {
+		pr_err("DBG speed %u slave %s\n", ecmd.base.speed,
+		       slave_dev->name);
 		return 1;
+	}
 	switch (ecmd.base.duplex) {
 	case DUPLEX_FULL:
 	case DUPLEX_HALF:
 		break;
 	default:
+		pr_err("DBG duplex %u slave %s\n", ecmd.base.duplex,
+		       slave_dev->name);
 		return 1;
 	}
 
@@ -3135,6 +3142,9 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		if (bond_update_speed_duplex(slave) &&
 		    BOND_MODE(bond) == BOND_MODE_8023AD) {
+			pr_err("DBG slave %s event %d carrier %d\n",
+			       slave->dev->name, event,
+			       netif_carrier_ok(slave->dev));
 			if (slave->last_link_up)
 				slave->link = BOND_LINK_FAIL;
 			else

	Thanks,

	-J

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

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Алексей Захаров]

чт, 19 сент. 2019 г. в 11:00, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Алексей Захаров wrote:
>
> >> >Once a while, one of 802.3ad slaves fails to initialize and hangs in
> >> >BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
> >> >link initialization transition states") checks slave->last_link_up. But
> >> >link can still hang in weird state.
> >> >After physical link comes up it sends first two LACPDU messages and
> >> >doesn't work properly after that. It doesn't send or receive LACPDU.
> >> >Once it happens, the only message in dmesg is:
> >> >bond1: link status up again after 0 ms for interface eth2
> >>
> >>         I believe this message indicates that the slave entered
> >> BOND_LINK_FAIL state, but downdelay was not set.  The _FAIL state is
> >> really for managing the downdelay expiration, and a slave should not be
> >> in that state (outside of a brief transition entirely within
> >> bond_miimon_inspect) if downdelay is 0.
> >That's true, downdelay was set to 0, we only use updelay 500.
> >Does it mean, that the bonding driver shouldn't set slave to FAIL
> >state in this case?
>
>         It really shouldn't change the slave->link outside of the
> monitoring functions at all, because there are side effects that are not
> happening (user space notifications, updelay / downdelay, etc).
>
> >> >This behavior can be reproduced (not every time):
> >> >1. Set slave link down
> >> >2. Wait for 1-3 seconds
> >> >3. Set slave link up
> >> >
> >> >The fix is to check slave->link before setting it to BOND_LINK_FAIL or
> >> >BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
> >> >BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
> >> >BOND_LINK_DOWN.
> >> >
> >> >Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
> >> >transition states")
> >> >Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
> >> >---
> >> > drivers/net/bonding/bond_main.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >> >index 931d9d935686..a28776d8f33f 100644
> >> >--- a/drivers/net/bonding/bond_main.c
> >> >+++ b/drivers/net/bonding/bond_main.c
> >> >@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
> >> >                */
> >> >               if (bond_update_speed_duplex(slave) &&
> >> >                   BOND_MODE(bond) == BOND_MODE_8023AD) {
> >> >-                      if (slave->last_link_up)
> >> >+                      if (slave->link == BOND_LINK_UP)
> >> >                               slave->link = BOND_LINK_FAIL;
> >> >                       else
> >> >                               slave->link = BOND_LINK_DOWN;
> >>
> >>         Is the core problem here that slaves are reporting link up, but
> >> returning invalid values for speed and/or duplex?  If so, what network
> >> device are you testing with that is exhibiting this behavior?
> >That's true, because link becomes FAIL right in this block of code.
> >We use Mellanox ConnectX-3 Pro nic.
> >
> >>
> >>         If I'm not mistaken, there have been several iterations of
> >> hackery on this block of code to work around this same problem, and each
> >> time there's some corner case that still doesn't work.
> >As i can see, commit 4d2c0cda0744 ("bonding: speed/duplex update at
> >NETDEV_UP event")
> >introduced BOND_LINK_DOWN state if update speed/duplex failed.
> >
> >Commit ea53abfab960 ("bonding/802.3ad: fix link_failure_count tracking")
> >changed DOWN state to FAIL.
> >
> >Commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
> >transition states")
> >implemented different new state for different current states, but it
> >was based on slave->last_link_up.
> >In our case slave->last_link_up !=0 when this code runs. But, slave is
> >not in UP state at the moment. It becomes
> >FAIL and hangs in this state.
> >So, it looks like checking if slave is in UP mode is more appropriate
> >here. At least it works in our case.
> >
> >There was one more commit 12185dfe4436 ("bonding: Force slave speed
> >check after link state recovery for 802.3ad")
> >but it doesn't help in our case.
> >
> >>
> >>         As Davem asked last time around, is the real problem that device
> >> drivers report carrier up but supply invalid speed and duplex state?
> >Probably, but I'm not quite sure right now. We didn't face this issue
> >before 4d2c0cda0744 and ea53abfab960
> >commits.
>
>         My concern here is that we keep adding special cases to this
> code apparently without really understanding the root cause of the
> failures.  4d2c0cda0744 asserts that there is a problem that drivers are
> not supplying speed and duplex information at NETDEV_UP time, but is not
> specific as to the details (hardware information).  Before we add
> another change, I would like to understand what the actual underlying
> cause of the failure is, and if yours is somehow different from what
> 4d2c0cda0744 or ea53abfab960 were fixing (or trying to fix).
>
>         Would it be possible for you to instrument the code here to dump
> out the duplex/speed failure information and carrier state of the slave
> device at this point when it fails in your testing?  Something like the
> following (which I have not compile tested):
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 931d9d935686..758af8c2b9e1 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -378,15 +378,22 @@ static int bond_update_speed_duplex(struct slave *slave)
>         slave->duplex = DUPLEX_UNKNOWN;
>
>         res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
> -       if (res < 0)
> +       if (res < 0) {
> +               pr_err("DBG ksettings res %d slave %s\n", res, slave_dev->name);
>                 return 1;
> -       if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
> +       }
> +       if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1)) {
> +               pr_err("DBG speed %u slave %s\n", ecmd.base.speed,
> +                      slave_dev->name);
>                 return 1;
> +       }
>         switch (ecmd.base.duplex) {
>         case DUPLEX_FULL:
>         case DUPLEX_HALF:
>                 break;
>         default:
> +               pr_err("DBG duplex %u slave %s\n", ecmd.base.duplex,
> +                      slave_dev->name);
>                 return 1;
>         }
>
> @@ -3135,6 +3142,9 @@ static int bond_slave_netdev_event(unsigned long event,
>                  */
>                 if (bond_update_speed_duplex(slave) &&
>                     BOND_MODE(bond) == BOND_MODE_8023AD) {
> +                       pr_err("DBG slave %s event %d carrier %d\n",
> +                              slave->dev->name, event,
> +                              netif_carrier_ok(slave->dev));
>                         if (slave->last_link_up)
>                                 slave->link = BOND_LINK_FAIL;
>                         else

Thanks, did that, without my patch. Here is the output when link doesn't work.
Host has actor port state 71 and partner port state 1:
[Thu Sep 19 12:14:04 2019] mlx4_en: eth2: Steering Mode 1
[Thu Sep 19 12:14:04 2019] DBG speed 4294967295 slave eth2
[Thu Sep 19 12:14:04 2019] DBG slave eth2 event 1 carrier 0
[Thu Sep 19 12:14:04 2019] 8021q: adding VLAN 0 to HW filter on device eth2
[Thu Sep 19 12:14:04 2019] mlx4_en: eth2: Link Up
[Thu Sep 19 12:14:04 2019] bond-san: link status up again after 0 ms
for interface eth2

Here is the output when everything works fine:
[Thu Sep 19 12:15:40 2019] mlx4_en: eth2: Steering Mode 1
[Thu Sep 19 12:15:40 2019] DBG speed 4294967295 slave eth2
[Thu Sep 19 12:15:40 2019] DBG slave eth2 event 1 carrier 0
[Thu Sep 19 12:15:40 2019] 8021q: adding VLAN 0 to HW filter on device eth2
[Thu Sep 19 12:15:40 2019] bond-san: link status definitely down for
interface eth2, disabling it
[Thu Sep 19 12:15:40 2019] mlx4_en: eth2: Link Up
[Thu Sep 19 12:15:40 2019] bond-san: link status up for interface
eth2, enabling it in 500 ms
[Thu Sep 19 12:15:41 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex

If I'm not mistaken, there's up event before carrier is up.


-- 
Best Regards,
Aleksei Zakharov
System administrator

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Алексей Захаров wrote:

>чт, 19 сент. 2019 г. в 11:00, Jay Vosburgh <jay.vosburgh@canonical.com>:
>>
>> Алексей Захаров wrote:
>>
>> >> >Once a while, one of 802.3ad slaves fails to initialize and hangs in
>> >> >BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
>> >> >link initialization transition states") checks slave->last_link_up. But
>> >> >link can still hang in weird state.
>> >> >After physical link comes up it sends first two LACPDU messages and
>> >> >doesn't work properly after that. It doesn't send or receive LACPDU.
>> >> >Once it happens, the only message in dmesg is:
>> >> >bond1: link status up again after 0 ms for interface eth2
>> >>
>> >>         I believe this message indicates that the slave entered
>> >> BOND_LINK_FAIL state, but downdelay was not set.  The _FAIL state is
>> >> really for managing the downdelay expiration, and a slave should not be
>> >> in that state (outside of a brief transition entirely within
>> >> bond_miimon_inspect) if downdelay is 0.
>> >That's true, downdelay was set to 0, we only use updelay 500.
>> >Does it mean, that the bonding driver shouldn't set slave to FAIL
>> >state in this case?
>>
>>         It really shouldn't change the slave->link outside of the
>> monitoring functions at all, because there are side effects that are not
>> happening (user space notifications, updelay / downdelay, etc).
>>
>> >> >This behavior can be reproduced (not every time):
>> >> >1. Set slave link down
>> >> >2. Wait for 1-3 seconds
>> >> >3. Set slave link up
>> >> >
>> >> >The fix is to check slave->link before setting it to BOND_LINK_FAIL or
>> >> >BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
>> >> >BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
>> >> >BOND_LINK_DOWN.
>> >> >
>> >> >Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
>> >> >transition states")
>> >> >Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
>> >> >---
>> >> > drivers/net/bonding/bond_main.c | 2 +-
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >> >index 931d9d935686..a28776d8f33f 100644
>> >> >--- a/drivers/net/bonding/bond_main.c
>> >> >+++ b/drivers/net/bonding/bond_main.c
>> >> >@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
>> >> >                */
>> >> >               if (bond_update_speed_duplex(slave) &&
>> >> >                   BOND_MODE(bond) == BOND_MODE_8023AD) {
>> >> >-                      if (slave->last_link_up)
>> >> >+                      if (slave->link == BOND_LINK_UP)
>> >> >                               slave->link = BOND_LINK_FAIL;
>> >> >                       else
>> >> >                               slave->link = BOND_LINK_DOWN;
>> >>
>> >>         Is the core problem here that slaves are reporting link up, but
>> >> returning invalid values for speed and/or duplex?  If so, what network
>> >> device are you testing with that is exhibiting this behavior?
>> >That's true, because link becomes FAIL right in this block of code.
>> >We use Mellanox ConnectX-3 Pro nic.
>> >
>> >>
>> >>         If I'm not mistaken, there have been several iterations of
>> >> hackery on this block of code to work around this same problem, and each
>> >> time there's some corner case that still doesn't work.
>> >As i can see, commit 4d2c0cda0744 ("bonding: speed/duplex update at
>> >NETDEV_UP event")
>> >introduced BOND_LINK_DOWN state if update speed/duplex failed.
>> >
>> >Commit ea53abfab960 ("bonding/802.3ad: fix link_failure_count tracking")
>> >changed DOWN state to FAIL.
>> >
>> >Commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
>> >transition states")
>> >implemented different new state for different current states, but it
>> >was based on slave->last_link_up.
>> >In our case slave->last_link_up !=0 when this code runs. But, slave is
>> >not in UP state at the moment. It becomes
>> >FAIL and hangs in this state.
>> >So, it looks like checking if slave is in UP mode is more appropriate
>> >here. At least it works in our case.
>> >
>> >There was one more commit 12185dfe4436 ("bonding: Force slave speed
>> >check after link state recovery for 802.3ad")
>> >but it doesn't help in our case.
>> >
>> >>
>> >>         As Davem asked last time around, is the real problem that device
>> >> drivers report carrier up but supply invalid speed and duplex state?
>> >Probably, but I'm not quite sure right now. We didn't face this issue
>> >before 4d2c0cda0744 and ea53abfab960
>> >commits.
>>
>>         My concern here is that we keep adding special cases to this
>> code apparently without really understanding the root cause of the
>> failures.  4d2c0cda0744 asserts that there is a problem that drivers are
>> not supplying speed and duplex information at NETDEV_UP time, but is not
>> specific as to the details (hardware information).  Before we add
>> another change, I would like to understand what the actual underlying
>> cause of the failure is, and if yours is somehow different from what
>> 4d2c0cda0744 or ea53abfab960 were fixing (or trying to fix).
>>
>>         Would it be possible for you to instrument the code here to dump
>> out the duplex/speed failure information and carrier state of the slave
>> device at this point when it fails in your testing?  Something like the
>> following (which I have not compile tested):
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 931d9d935686..758af8c2b9e1 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -378,15 +378,22 @@ static int bond_update_speed_duplex(struct slave *slave)
>>         slave->duplex = DUPLEX_UNKNOWN;
>>
>>         res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
>> -       if (res < 0)
>> +       if (res < 0) {
>> +               pr_err("DBG ksettings res %d slave %s\n", res, slave_dev->name);
>>                 return 1;
>> -       if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
>> +       }
>> +       if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1)) {
>> +               pr_err("DBG speed %u slave %s\n", ecmd.base.speed,
>> +                      slave_dev->name);
>>                 return 1;
>> +       }
>>         switch (ecmd.base.duplex) {
>>         case DUPLEX_FULL:
>>         case DUPLEX_HALF:
>>                 break;
>>         default:
>> +               pr_err("DBG duplex %u slave %s\n", ecmd.base.duplex,
>> +                      slave_dev->name);
>>                 return 1;
>>         }
>>
>> @@ -3135,6 +3142,9 @@ static int bond_slave_netdev_event(unsigned long event,
>>                  */
>>                 if (bond_update_speed_duplex(slave) &&
>>                     BOND_MODE(bond) == BOND_MODE_8023AD) {
>> +                       pr_err("DBG slave %s event %d carrier %d\n",
>> +                              slave->dev->name, event,
>> +                              netif_carrier_ok(slave->dev));
>>                         if (slave->last_link_up)
>>                                 slave->link = BOND_LINK_FAIL;
>>                         else
>
>Thanks, did that, without my patch. Here is the output when link doesn't work.
>Host has actor port state 71 and partner port state 1:
>[Thu Sep 19 12:14:04 2019] mlx4_en: eth2: Steering Mode 1
>[Thu Sep 19 12:14:04 2019] DBG speed 4294967295 slave eth2
>[Thu Sep 19 12:14:04 2019] DBG slave eth2 event 1 carrier 0
>[Thu Sep 19 12:14:04 2019] 8021q: adding VLAN 0 to HW filter on device eth2
>[Thu Sep 19 12:14:04 2019] mlx4_en: eth2: Link Up
>[Thu Sep 19 12:14:04 2019] bond-san: link status up again after 0 ms
>for interface eth2
>
>Here is the output when everything works fine:
>[Thu Sep 19 12:15:40 2019] mlx4_en: eth2: Steering Mode 1
>[Thu Sep 19 12:15:40 2019] DBG speed 4294967295 slave eth2
>[Thu Sep 19 12:15:40 2019] DBG slave eth2 event 1 carrier 0
>[Thu Sep 19 12:15:40 2019] 8021q: adding VLAN 0 to HW filter on device eth2
>[Thu Sep 19 12:15:40 2019] bond-san: link status definitely down for
>interface eth2, disabling it
>[Thu Sep 19 12:15:40 2019] mlx4_en: eth2: Link Up
>[Thu Sep 19 12:15:40 2019] bond-san: link status up for interface
>eth2, enabling it in 500 ms
>[Thu Sep 19 12:15:41 2019] bond-san: link status definitely up for
>interface eth2, 10000 Mbps full duplex
>
>If I'm not mistaken, there's up event before carrier is up.

	Yes; the NETDEV_UP is presumably coming from dev_open(), which
makes the device administratively up.  This is discrete from the carrier
"up" state, so NETDEV_UP before carrier up is not unexpected.

	What I was concerned with is that the carrier would be up but
speed or duplex would be invalid, which does not appear to be the case.

	In any event, I think I see what the failure is, I'm working up
a patch to test and will post it when I have it ready.

	-J

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

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
[...]
>	In any event, I think I see what the failure is, I'm working up
>a patch to test and will post it when I have it ready.

	Aleksei,

	Would you be able to test the following patch and see if it
resolves the issue in your testing?  This is against current net-next,
but applies to current net as well.  Your kernel appears to be a bit
older (as the message formats differ), so hopefully it will apply there
as well.  I've tested this a bit (but not the ARP mon portion), and it
seems to do the right thing, but I can't reproduce the original issue
locally.

	Thanks,

	-J

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 931d9d935686..38042399717b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2086,8 +2086,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 	ignore_updelay = !rcu_dereference(bond->curr_active_slave);
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
-		slave->link_new_state = slave->link;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
@@ -2121,7 +2120,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 			}
 
 			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_DOWN;
+				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				commit++;
 				continue;
 			}
@@ -2158,7 +2157,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 				slave->delay = 0;
 
 			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				commit++;
 				ignore_updelay = false;
 				continue;
@@ -2196,7 +2195,7 @@ static void bond_miimon_commit(struct bonding *bond)
 	struct slave *slave, *primary;
 
 	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
+		switch (slave->link_new_state) {
 		case BOND_LINK_NOCHANGE:
 			/* For 802.3ad mode, check current slave speed and
 			 * duplex again in case its port was disabled after
@@ -2268,8 +2267,8 @@ static void bond_miimon_commit(struct bonding *bond)
 
 		default:
 			slave_err(bond->dev, slave->dev, "invalid new link %d on slave\n",
-				  slave->new_link);
-			slave->new_link = BOND_LINK_NOCHANGE;
+				  slave->link_new_state);
+			bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 			continue;
 		}
@@ -2677,13 +2676,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		unsigned long trans_start = dev_trans_start(slave->dev);
 
-		slave->new_link = BOND_LINK_NOCHANGE;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, trans_start, 1) &&
 			    bond_time_in_interval(bond, slave->last_rx, 1)) {
 
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				slave_state_changed = 1;
 
 				/* primary_slave has no meaning in round-robin
@@ -2708,7 +2707,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			if (!bond_time_in_interval(bond, trans_start, 2) ||
 			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
 
-				slave->new_link = BOND_LINK_DOWN;
+				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				slave_state_changed = 1;
 
 				if (slave->link_failure_count < UINT_MAX)
@@ -2739,8 +2738,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			goto re_arm;
 
 		bond_for_each_slave(bond, slave, iter) {
-			if (slave->new_link != BOND_LINK_NOCHANGE)
-				slave->link = slave->new_link;
+			if (slave->link_new_state != BOND_LINK_NOCHANGE)
+				slave->link = slave->link_new_state;
 		}
 
 		if (slave_state_changed) {
@@ -2763,9 +2762,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 }
 
 /* Called to inspect slaves for active-backup mode ARP monitor link state
- * changes.  Sets new_link in slaves to specify what action should take
- * place for the slave.  Returns 0 if no changes are found, >0 if changes
- * to link states must be committed.
+ * changes.  Sets proposed link state in slaves to specify what action
+ * should take place for the slave.  Returns 0 if no changes are found, >0
+ * if changes to link states must be committed.
  *
  * Called with rcu_read_lock held.
  */
@@ -2777,12 +2776,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 	int commit = 0;
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 		last_rx = slave_last_rx(bond, slave);
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, last_rx, 1)) {
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				commit++;
 			}
 			continue;
@@ -2810,7 +2809,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		if (!bond_is_active_slave(slave) &&
 		    !rcu_access_pointer(bond->current_arp_slave) &&
 		    !bond_time_in_interval(bond, last_rx, 3)) {
-			slave->new_link = BOND_LINK_DOWN;
+			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 
@@ -2823,7 +2822,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		if (bond_is_active_slave(slave) &&
 		    (!bond_time_in_interval(bond, trans_start, 2) ||
 		     !bond_time_in_interval(bond, last_rx, 2))) {
-			slave->new_link = BOND_LINK_DOWN;
+			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 	}
@@ -2843,7 +2842,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
 	struct slave *slave;
 
 	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
+		switch (slave->link_new_state) {
 		case BOND_LINK_NOCHANGE:
 			continue;
 
@@ -2893,8 +2892,9 @@ static void bond_ab_arp_commit(struct bonding *bond)
 			continue;
 
 		default:
-			slave_err(bond->dev, slave->dev, "impossible: new_link %d on slave\n",
-				  slave->new_link);
+			slave_err(bond->dev, slave->dev,
+				  "impossible: link_new_state %d on slave\n",
+				  slave->link_new_state);
 			continue;
 		}
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index f7fe45689142..d416af72404b 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -159,7 +159,6 @@ struct slave {
 	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
 	s8     link;		/* one of BOND_LINK_XXXX */
 	s8     link_new_state;	/* one of BOND_LINK_XXXX */
-	s8     new_link;
 	u8     backup:1,   /* indicates backup slave. Value corresponds with
 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
 	       inactive:1, /* indicates inactive slave */
@@ -549,7 +548,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
 
 static inline void bond_commit_link_state(struct slave *slave, bool notify)
 {
-	if (slave->link == slave->link_new_state)
+	if (slave->link_new_state == BOND_LINK_NOCHANGE)
 		return;
 
 	slave->link = slave->link_new_state;


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

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Алексей Захаров]

>
> Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> [...]
> >       In any event, I think I see what the failure is, I'm working up
> >a patch to test and will post it when I have it ready.
>
>         Aleksei,
>
>         Would you be able to test the following patch and see if it
> resolves the issue in your testing?  This is against current net-next,
> but applies to current net as well.  Your kernel appears to be a bit
> older (as the message formats differ), so hopefully it will apply there
> as well.  I've tested this a bit (but not the ARP mon portion), and it
> seems to do the right thing, but I can't reproduce the original issue
> locally.
We're testing on ubuntu-bionic 4.15 kernel.

>
>         Thanks,
>
>         -J
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 931d9d935686..38042399717b 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2086,8 +2086,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>         ignore_updelay = !rcu_dereference(bond->curr_active_slave);
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> -               slave->link_new_state = slave->link;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 link_state = bond_check_dev_link(bond, slave->dev, 0);
>
> @@ -2121,7 +2120,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         }
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 commit++;
>                                 continue;
>                         }
> @@ -2158,7 +2157,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                                 slave->delay = 0;
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                                 ignore_updelay = false;
>                                 continue;
> @@ -2196,7 +2195,7 @@ static void bond_miimon_commit(struct bonding *bond)
>         struct slave *slave, *primary;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         /* For 802.3ad mode, check current slave speed and
>                          * duplex again in case its port was disabled after
> @@ -2268,8 +2267,8 @@ static void bond_miimon_commit(struct bonding *bond)
>
>                 default:
>                         slave_err(bond->dev, slave->dev, "invalid new link %d on slave\n",
> -                                 slave->new_link);
> -                       slave->new_link = BOND_LINK_NOCHANGE;
> +                                 slave->link_new_state);
> +                       bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                         continue;
>                 }
> @@ -2677,13 +2676,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>         bond_for_each_slave_rcu(bond, slave, iter) {
>                 unsigned long trans_start = dev_trans_start(slave->dev);
>
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>                             bond_time_in_interval(bond, slave->last_rx, 1)) {
>
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 slave_state_changed = 1;
>
>                                 /* primary_slave has no meaning in round-robin
> @@ -2708,7 +2707,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         if (!bond_time_in_interval(bond, trans_start, 2) ||
>                             !bond_time_in_interval(bond, slave->last_rx, 2)) {
>
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 slave_state_changed = 1;
>
>                                 if (slave->link_failure_count < UINT_MAX)
> @@ -2739,8 +2738,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         goto re_arm;
>
>                 bond_for_each_slave(bond, slave, iter) {
> -                       if (slave->new_link != BOND_LINK_NOCHANGE)
> -                               slave->link = slave->new_link;
> +                       if (slave->link_new_state != BOND_LINK_NOCHANGE)
> +                               slave->link = slave->link_new_state;
>                 }
>
>                 if (slave_state_changed) {
> @@ -2763,9 +2762,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>  }
>
>  /* Called to inspect slaves for active-backup mode ARP monitor link state
> - * changes.  Sets new_link in slaves to specify what action should take
> - * place for the slave.  Returns 0 if no changes are found, >0 if changes
> - * to link states must be committed.
> + * changes.  Sets proposed link state in slaves to specify what action
> + * should take place for the slave.  Returns 0 if no changes are found, >0
> + * if changes to link states must be committed.
>   *
>   * Called with rcu_read_lock held.
>   */
> @@ -2777,12 +2776,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>         int commit = 0;
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>                 last_rx = slave_last_rx(bond, slave);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, last_rx, 1)) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                         }
>                         continue;
> @@ -2810,7 +2809,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (!bond_is_active_slave(slave) &&
>                     !rcu_access_pointer(bond->current_arp_slave) &&
>                     !bond_time_in_interval(bond, last_rx, 3)) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>
> @@ -2823,7 +2822,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (bond_is_active_slave(slave) &&
>                     (!bond_time_in_interval(bond, trans_start, 2) ||
>                      !bond_time_in_interval(bond, last_rx, 2))) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>         }
> @@ -2843,7 +2842,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
>         struct slave *slave;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         continue;
>
> @@ -2893,8 +2892,9 @@ static void bond_ab_arp_commit(struct bonding *bond)
>                         continue;
>
>                 default:
> -                       slave_err(bond->dev, slave->dev, "impossible: new_link %d on slave\n",
> -                                 slave->new_link);
> +                       slave_err(bond->dev, slave->dev,
> +                                 "impossible: link_new_state %d on slave\n",
> +                                 slave->link_new_state);
>                         continue;
>                 }
>
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index f7fe45689142..d416af72404b 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -159,7 +159,6 @@ struct slave {
>         unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
>         s8     link;            /* one of BOND_LINK_XXXX */
>         s8     link_new_state;  /* one of BOND_LINK_XXXX */
> -       s8     new_link;
>         u8     backup:1,   /* indicates backup slave. Value corresponds with
>                               BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>                inactive:1, /* indicates inactive slave */
> @@ -549,7 +548,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
>
>  static inline void bond_commit_link_state(struct slave *slave, bool notify)
>  {
> -       if (slave->link == slave->link_new_state)
> +       if (slave->link_new_state == BOND_LINK_NOCHANGE)
>                 return;
>
>         slave->link = slave->link_new_state;
>
>
> ---
>         -Jay Vosburgh, jay.vosburgh@canonical.com
I had to change slave_err to netdev_err, because there's no slave_err
macro in 4.15.
I did some fast testing, things become a bit more weird for me.
Right after reboot one of the slaves hangs with actor port state 71
and partner port state 1.
It doesn't send lacpdu and seems to be broken.
Setting link down and up again fixes slave state.
Dmesg after boot:
[Fri Sep 20 17:56:53 2019] bond-san: Enslaving eth3 as a backup
interface with an up link
[Fri Sep 20 17:56:53 2019] bond-san: Enslaving eth2 as a backup
interface with an up link
[Fri Sep 20 17:56:54 2019] bond-san: Warning: No 802.3ad response from
the link partner for any adapters in the bond
[Fri Sep 20 17:56:54 2019] IPv6: ADDRCONF(NETDEV_UP): bond-san: link
is not ready
[Fri Sep 20 17:56:54 2019] 8021q: adding VLAN 0 to HW filter on device bond-san
[Fri Sep 20 17:56:54 2019] IPv6: ADDRCONF(NETDEV_CHANGE): bond-san:
link becomes ready
[Fri Sep 20 17:56:54 2019] bond-san: link status definitely up for
interface eth3, 10000 Mbps full duplex
[Fri Sep 20 17:56:54 2019] bond-san: first active interface up!

Broken link here is eth2. After set it down and up:
[Fri Sep 20 18:02:04 2019] mlx4_en: eth2: Link Up
[Fri Sep 20 18:02:04 2019] bond-san: link status up again after -200
ms for interface eth2
[Fri Sep 20 18:02:04 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex

If I'm trying to reproduce previous behavior, I get different messages
from time to time:
[Fri Sep 20 18:04:48 2019] mlx4_en: eth2: Link Up
[Fri Sep 20 18:04:48 2019] bond-san: link status up for interface
eth2, enabling it in 500 ms
[Fri Sep 20 18:04:48 2019] bond-san: invalid new link 3 on slave eth2
[Fri Sep 20 18:04:49 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex
or:
[Fri Sep 20 18:05:48 2019] mlx4_en: eth2: Link Up
[Fri Sep 20 18:05:49 2019] bond-san: link status up again after 0 ms
for interface eth2
[Fri Sep 20 18:05:49 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex

In both cases this slave works after up.

-- 
Best Regards,
Aleksei Zakharov
System administrator

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Алексей Захаров wrote:

>>
>> Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>> [...]
>> >       In any event, I think I see what the failure is, I'm working up
>> >a patch to test and will post it when I have it ready.
>>
>>         Aleksei,
>>
>>         Would you be able to test the following patch and see if it
>> resolves the issue in your testing?  This is against current net-next,
>> but applies to current net as well.  Your kernel appears to be a bit
>> older (as the message formats differ), so hopefully it will apply there
>> as well.  I've tested this a bit (but not the ARP mon portion), and it
>> seems to do the right thing, but I can't reproduce the original issue
>> locally.
>We're testing on ubuntu-bionic 4.15 kernel.

	I believe the following will apply for the Ubuntu 4.15 kernels;
this is against 4.15.0-60, so you may have some fuzz if your version is
far removed.  I've not tested this as I'm travelling at the moment, but
the only real difference is the netdev_err vs. slave_err changes in
bonding that aren't relevant to the fix itself.

	-J

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8cd25eb26a9a..b159a6595e11 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2057,8 +2057,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 	ignore_updelay = !rcu_dereference(bond->curr_active_slave);
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
-		slave->link_new_state = slave->link;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
@@ -2094,7 +2093,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 			}
 
 			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_DOWN;
+				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				commit++;
 				continue;
 			}
@@ -2133,7 +2132,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 				slave->delay = 0;
 
 			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				commit++;
 				ignore_updelay = false;
 				continue;
@@ -2153,7 +2152,7 @@ static void bond_miimon_commit(struct bonding *bond)
 	struct slave *slave, *primary;
 
 	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
+		switch (slave->link_new_state) {
 		case BOND_LINK_NOCHANGE:
 			continue;
 
@@ -2237,8 +2236,8 @@ static void bond_miimon_commit(struct bonding *bond)
 
 		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;
+				   slave->link_new_state, slave->dev->name);
+			bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 			continue;
 		}
@@ -2638,13 +2637,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		unsigned long trans_start = dev_trans_start(slave->dev);
 
-		slave->new_link = BOND_LINK_NOCHANGE;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, trans_start, 1) &&
 			    bond_time_in_interval(bond, slave->last_rx, 1)) {
 
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				slave_state_changed = 1;
 
 				/* primary_slave has no meaning in round-robin
@@ -2671,7 +2670,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			if (!bond_time_in_interval(bond, trans_start, 2) ||
 			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
 
-				slave->new_link = BOND_LINK_DOWN;
+				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				slave_state_changed = 1;
 
 				if (slave->link_failure_count < UINT_MAX)
@@ -2703,8 +2702,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			goto re_arm;
 
 		bond_for_each_slave(bond, slave, iter) {
-			if (slave->new_link != BOND_LINK_NOCHANGE)
-				slave->link = slave->new_link;
+			if (slave->link_new_state != BOND_LINK_NOCHANGE)
+				slave->link = slave->link_new_state;
 		}
 
 		if (slave_state_changed) {
@@ -2727,9 +2726,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 }
 
 /* Called to inspect slaves for active-backup mode ARP monitor link state
- * changes.  Sets new_link in slaves to specify what action should take
- * place for the slave.  Returns 0 if no changes are found, >0 if changes
- * to link states must be committed.
+ * changes.  Sets proposed link state in slaves to specify what action
+ * should take place for the slave.  Returns 0 if no changes are found, >0
+ * if changes to link states must be committed.
  *
  * Called with rcu_read_lock held.
  */
@@ -2741,12 +2740,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 	int commit = 0;
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 		last_rx = slave_last_rx(bond, slave);
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, last_rx, 1)) {
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				commit++;
 			}
 			continue;
@@ -2774,7 +2773,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		if (!bond_is_active_slave(slave) &&
 		    !rcu_access_pointer(bond->current_arp_slave) &&
 		    !bond_time_in_interval(bond, last_rx, 3)) {
-			slave->new_link = BOND_LINK_DOWN;
+			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 
@@ -2787,7 +2786,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		if (bond_is_active_slave(slave) &&
 		    (!bond_time_in_interval(bond, trans_start, 2) ||
 		     !bond_time_in_interval(bond, last_rx, 2))) {
-			slave->new_link = BOND_LINK_DOWN;
+			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 	}
@@ -2807,7 +2806,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
 	struct slave *slave;
 
 	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
+		switch (slave->link_new_state) {
 		case BOND_LINK_NOCHANGE:
 			continue;
 
@@ -2859,8 +2858,8 @@ static void bond_ab_arp_commit(struct bonding *bond)
 			continue;
 
 		default:
-			netdev_err(bond->dev, "impossible: new_link %d on slave %s\n",
-				   slave->new_link, slave->dev->name);
+			netdev_err(bond->dev, "impossible: link_new_state %d on slave %s\n",
+				   slave->link_new_state, slave->dev->name);
 			continue;
 		}
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index af927d97c1c1..65361d56109e 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -149,7 +149,6 @@ struct slave {
 	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
 	s8     link;		/* one of BOND_LINK_XXXX */
 	s8     link_new_state;	/* one of BOND_LINK_XXXX */
-	s8     new_link;
 	u8     backup:1,   /* indicates backup slave. Value corresponds with
 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
 	       inactive:1, /* indicates inactive slave */
@@ -519,7 +518,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
 
 static inline void bond_commit_link_state(struct slave *slave, bool notify)
 {
-	if (slave->link == slave->link_new_state)
+	if (slave->link_new_state == BOND_LINK_NOCHANGE)
 		return;
 
 	slave->link = slave->link_new_state;
-- 
2.23.0

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

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Алексей Захаров]

сб, 21 сент. 2019 г. в 10:06, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Алексей Захаров wrote:
>
> >>
> >> Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> >> [...]
> >> >       In any event, I think I see what the failure is, I'm working up
> >> >a patch to test and will post it when I have it ready.
> >>
> >>         Aleksei,
> >>
> >>         Would you be able to test the following patch and see if it
> >> resolves the issue in your testing?  This is against current net-next,
> >> but applies to current net as well.  Your kernel appears to be a bit
> >> older (as the message formats differ), so hopefully it will apply there
> >> as well.  I've tested this a bit (but not the ARP mon portion), and it
> >> seems to do the right thing, but I can't reproduce the original issue
> >> locally.
> >We're testing on ubuntu-bionic 4.15 kernel.
>
>         I believe the following will apply for the Ubuntu 4.15 kernels;
> this is against 4.15.0-60, so you may have some fuzz if your version is
> far removed.  I've not tested this as I'm travelling at the moment, but
> the only real difference is the netdev_err vs. slave_err changes in
> bonding that aren't relevant to the fix itself.
Thanks! But I've already applied previous patch. Changed slave_err to
netdev_err.
As I mentioned in the previous email, we have another issue now.
With this patch one slave is in broken state right after boot.

Right after reboot one of the slaves hangs with actor port state 71
and partner port state 1.
It doesn't send lacpdu and seems to be broken.
Setting link down and up again fixes slave state.
Dmesg after boot:
[Fri Sep 20 17:56:53 2019] bond-san: Enslaving eth3 as a backup
interface with an up link
[Fri Sep 20 17:56:53 2019] bond-san: Enslaving eth2 as a backup
interface with an up link
[Fri Sep 20 17:56:54 2019] bond-san: Warning: No 802.3ad response from
the link partner for any adapters in the bond
[Fri Sep 20 17:56:54 2019] IPv6: ADDRCONF(NETDEV_UP): bond-san: link
is not ready
[Fri Sep 20 17:56:54 2019] 8021q: adding VLAN 0 to HW filter on device bond-san
[Fri Sep 20 17:56:54 2019] IPv6: ADDRCONF(NETDEV_CHANGE): bond-san:
link becomes ready
[Fri Sep 20 17:56:54 2019] bond-san: link status definitely up for
interface eth3, 10000 Mbps full duplex
[Fri Sep 20 17:56:54 2019] bond-san: first active interface up!

Broken link here is eth2. After set it down and up:
[Fri Sep 20 18:02:04 2019] mlx4_en: eth2: Link Up
[Fri Sep 20 18:02:04 2019] bond-san: link status up again after -200
ms for interface eth2
[Fri Sep 20 18:02:04 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex

If I'm trying to reproduce previous behavior, I get different messages
from time to time:
[Fri Sep 20 18:04:48 2019] mlx4_en: eth2: Link Up
[Fri Sep 20 18:04:48 2019] bond-san: link status up for interface
eth2, enabling it in 500 ms
[Fri Sep 20 18:04:48 2019] bond-san: invalid new link 3 on slave eth2
[Fri Sep 20 18:04:49 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex
or:
[Fri Sep 20 18:05:48 2019] mlx4_en: eth2: Link Up
[Fri Sep 20 18:05:49 2019] bond-san: link status up again after 0 ms
for interface eth2
[Fri Sep 20 18:05:49 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex

In both cases this slave works after up.

>
>         -J
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 8cd25eb26a9a..b159a6595e11 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2057,8 +2057,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>         ignore_updelay = !rcu_dereference(bond->curr_active_slave);
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> -               slave->link_new_state = slave->link;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 link_state = bond_check_dev_link(bond, slave->dev, 0);
>
> @@ -2094,7 +2093,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         }
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 commit++;
>                                 continue;
>                         }
> @@ -2133,7 +2132,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                                 slave->delay = 0;
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                                 ignore_updelay = false;
>                                 continue;
> @@ -2153,7 +2152,7 @@ static void bond_miimon_commit(struct bonding *bond)
>         struct slave *slave, *primary;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         continue;
>
> @@ -2237,8 +2236,8 @@ static void bond_miimon_commit(struct bonding *bond)
>
>                 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;
> +                                  slave->link_new_state, slave->dev->name);
> +                       bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                         continue;
>                 }
> @@ -2638,13 +2637,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>         bond_for_each_slave_rcu(bond, slave, iter) {
>                 unsigned long trans_start = dev_trans_start(slave->dev);
>
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>                             bond_time_in_interval(bond, slave->last_rx, 1)) {
>
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 slave_state_changed = 1;
>
>                                 /* primary_slave has no meaning in round-robin
> @@ -2671,7 +2670,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         if (!bond_time_in_interval(bond, trans_start, 2) ||
>                             !bond_time_in_interval(bond, slave->last_rx, 2)) {
>
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 slave_state_changed = 1;
>
>                                 if (slave->link_failure_count < UINT_MAX)
> @@ -2703,8 +2702,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         goto re_arm;
>
>                 bond_for_each_slave(bond, slave, iter) {
> -                       if (slave->new_link != BOND_LINK_NOCHANGE)
> -                               slave->link = slave->new_link;
> +                       if (slave->link_new_state != BOND_LINK_NOCHANGE)
> +                               slave->link = slave->link_new_state;
>                 }
>
>                 if (slave_state_changed) {
> @@ -2727,9 +2726,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>  }
>
>  /* Called to inspect slaves for active-backup mode ARP monitor link state
> - * changes.  Sets new_link in slaves to specify what action should take
> - * place for the slave.  Returns 0 if no changes are found, >0 if changes
> - * to link states must be committed.
> + * changes.  Sets proposed link state in slaves to specify what action
> + * should take place for the slave.  Returns 0 if no changes are found, >0
> + * if changes to link states must be committed.
>   *
>   * Called with rcu_read_lock held.
>   */
> @@ -2741,12 +2740,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>         int commit = 0;
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>                 last_rx = slave_last_rx(bond, slave);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, last_rx, 1)) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                         }
>                         continue;
> @@ -2774,7 +2773,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (!bond_is_active_slave(slave) &&
>                     !rcu_access_pointer(bond->current_arp_slave) &&
>                     !bond_time_in_interval(bond, last_rx, 3)) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>
> @@ -2787,7 +2786,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (bond_is_active_slave(slave) &&
>                     (!bond_time_in_interval(bond, trans_start, 2) ||
>                      !bond_time_in_interval(bond, last_rx, 2))) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>         }
> @@ -2807,7 +2806,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
>         struct slave *slave;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         continue;
>
> @@ -2859,8 +2858,8 @@ static void bond_ab_arp_commit(struct bonding *bond)
>                         continue;
>
>                 default:
> -                       netdev_err(bond->dev, "impossible: new_link %d on slave %s\n",
> -                                  slave->new_link, slave->dev->name);
> +                       netdev_err(bond->dev, "impossible: link_new_state %d on slave %s\n",
> +                                  slave->link_new_state, slave->dev->name);
>                         continue;
>                 }
>
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index af927d97c1c1..65361d56109e 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -149,7 +149,6 @@ struct slave {
>         unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
>         s8     link;            /* one of BOND_LINK_XXXX */
>         s8     link_new_state;  /* one of BOND_LINK_XXXX */
> -       s8     new_link;
>         u8     backup:1,   /* indicates backup slave. Value corresponds with
>                               BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>                inactive:1, /* indicates inactive slave */
> @@ -519,7 +518,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
>
>  static inline void bond_commit_link_state(struct slave *slave, bool notify)
>  {
> -       if (slave->link == slave->link_new_state)
> +       if (slave->link_new_state == BOND_LINK_NOCHANGE)
>                 return;
>
>         slave->link = slave->link_new_state;
> --
> 2.23.0
>
> ---
>         -Jay Vosburgh, jay.vosburgh@canonical.com



-- 
Best Regards,
Aleksei Zakharov
System administrator

Re: [PATCH] bonding/802.3ad: fix slave initialization states race

[David Miller]

From: Aleksei Zakharov <zaharov@selectel.ru>
Date: Wed, 18 Sep 2019 16:05:45 +0300

> Once a while, one of 802.3ad slaves fails to initialize and hangs in
> BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
> link initialization transition states") checks slave->last_link_up. But
> link can still hang in weird state.
> After physical link comes up it sends first two LACPDU messages and
> doesn't work properly after that. It doesn't send or receive LACPDU.
> Once it happens, the only message in dmesg is:
> bond1: link status up again after 0 ms for interface eth2
> 
> This behavior can be reproduced (not every time):
> 1. Set slave link down
> 2. Wait for 1-3 seconds
> 3. Set slave link up
> 
> The fix is to check slave->link before setting it to BOND_LINK_FAIL or
> BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
> BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
> BOND_LINK_DOWN.
> 
> Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
> transition states")

Please do not split Fixes: tags onto mutliple lines.

> Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>

Please work out the final way to fix this with Jay and repost.

Thank you.

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Алексей Захаров wrote:
[...]
>Right after reboot one of the slaves hangs with actor port state 71
>and partner port state 1.
>It doesn't send lacpdu and seems to be broken.
>Setting link down and up again fixes slave state.
[...]

	I think I see what failed in the first patch, could you test the
following patch?  This one is for net-next, so you'd need to again swap
slave_err / netdev_err for the Ubuntu 4.15 kernel.

	Thanks,

	-J


diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 931d9d935686..5e248588259a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1617,6 +1617,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (bond->params.miimon) {
 		if (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS) {
 			if (bond->params.updelay) {
+/*XXX*/slave_info(bond_dev, slave_dev, "BOND_LINK_BACK initial state\n");
 				bond_set_slave_link_state(new_slave,
 							  BOND_LINK_BACK,
 							  BOND_SLAVE_NOTIFY_NOW);
@@ -2086,8 +2087,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 	ignore_updelay = !rcu_dereference(bond->curr_active_slave);
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
-		slave->link_new_state = slave->link;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		link_state = bond_check_dev_link(bond, slave->dev, 0);
 
@@ -2096,8 +2096,6 @@ static int bond_miimon_inspect(struct bonding *bond)
 			if (link_state)
 				continue;
 
-			bond_propose_link_state(slave, BOND_LINK_FAIL);
-			commit++;
 			slave->delay = bond->params.downdelay;
 			if (slave->delay) {
 				slave_info(bond->dev, slave->dev, "link status down for %sinterface, disabling it in %d ms\n",
@@ -2106,6 +2104,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 					    (bond_is_active_slave(slave) ?
 					     "active " : "backup ") : "",
 					   bond->params.downdelay * bond->params.miimon);
+				slave->link = BOND_LINK_FAIL;
 			}
 			/*FALLTHRU*/
 		case BOND_LINK_FAIL:
@@ -2121,7 +2120,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 			}
 
 			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_DOWN;
+				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				commit++;
 				continue;
 			}
@@ -2133,15 +2132,13 @@ static int bond_miimon_inspect(struct bonding *bond)
 			if (!link_state)
 				continue;
 
-			bond_propose_link_state(slave, BOND_LINK_BACK);
-			commit++;
 			slave->delay = bond->params.updelay;
-
 			if (slave->delay) {
 				slave_info(bond->dev, slave->dev, "link status up, enabling it in %d ms\n",
 					   ignore_updelay ? 0 :
 					   bond->params.updelay *
 					   bond->params.miimon);
+				slave->link = BOND_LINK_BACK;
 			}
 			/*FALLTHRU*/
 		case BOND_LINK_BACK:
@@ -2158,7 +2155,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 				slave->delay = 0;
 
 			if (slave->delay <= 0) {
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				commit++;
 				ignore_updelay = false;
 				continue;
@@ -2196,7 +2193,7 @@ static void bond_miimon_commit(struct bonding *bond)
 	struct slave *slave, *primary;
 
 	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
+		switch (slave->link_new_state) {
 		case BOND_LINK_NOCHANGE:
 			/* For 802.3ad mode, check current slave speed and
 			 * duplex again in case its port was disabled after
@@ -2268,8 +2265,8 @@ static void bond_miimon_commit(struct bonding *bond)
 
 		default:
 			slave_err(bond->dev, slave->dev, "invalid new link %d on slave\n",
-				  slave->new_link);
-			slave->new_link = BOND_LINK_NOCHANGE;
+				  slave->link_new_state);
+			bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 			continue;
 		}
@@ -2677,13 +2674,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		unsigned long trans_start = dev_trans_start(slave->dev);
 
-		slave->new_link = BOND_LINK_NOCHANGE;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, trans_start, 1) &&
 			    bond_time_in_interval(bond, slave->last_rx, 1)) {
 
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				slave_state_changed = 1;
 
 				/* primary_slave has no meaning in round-robin
@@ -2708,7 +2705,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			if (!bond_time_in_interval(bond, trans_start, 2) ||
 			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
 
-				slave->new_link = BOND_LINK_DOWN;
+				bond_propose_link_state(slave, BOND_LINK_DOWN);
 				slave_state_changed = 1;
 
 				if (slave->link_failure_count < UINT_MAX)
@@ -2739,8 +2736,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 			goto re_arm;
 
 		bond_for_each_slave(bond, slave, iter) {
-			if (slave->new_link != BOND_LINK_NOCHANGE)
-				slave->link = slave->new_link;
+			if (slave->link_new_state != BOND_LINK_NOCHANGE)
+				slave->link = slave->link_new_state;
 		}
 
 		if (slave_state_changed) {
@@ -2763,9 +2760,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 }
 
 /* Called to inspect slaves for active-backup mode ARP monitor link state
- * changes.  Sets new_link in slaves to specify what action should take
- * place for the slave.  Returns 0 if no changes are found, >0 if changes
- * to link states must be committed.
+ * changes.  Sets proposed link state in slaves to specify what action
+ * should take place for the slave.  Returns 0 if no changes are found, >0
+ * if changes to link states must be committed.
  *
  * Called with rcu_read_lock held.
  */
@@ -2777,12 +2774,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 	int commit = 0;
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		slave->new_link = BOND_LINK_NOCHANGE;
+		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 		last_rx = slave_last_rx(bond, slave);
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, last_rx, 1)) {
-				slave->new_link = BOND_LINK_UP;
+				bond_propose_link_state(slave, BOND_LINK_UP);
 				commit++;
 			}
 			continue;
@@ -2810,7 +2807,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		if (!bond_is_active_slave(slave) &&
 		    !rcu_access_pointer(bond->current_arp_slave) &&
 		    !bond_time_in_interval(bond, last_rx, 3)) {
-			slave->new_link = BOND_LINK_DOWN;
+			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 
@@ -2823,7 +2820,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		if (bond_is_active_slave(slave) &&
 		    (!bond_time_in_interval(bond, trans_start, 2) ||
 		     !bond_time_in_interval(bond, last_rx, 2))) {
-			slave->new_link = BOND_LINK_DOWN;
+			bond_propose_link_state(slave, BOND_LINK_DOWN);
 			commit++;
 		}
 	}
@@ -2843,7 +2840,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
 	struct slave *slave;
 
 	bond_for_each_slave(bond, slave, iter) {
-		switch (slave->new_link) {
+		switch (slave->link_new_state) {
 		case BOND_LINK_NOCHANGE:
 			continue;
 
@@ -2893,8 +2890,9 @@ static void bond_ab_arp_commit(struct bonding *bond)
 			continue;
 
 		default:
-			slave_err(bond->dev, slave->dev, "impossible: new_link %d on slave\n",
-				  slave->new_link);
+			slave_err(bond->dev, slave->dev,
+				  "impossible: link_new_state %d on slave\n",
+				  slave->link_new_state);
 			continue;
 		}
 
@@ -3133,6 +3131,7 @@ static int bond_slave_netdev_event(unsigned long event,
 		 * let link-monitoring (miimon) set it right when correct
 		 * speeds/duplex are available.
 		 */
+/*XXX*/slave_info(bond_dev, slave_dev, "EVENT %lu llu %lu\n", event, slave->last_link_up);
 		if (bond_update_speed_duplex(slave) &&
 		    BOND_MODE(bond) == BOND_MODE_8023AD) {
 			if (slave->last_link_up)
diff --git a/include/net/bonding.h b/include/net/bonding.h
index f7fe45689142..d416af72404b 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -159,7 +159,6 @@ struct slave {
 	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
 	s8     link;		/* one of BOND_LINK_XXXX */
 	s8     link_new_state;	/* one of BOND_LINK_XXXX */
-	s8     new_link;
 	u8     backup:1,   /* indicates backup slave. Value corresponds with
 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
 	       inactive:1, /* indicates inactive slave */
@@ -549,7 +548,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
 
 static inline void bond_commit_link_state(struct slave *slave, bool notify)
 {
-	if (slave->link == slave->link_new_state)
+	if (slave->link_new_state == BOND_LINK_NOCHANGE)
 		return;
 
 	slave->link = slave->link_new_state;


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

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Aleksei Zakharov]

ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Алексей Захаров wrote:
> [...]
> >Right after reboot one of the slaves hangs with actor port state 71
> >and partner port state 1.
> >It doesn't send lacpdu and seems to be broken.
> >Setting link down and up again fixes slave state.
> [...]
>
>         I think I see what failed in the first patch, could you test the
> following patch?  This one is for net-next, so you'd need to again swap
> slave_err / netdev_err for the Ubuntu 4.15 kernel.
>
I've tested new patch. It seems to work. I can't reproduce the bug
with this patch.
There are two types of messages when link becomes up:
First:
bond-san: EVENT 1 llu 4294895911 slave eth2
8021q: adding VLAN 0 to HW filter on device eth2
bond-san: link status definitely down for interface eth2, disabling it
mlx4_en: eth2: Link Up
bond-san: EVENT 4 llu 4294895911 slave eth2
bond-san: link status up for interface eth2, enabling it in 500 ms
bond-san: invalid new link 3 on slave eth2
bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
Second:
bond-san: EVENT 1 llu 4295147594 slave eth2
8021q: adding VLAN 0 to HW filter on device eth2
mlx4_en: eth2: Link Up
bond-san: EVENT 4 llu 4295147594 slave eth2
bond-san: link status up again after 0 ms for interface eth2
bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex

These messages (especially "invalid new link") look a bit unclear from
sysadmin point of view.
But lacp seems to work fine, thank you!

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 931d9d935686..5e248588259a 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1617,6 +1617,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>         if (bond->params.miimon) {
>                 if (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS) {
>                         if (bond->params.updelay) {
> +/*XXX*/slave_info(bond_dev, slave_dev, "BOND_LINK_BACK initial state\n");
>                                 bond_set_slave_link_state(new_slave,
>                                                           BOND_LINK_BACK,
>                                                           BOND_SLAVE_NOTIFY_NOW);
> @@ -2086,8 +2087,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>         ignore_updelay = !rcu_dereference(bond->curr_active_slave);
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> -               slave->link_new_state = slave->link;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 link_state = bond_check_dev_link(bond, slave->dev, 0);
>
> @@ -2096,8 +2096,6 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         if (link_state)
>                                 continue;
>
> -                       bond_propose_link_state(slave, BOND_LINK_FAIL);
> -                       commit++;
>                         slave->delay = bond->params.downdelay;
>                         if (slave->delay) {
>                                 slave_info(bond->dev, slave->dev, "link status down for %sinterface, disabling it in %d ms\n",
> @@ -2106,6 +2104,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                                             (bond_is_active_slave(slave) ?
>                                              "active " : "backup ") : "",
>                                            bond->params.downdelay * bond->params.miimon);
> +                               slave->link = BOND_LINK_FAIL;
>                         }
>                         /*FALLTHRU*/
>                 case BOND_LINK_FAIL:
> @@ -2121,7 +2120,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         }
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 commit++;
>                                 continue;
>                         }
> @@ -2133,15 +2132,13 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         if (!link_state)
>                                 continue;
>
> -                       bond_propose_link_state(slave, BOND_LINK_BACK);
> -                       commit++;
>                         slave->delay = bond->params.updelay;
> -
>                         if (slave->delay) {
>                                 slave_info(bond->dev, slave->dev, "link status up, enabling it in %d ms\n",
>                                            ignore_updelay ? 0 :
>                                            bond->params.updelay *
>                                            bond->params.miimon);
> +                               slave->link = BOND_LINK_BACK;
>                         }
>                         /*FALLTHRU*/
>                 case BOND_LINK_BACK:
> @@ -2158,7 +2155,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                                 slave->delay = 0;
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                                 ignore_updelay = false;
>                                 continue;
> @@ -2196,7 +2193,7 @@ static void bond_miimon_commit(struct bonding *bond)
>         struct slave *slave, *primary;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         /* For 802.3ad mode, check current slave speed and
>                          * duplex again in case its port was disabled after
> @@ -2268,8 +2265,8 @@ static void bond_miimon_commit(struct bonding *bond)
>
>                 default:
>                         slave_err(bond->dev, slave->dev, "invalid new link %d on slave\n",
> -                                 slave->new_link);
> -                       slave->new_link = BOND_LINK_NOCHANGE;
> +                                 slave->link_new_state);
> +                       bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                         continue;
>                 }
> @@ -2677,13 +2674,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>         bond_for_each_slave_rcu(bond, slave, iter) {
>                 unsigned long trans_start = dev_trans_start(slave->dev);
>
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>                             bond_time_in_interval(bond, slave->last_rx, 1)) {
>
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 slave_state_changed = 1;
>
>                                 /* primary_slave has no meaning in round-robin
> @@ -2708,7 +2705,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         if (!bond_time_in_interval(bond, trans_start, 2) ||
>                             !bond_time_in_interval(bond, slave->last_rx, 2)) {
>
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 slave_state_changed = 1;
>
>                                 if (slave->link_failure_count < UINT_MAX)
> @@ -2739,8 +2736,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         goto re_arm;
>
>                 bond_for_each_slave(bond, slave, iter) {
> -                       if (slave->new_link != BOND_LINK_NOCHANGE)
> -                               slave->link = slave->new_link;
> +                       if (slave->link_new_state != BOND_LINK_NOCHANGE)
> +                               slave->link = slave->link_new_state;
>                 }
>
>                 if (slave_state_changed) {
> @@ -2763,9 +2760,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>  }
>
>  /* Called to inspect slaves for active-backup mode ARP monitor link state
> - * changes.  Sets new_link in slaves to specify what action should take
> - * place for the slave.  Returns 0 if no changes are found, >0 if changes
> - * to link states must be committed.
> + * changes.  Sets proposed link state in slaves to specify what action
> + * should take place for the slave.  Returns 0 if no changes are found, >0
> + * if changes to link states must be committed.
>   *
>   * Called with rcu_read_lock held.
>   */
> @@ -2777,12 +2774,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>         int commit = 0;
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>                 last_rx = slave_last_rx(bond, slave);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, last_rx, 1)) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                         }
>                         continue;
> @@ -2810,7 +2807,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (!bond_is_active_slave(slave) &&
>                     !rcu_access_pointer(bond->current_arp_slave) &&
>                     !bond_time_in_interval(bond, last_rx, 3)) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>
> @@ -2823,7 +2820,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (bond_is_active_slave(slave) &&
>                     (!bond_time_in_interval(bond, trans_start, 2) ||
>                      !bond_time_in_interval(bond, last_rx, 2))) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>         }
> @@ -2843,7 +2840,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
>         struct slave *slave;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         continue;
>
> @@ -2893,8 +2890,9 @@ static void bond_ab_arp_commit(struct bonding *bond)
>                         continue;
>
>                 default:
> -                       slave_err(bond->dev, slave->dev, "impossible: new_link %d on slave\n",
> -                                 slave->new_link);
> +                       slave_err(bond->dev, slave->dev,
> +                                 "impossible: link_new_state %d on slave\n",
> +                                 slave->link_new_state);
>                         continue;
>                 }
>
> @@ -3133,6 +3131,7 @@ static int bond_slave_netdev_event(unsigned long event,
>                  * let link-monitoring (miimon) set it right when correct
>                  * speeds/duplex are available.
>                  */
> +/*XXX*/slave_info(bond_dev, slave_dev, "EVENT %lu llu %lu\n", event, slave->last_link_up);
>                 if (bond_update_speed_duplex(slave) &&
>                     BOND_MODE(bond) == BOND_MODE_8023AD) {
>                         if (slave->last_link_up)
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index f7fe45689142..d416af72404b 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -159,7 +159,6 @@ struct slave {
>         unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
>         s8     link;            /* one of BOND_LINK_XXXX */
>         s8     link_new_state;  /* one of BOND_LINK_XXXX */
> -       s8     new_link;
>         u8     backup:1,   /* indicates backup slave. Value corresponds with
>                               BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>                inactive:1, /* indicates inactive slave */
> @@ -549,7 +548,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
>
>  static inline void bond_commit_link_state(struct slave *slave, bool notify)
>  {
> -       if (slave->link == slave->link_new_state)
> +       if (slave->link_new_state == BOND_LINK_NOCHANGE)
>                 return;
>
>         slave->link = slave->link_new_state;
>
>
> ---
>         -Jay Vosburgh, jay.vosburgh@canonical.com



-- 
Best Regards,
Aleksei Zakharov
System administrator
Selectel - hosting for professionals
tel: +7 (812) 677-80-36

www.selectel.com

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Aleksei Zakharov <zaharov@selectel.ru> wrote:

>ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
>>
>> Алексей Захаров wrote:
>> [...]
>> >Right after reboot one of the slaves hangs with actor port state 71
>> >and partner port state 1.
>> >It doesn't send lacpdu and seems to be broken.
>> >Setting link down and up again fixes slave state.
>> [...]
>>
>>         I think I see what failed in the first patch, could you test the
>> following patch?  This one is for net-next, so you'd need to again swap
>> slave_err / netdev_err for the Ubuntu 4.15 kernel.
>>
>I've tested new patch. It seems to work. I can't reproduce the bug
>with this patch.
>There are two types of messages when link becomes up:
>First:
>bond-san: EVENT 1 llu 4294895911 slave eth2
>8021q: adding VLAN 0 to HW filter on device eth2
>bond-san: link status definitely down for interface eth2, disabling it
>mlx4_en: eth2: Link Up
>bond-san: EVENT 4 llu 4294895911 slave eth2
>bond-san: link status up for interface eth2, enabling it in 500 ms
>bond-san: invalid new link 3 on slave eth2
>bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
>Second:
>bond-san: EVENT 1 llu 4295147594 slave eth2
>8021q: adding VLAN 0 to HW filter on device eth2
>mlx4_en: eth2: Link Up
>bond-san: EVENT 4 llu 4295147594 slave eth2
>bond-san: link status up again after 0 ms for interface eth2
>bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
>
>These messages (especially "invalid new link") look a bit unclear from
>sysadmin point of view.

	The "invalid new link" is appearing because bond_miimon_commit
is being asked to commit a new state that isn't UP or DOWN (3 is
BOND_LINK_BACK).  I looked through the patched code today, and I don't
see a way to get to that message with the new link set to 3, so I'll add
some instrumentation and send out another patch to figure out what's
going on, as that shouldn't happen.

	I don't see the "invalid" message testing locally, I think
because my network device doesn't transition to carrier up as quickly as
yours.  I thought you were getting BOND_LINK_BACK passed through from
bond_enslave (which calls bond_set_slave_link_state, which will set
link_new_link to BOND_LINK_BACK and leave it there), but the
link_new_link is reset first thing in bond_miimon_inspect, so I'm not
sure how it gets into bond_miimon_commit (I'm thinking perhaps a
concurrent commit triggered by another slave, which then picks up this
proposed link state change by happenstance).

	-J

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

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Aleksei Zakharov]

чт, 26 сент. 2019 г. в 07:38, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Aleksei Zakharov <zaharov@selectel.ru> wrote:
>
> >ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
> >>
> >> Алексей Захаров wrote:
> >> [...]
> >> >Right after reboot one of the slaves hangs with actor port state 71
> >> >and partner port state 1.
> >> >It doesn't send lacpdu and seems to be broken.
> >> >Setting link down and up again fixes slave state.
> >> [...]
> >>
> >>         I think I see what failed in the first patch, could you test the
> >> following patch?  This one is for net-next, so you'd need to again swap
> >> slave_err / netdev_err for the Ubuntu 4.15 kernel.
> >>
> >I've tested new patch. It seems to work. I can't reproduce the bug
> >with this patch.
> >There are two types of messages when link becomes up:
> >First:
> >bond-san: EVENT 1 llu 4294895911 slave eth2
> >8021q: adding VLAN 0 to HW filter on device eth2
> >bond-san: link status definitely down for interface eth2, disabling it
> >mlx4_en: eth2: Link Up
> >bond-san: EVENT 4 llu 4294895911 slave eth2
> >bond-san: link status up for interface eth2, enabling it in 500 ms
> >bond-san: invalid new link 3 on slave eth2
> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> >Second:
> >bond-san: EVENT 1 llu 4295147594 slave eth2
> >8021q: adding VLAN 0 to HW filter on device eth2
> >mlx4_en: eth2: Link Up
> >bond-san: EVENT 4 llu 4295147594 slave eth2
> >bond-san: link status up again after 0 ms for interface eth2
> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> > [...]
>
>         The "invalid new link" is appearing because bond_miimon_commit
> is being asked to commit a new state that isn't UP or DOWN (3 is
> BOND_LINK_BACK).  I looked through the patched code today, and I don't
> see a way to get to that message with the new link set to 3, so I'll add
> some instrumentation and send out another patch to figure out what's
> going on, as that shouldn't happen.
>
>         I don't see the "invalid" message testing locally, I think
> because my network device doesn't transition to carrier up as quickly as
> yours.  I thought you were getting BOND_LINK_BACK passed through from
> bond_enslave (which calls bond_set_slave_link_state, which will set
> link_new_link to BOND_LINK_BACK and leave it there), but the
> link_new_link is reset first thing in bond_miimon_inspect, so I'm not
> sure how it gets into bond_miimon_commit (I'm thinking perhaps a
> concurrent commit triggered by another slave, which then picks up this
> proposed link state change by happenstance).
I assume that "invalid new link" happens in this way:
Interface goes up
NETDEV_CHANGE event occurs
bond_update_speed_duplex fails
and slave->last_link_up returns true
slave->link becomes BOND_LINK_FAIL
bond_check_dev_link returns 0
miimon proposes slave->link_new_state BOND_LINK_DOWN
NETDEV_UP event occurs
miimon sets commit++
miimon proposes slave->link_new_state BOND_LINK_BACK
miimon sets slave->link to BOND_LINK_BACK
we have updelay configured, so it doesn't set BOND_LINK_UP in the next
case section
miimon says "Invalid new link" and sets link state UP during next
inspection(after updelay, i suppose)

For the second type of messages it looks like this:
Interface goes up
NETDEV_CHANGE event occurs
bond_update_speed_duplex fails
and slave->last_link_up returns true
slave->link becomes BOND_LINK_FAIL
NETDEV_UP event occurs
bond_check_dev_link returns 1
miimon proposes slave->link_new_state BOND_LINK_UP and says "link
status up again"

My first patch changed slave->last_link_up check to (slave->link ==
BOND_LINK_UP).
This check looks more consistent for me, but I might be wrong here.
As a result if link was in BOND_LINK_FAIL or BOND_LINK_BACK when
CHANGE or UP event,
it became BOND_LINK_DOWN.
But if it was initially UP and bond_update_speed_duplex was unable to
get speed/duplex,
link became BOND_LINK_FAIL.

I don't understand a few things here:
How could a link be in a different state from time to time during the
first NETDEV_* event?
And why slave->last_link_up is set when the first NETDEV event occurs?

I hope I didn't messed things up too much here.

-- 
Best Regards,
Aleksei Zakharov

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Jay Vosburgh]

Aleksei Zakharov <zaharov@selectel.ru> wrote:

>чт, 26 сент. 2019 г. в 07:38, Jay Vosburgh <jay.vosburgh@canonical.com>:
>>
>> Aleksei Zakharov <zaharov@selectel.ru> wrote:
>>
>> >ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
>> >>
>> >> Алексей Захаров wrote:
>> >> [...]
>> >> >Right after reboot one of the slaves hangs with actor port state 71
>> >> >and partner port state 1.
>> >> >It doesn't send lacpdu and seems to be broken.
>> >> >Setting link down and up again fixes slave state.
>> >> [...]
>> >>
>> >>         I think I see what failed in the first patch, could you test the
>> >> following patch?  This one is for net-next, so you'd need to again swap
>> >> slave_err / netdev_err for the Ubuntu 4.15 kernel.
>> >>
>> >I've tested new patch. It seems to work. I can't reproduce the bug
>> >with this patch.
>> >There are two types of messages when link becomes up:
>> >First:
>> >bond-san: EVENT 1 llu 4294895911 slave eth2
>> >8021q: adding VLAN 0 to HW filter on device eth2
>> >bond-san: link status definitely down for interface eth2, disabling it
>> >mlx4_en: eth2: Link Up
>> >bond-san: EVENT 4 llu 4294895911 slave eth2
>> >bond-san: link status up for interface eth2, enabling it in 500 ms
>> >bond-san: invalid new link 3 on slave eth2
>> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
>> >Second:
>> >bond-san: EVENT 1 llu 4295147594 slave eth2
>> >8021q: adding VLAN 0 to HW filter on device eth2
>> >mlx4_en: eth2: Link Up
>> >bond-san: EVENT 4 llu 4295147594 slave eth2
>> >bond-san: link status up again after 0 ms for interface eth2
>> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
>> > [...]
>>
>>         The "invalid new link" is appearing because bond_miimon_commit
>> is being asked to commit a new state that isn't UP or DOWN (3 is
>> BOND_LINK_BACK).  I looked through the patched code today, and I don't
>> see a way to get to that message with the new link set to 3, so I'll add
>> some instrumentation and send out another patch to figure out what's
>> going on, as that shouldn't happen.
>>
>>         I don't see the "invalid" message testing locally, I think
>> because my network device doesn't transition to carrier up as quickly as
>> yours.  I thought you were getting BOND_LINK_BACK passed through from
>> bond_enslave (which calls bond_set_slave_link_state, which will set
>> link_new_link to BOND_LINK_BACK and leave it there), but the
>> link_new_link is reset first thing in bond_miimon_inspect, so I'm not
>> sure how it gets into bond_miimon_commit (I'm thinking perhaps a
>> concurrent commit triggered by another slave, which then picks up this
>> proposed link state change by happenstance).
>I assume that "invalid new link" happens in this way:
>Interface goes up
>NETDEV_CHANGE event occurs
>bond_update_speed_duplex fails
>and slave->last_link_up returns true
>slave->link becomes BOND_LINK_FAIL
>bond_check_dev_link returns 0
>miimon proposes slave->link_new_state BOND_LINK_DOWN
>NETDEV_UP event occurs
>miimon sets commit++
>miimon proposes slave->link_new_state BOND_LINK_BACK
>miimon sets slave->link to BOND_LINK_BACK

	I removed the "proposes link_new_state BOND_LINK_BACK" from the
second test patch and replaced it with the slave->link = BOND_LINK_BACK.
This particular place in the code also does not do commit++.  If you
have both of those in the code you're running, then perhaps you have a
merge error or some such.

	In the second test patch, the only place that could set
link_new_state to BOND_LINK_BACK is in bond_enslave, which calls
bond_set_slave_link_state if the slave is carrier up and updelay is
configured.  If that were to happen, there should be a "BOND_LINK_BACK
initial state" debug message, and the link_new_state should be replaced
with NOCHANGE at the first pass through bond_miimon_inspect.

	So, I'm unclear how the link_new_state can be BOND_LINK_BACK
from the message log you provided based on the second test patch code.

>we have updelay configured, so it doesn't set BOND_LINK_UP in the next
>case section
>miimon says "Invalid new link" and sets link state UP during next
>inspection(after updelay, i suppose)
>
>For the second type of messages it looks like this:
>Interface goes up
>NETDEV_CHANGE event occurs
>bond_update_speed_duplex fails
>and slave->last_link_up returns true
>slave->link becomes BOND_LINK_FAIL
>NETDEV_UP event occurs
>bond_check_dev_link returns 1
>miimon proposes slave->link_new_state BOND_LINK_UP and says "link
>status up again"
>
>My first patch changed slave->last_link_up check to (slave->link ==
>BOND_LINK_UP).
>This check looks more consistent for me, but I might be wrong here.
>As a result if link was in BOND_LINK_FAIL or BOND_LINK_BACK when
>CHANGE or UP event,
>it became BOND_LINK_DOWN.
>But if it was initially UP and bond_update_speed_duplex was unable to
>get speed/duplex,
>link became BOND_LINK_FAIL.
>
>I don't understand a few things here:
>How could a link be in a different state from time to time during the
>first NETDEV_* event?

	I'm not sure; possibly a race between the events in the kernel
and how long it takes for the hardware to establish Ethernet link up.

>And why slave->last_link_up is set when the first NETDEV event occurs?

	slave->last_link_up can be set at enslave time if the carrier
state of the slave (and thus the initial slave->link) is in a not-down
state.  There are some paths as well for modes that have an "active"
slave, but 802.3ad is not one of those.

	-J

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

RE: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[zhangsha (A)]

> -----Original Message-----
> From: Aleksei Zakharov [mailto:zaharov@selectel.ru]
> Sent: 2019年9月25日 19:02
> To: Jay Vosburgh <jay.vosburgh@canonical.com>
> Cc: netdev@vger.kernel.org; zhangsha (A) <zhangsha.zhang@huawei.com>
> Subject: Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race
> 
> ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
> >
> > Алексей Захаров wrote:
> > [...]
> > >Right after reboot one of the slaves hangs with actor port state 71
> > >and partner port state 1.
> > >It doesn't send lacpdu and seems to be broken.
> > >Setting link down and up again fixes slave state.
> > [...]
> >
> >         I think I see what failed in the first patch, could you test
> > the following patch?  This one is for net-next, so you'd need to again
> > swap slave_err / netdev_err for the Ubuntu 4.15 kernel.
> >
> I've tested new patch. It seems to work. I can't reproduce the bug with this
> patch.
> There are two types of messages when link becomes up:
> First:
> bond-san: EVENT 1 llu 4294895911 slave eth2
> 8021q: adding VLAN 0 to HW filter on device eth2
> bond-san: link status definitely down for interface eth2, disabling it
> mlx4_en: eth2: Link Up
> bond-san: EVENT 4 llu 4294895911 slave eth2
> bond-san: link status up for interface eth2, enabling it in 500 ms
> bond-san: invalid new link 3 on slave eth2
> bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> Second:
> bond-san: EVENT 1 llu 4295147594 slave eth2
> 8021q: adding VLAN 0 to HW filter on device eth2
> mlx4_en: eth2: Link Up
> bond-san: EVENT 4 llu 4295147594 slave eth2
> bond-san: link status up again after 0 ms for interface eth2
> bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> 
> These messages (especially "invalid new link") look a bit unclear from sysadmin
> point of view.
> But lacp seems to work fine, thank you!
> 

I tests the patch by skipping the bond_update_speed_duplex() function when
the bond receiving a 'NETDEV_UP' event, so that I can force the bond entering status
BOND_LINK_FAIL. In this scenario, my lacp seems to work fine. 

Messages I got:
[24662.081877] bond0: link status definitely down for interface eth2, disabling it
[24662.081881] bond0: first active interface up!
[24684.153871] bond0: link status up again after 0 ms for interface eth2
[24684.156846] bond0: link status definitely up for interface eth2, 10000 Mbps full duplex
[24684.156851] bond0: first active interface up!

> > diff --git a/drivers/net/bonding/bond_main.c
> > b/drivers/net/bonding/bond_main.c index 931d9d935686..5e248588259a
> > 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1617,6 +1617,7 @@ int bond_enslave(struct net_device *bond_dev,
> struct net_device *slave_dev,
> >         if (bond->params.miimon) {
> >                 if (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS) {
> >                         if (bond->params.updelay) {
> > +/*XXX*/slave_info(bond_dev, slave_dev, "BOND_LINK_BACK initial
> > +state\n");
> >                                 bond_set_slave_link_state(new_slave,
> >                                                           BOND_LINK_BACK,
> >
> > BOND_SLAVE_NOTIFY_NOW); @@ -2086,8 +2087,7 @@ static int
> bond_miimon_inspect(struct bonding *bond)
> >         ignore_updelay = !rcu_dereference(bond->curr_active_slave);
> >
> >         bond_for_each_slave_rcu(bond, slave, iter) {
> > -               slave->new_link = BOND_LINK_NOCHANGE;
> > -               slave->link_new_state = slave->link;
> > +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
> >
> >                 link_state = bond_check_dev_link(bond, slave->dev, 0);
> >
> > @@ -2096,8 +2096,6 @@ static int bond_miimon_inspect(struct bonding
> *bond)
> >                         if (link_state)
> >                                 continue;
> >
> > -                       bond_propose_link_state(slave, BOND_LINK_FAIL);
> > -                       commit++;
> >                         slave->delay = bond->params.downdelay;
> >                         if (slave->delay) {
> >                                 slave_info(bond->dev, slave->dev,
> > "link status down for %sinterface, disabling it in %d ms\n", @@ -2106,6
> +2104,7 @@ static int bond_miimon_inspect(struct bonding *bond)
> >                                             (bond_is_active_slave(slave) ?
> >                                              "active " : "backup ") : "",
> >                                            bond->params.downdelay *
> > bond->params.miimon);
> > +                               slave->link = BOND_LINK_FAIL;
> >                         }
> >                         /*FALLTHRU*/
> >                 case BOND_LINK_FAIL:
> > @@ -2121,7 +2120,7 @@ static int bond_miimon_inspect(struct bonding
> *bond)
> >                         }
> >
> >                         if (slave->delay <= 0) {
> > -                               slave->new_link = BOND_LINK_DOWN;
> > +                               bond_propose_link_state(slave,
> > + BOND_LINK_DOWN);
> >                                 commit++;
> >                                 continue;
> >                         }
> > @@ -2133,15 +2132,13 @@ static int bond_miimon_inspect(struct bonding
> *bond)
> >                         if (!link_state)
> >                                 continue;
> >
> > -                       bond_propose_link_state(slave, BOND_LINK_BACK);
> > -                       commit++;
> >                         slave->delay = bond->params.updelay;
> > -
> >                         if (slave->delay) {
> >                                 slave_info(bond->dev, slave->dev, "link status up, enabling
> it in %d ms\n",
> >                                            ignore_updelay ? 0 :
> >                                            bond->params.updelay *
> >                                            bond->params.miimon);
> > +                               slave->link = BOND_LINK_BACK;
> >                         }
> >                         /*FALLTHRU*/
> >                 case BOND_LINK_BACK:
> > @@ -2158,7 +2155,7 @@ static int bond_miimon_inspect(struct bonding
> *bond)
> >                                 slave->delay = 0;
> >
> >                         if (slave->delay <= 0) {
> > -                               slave->new_link = BOND_LINK_UP;
> > +                               bond_propose_link_state(slave,
> > + BOND_LINK_UP);
> >                                 commit++;
> >                                 ignore_updelay = false;
> >                                 continue; @@ -2196,7 +2193,7 @@ static
> > void bond_miimon_commit(struct bonding *bond)
> >         struct slave *slave, *primary;
> >
> >         bond_for_each_slave(bond, slave, iter) {
> > -               switch (slave->new_link) {
> > +               switch (slave->link_new_state) {
> >                 case BOND_LINK_NOCHANGE:
> >                         /* For 802.3ad mode, check current slave speed and
> >                          * duplex again in case its port was disabled
> > after @@ -2268,8 +2265,8 @@ static void bond_miimon_commit(struct
> > bonding *bond)
> >
> >                 default:
> >                         slave_err(bond->dev, slave->dev, "invalid new link %d on
> slave\n",
> > -                                 slave->new_link);
> > -                       slave->new_link = BOND_LINK_NOCHANGE;
> > +                                 slave->link_new_state);
> > +                       bond_propose_link_state(slave,
> > + BOND_LINK_NOCHANGE);
> >
> >                         continue;
> >                 }
> > @@ -2677,13 +2674,13 @@ static void bond_loadbalance_arp_mon(struct
> bonding *bond)
> >         bond_for_each_slave_rcu(bond, slave, iter) {
> >                 unsigned long trans_start =
> > dev_trans_start(slave->dev);
> >
> > -               slave->new_link = BOND_LINK_NOCHANGE;
> > +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
> >
> >                 if (slave->link != BOND_LINK_UP) {
> >                         if (bond_time_in_interval(bond, trans_start, 1) &&
> >                             bond_time_in_interval(bond,
> > slave->last_rx, 1)) {
> >
> > -                               slave->new_link = BOND_LINK_UP;
> > +                               bond_propose_link_state(slave,
> > + BOND_LINK_UP);
> >                                 slave_state_changed = 1;
> >
> >                                 /* primary_slave has no meaning in
> > round-robin @@ -2708,7 +2705,7 @@ static void
> bond_loadbalance_arp_mon(struct bonding *bond)
> >                         if (!bond_time_in_interval(bond, trans_start, 2) ||
> >                             !bond_time_in_interval(bond,
> > slave->last_rx, 2)) {
> >
> > -                               slave->new_link = BOND_LINK_DOWN;
> > +                               bond_propose_link_state(slave,
> > + BOND_LINK_DOWN);
> >                                 slave_state_changed = 1;
> >
> >                                 if (slave->link_failure_count <
> > UINT_MAX) @@ -2739,8 +2736,8 @@ static void
> bond_loadbalance_arp_mon(struct bonding *bond)
> >                         goto re_arm;
> >
> >                 bond_for_each_slave(bond, slave, iter) {
> > -                       if (slave->new_link != BOND_LINK_NOCHANGE)
> > -                               slave->link = slave->new_link;
> > +                       if (slave->link_new_state != BOND_LINK_NOCHANGE)
> > +                               slave->link = slave->link_new_state;
> >                 }
> >
> >                 if (slave_state_changed) { @@ -2763,9 +2760,9 @@
> > static void bond_loadbalance_arp_mon(struct bonding *bond)  }
> >
> >  /* Called to inspect slaves for active-backup mode ARP monitor link
> > state
> > - * changes.  Sets new_link in slaves to specify what action should
> > take
> > - * place for the slave.  Returns 0 if no changes are found, >0 if
> > changes
> > - * to link states must be committed.
> > + * changes.  Sets proposed link state in slaves to specify what
> > + action
> > + * should take place for the slave.  Returns 0 if no changes are
> > + found, >0
> > + * if changes to link states must be committed.
> >   *
> >   * Called with rcu_read_lock held.
> >   */
> > @@ -2777,12 +2774,12 @@ static int bond_ab_arp_inspect(struct bonding
> *bond)
> >         int commit = 0;
> >
> >         bond_for_each_slave_rcu(bond, slave, iter) {
> > -               slave->new_link = BOND_LINK_NOCHANGE;
> > +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
> >                 last_rx = slave_last_rx(bond, slave);
> >
> >                 if (slave->link != BOND_LINK_UP) {
> >                         if (bond_time_in_interval(bond, last_rx, 1)) {
> > -                               slave->new_link = BOND_LINK_UP;
> > +                               bond_propose_link_state(slave,
> > + BOND_LINK_UP);
> >                                 commit++;
> >                         }
> >                         continue;
> > @@ -2810,7 +2807,7 @@ static int bond_ab_arp_inspect(struct bonding
> *bond)
> >                 if (!bond_is_active_slave(slave) &&
> >                     !rcu_access_pointer(bond->current_arp_slave) &&
> >                     !bond_time_in_interval(bond, last_rx, 3)) {
> > -                       slave->new_link = BOND_LINK_DOWN;
> > +                       bond_propose_link_state(slave,
> > + BOND_LINK_DOWN);
> >                         commit++;
> >                 }
> >
> > @@ -2823,7 +2820,7 @@ static int bond_ab_arp_inspect(struct bonding
> *bond)
> >                 if (bond_is_active_slave(slave) &&
> >                     (!bond_time_in_interval(bond, trans_start, 2) ||
> >                      !bond_time_in_interval(bond, last_rx, 2))) {
> > -                       slave->new_link = BOND_LINK_DOWN;
> > +                       bond_propose_link_state(slave,
> > + BOND_LINK_DOWN);
> >                         commit++;
> >                 }
> >         }
> > @@ -2843,7 +2840,7 @@ static void bond_ab_arp_commit(struct bonding
> *bond)
> >         struct slave *slave;
> >
> >         bond_for_each_slave(bond, slave, iter) {
> > -               switch (slave->new_link) {
> > +               switch (slave->link_new_state) {
> >                 case BOND_LINK_NOCHANGE:
> >                         continue;
> >
> > @@ -2893,8 +2890,9 @@ static void bond_ab_arp_commit(struct bonding
> *bond)
> >                         continue;
> >
> >                 default:
> > -                       slave_err(bond->dev, slave->dev, "impossible: new_link %d on
> slave\n",
> > -                                 slave->new_link);
> > +                       slave_err(bond->dev, slave->dev,
> > +                                 "impossible: link_new_state %d on slave\n",
> > +                                 slave->link_new_state);
> >                         continue;
> >                 }
> >
> > @@ -3133,6 +3131,7 @@ static int bond_slave_netdev_event(unsigned long
> event,
> >                  * let link-monitoring (miimon) set it right when correct
> >                  * speeds/duplex are available.
> >                  */
> > +/*XXX*/slave_info(bond_dev, slave_dev, "EVENT %lu llu %lu\n", event,
> > +slave->last_link_up);
> >                 if (bond_update_speed_duplex(slave) &&
> >                     BOND_MODE(bond) == BOND_MODE_8023AD) {
> >                         if (slave->last_link_up) diff --git
> > a/include/net/bonding.h b/include/net/bonding.h index
> > f7fe45689142..d416af72404b 100644
> > --- a/include/net/bonding.h
> > +++ b/include/net/bonding.h
> > @@ -159,7 +159,6 @@ struct slave {
> >         unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
> >         s8     link;            /* one of BOND_LINK_XXXX */
> >         s8     link_new_state;  /* one of BOND_LINK_XXXX */
> > -       s8     new_link;
> >         u8     backup:1,   /* indicates backup slave. Value corresponds with
> >                               BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
> >                inactive:1, /* indicates inactive slave */ @@ -549,7
> > +548,7 @@ static inline void bond_propose_link_state(struct slave
> > *slave, int state)
> >
> >  static inline void bond_commit_link_state(struct slave *slave, bool
> > notify)  {
> > -       if (slave->link == slave->link_new_state)
> > +       if (slave->link_new_state == BOND_LINK_NOCHANGE)
> >                 return;
> >
> >         slave->link = slave->link_new_state;
> >
> >
> > ---
> >         -Jay Vosburgh, jay.vosburgh@canonical.com
> 
> 
> 
> --
> Best Regards,
> Aleksei Zakharov
> System administrator
> Selectel - hosting for professionals
> tel: +7 (812) 677-80-36
> 
> www.selectel.com

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Aleksei Zakharov]

чт, 26 сент. 2019 г. в 23:01, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Aleksei Zakharov <zaharov@selectel.ru> wrote:
>
> >чт, 26 сент. 2019 г. в 07:38, Jay Vosburgh <jay.vosburgh@canonical.com>:
> >>
> >> Aleksei Zakharov <zaharov@selectel.ru> wrote:
> >>
> >> >ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
> >> >>
> >> >> Алексей Захаров wrote:
> >> >> [...]
> >> >> >Right after reboot one of the slaves hangs with actor port state 71
> >> >> >and partner port state 1.
> >> >> >It doesn't send lacpdu and seems to be broken.
> >> >> >Setting link down and up again fixes slave state.
> >> >> [...]
> >> >>
> >> >>         I think I see what failed in the first patch, could you test the
> >> >> following patch?  This one is for net-next, so you'd need to again swap
> >> >> slave_err / netdev_err for the Ubuntu 4.15 kernel.
> >> >>
> >> >I've tested new patch. It seems to work. I can't reproduce the bug
> >> >with this patch.
> >> >There are two types of messages when link becomes up:
> >> >First:
> >> >bond-san: EVENT 1 llu 4294895911 slave eth2
> >> >8021q: adding VLAN 0 to HW filter on device eth2
> >> >bond-san: link status definitely down for interface eth2, disabling it
> >> >mlx4_en: eth2: Link Up
> >> >bond-san: EVENT 4 llu 4294895911 slave eth2
> >> >bond-san: link status up for interface eth2, enabling it in 500 ms
> >> >bond-san: invalid new link 3 on slave eth2
> >> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> >> >Second:
> >> >bond-san: EVENT 1 llu 4295147594 slave eth2
> >> >8021q: adding VLAN 0 to HW filter on device eth2
> >> >mlx4_en: eth2: Link Up
> >> >bond-san: EVENT 4 llu 4295147594 slave eth2
> >> >bond-san: link status up again after 0 ms for interface eth2
> >> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> >> > [...]
> >>
> >>         The "invalid new link" is appearing because bond_miimon_commit
> >> is being asked to commit a new state that isn't UP or DOWN (3 is
> >> BOND_LINK_BACK).  I looked through the patched code today, and I don't
> >> see a way to get to that message with the new link set to 3, so I'll add
> >> some instrumentation and send out another patch to figure out what's
> >> going on, as that shouldn't happen.
> >>
> >>         I don't see the "invalid" message testing locally, I think
> >> because my network device doesn't transition to carrier up as quickly as
> >> yours.  I thought you were getting BOND_LINK_BACK passed through from
> >> bond_enslave (which calls bond_set_slave_link_state, which will set
> >> link_new_link to BOND_LINK_BACK and leave it there), but the
> >> link_new_link is reset first thing in bond_miimon_inspect, so I'm not
> >> sure how it gets into bond_miimon_commit (I'm thinking perhaps a
> >> concurrent commit triggered by another slave, which then picks up this
> >> proposed link state change by happenstance).
> >I assume that "invalid new link" happens in this way:
> >Interface goes up
> >NETDEV_CHANGE event occurs
> >bond_update_speed_duplex fails
> >and slave->last_link_up returns true
> >slave->link becomes BOND_LINK_FAIL
> >bond_check_dev_link returns 0
> >miimon proposes slave->link_new_state BOND_LINK_DOWN
> >NETDEV_UP event occurs
> >miimon sets commit++
> >miimon proposes slave->link_new_state BOND_LINK_BACK
> >miimon sets slave->link to BOND_LINK_BACK
>
>         I removed the "proposes link_new_state BOND_LINK_BACK" from the
> second test patch and replaced it with the slave->link = BOND_LINK_BACK.
> This particular place in the code also does not do commit++.  If you
> have both of those in the code you're running, then perhaps you have a
> merge error or some such.
You are right, it was a merge issue.
I re-applied the patch and now it works without any error messages.
As usual, there are two types of messages.
This one is less often:
bond-san: EVENT 1 llu 4295238188 slave eth2
8021q: adding VLAN 0 to HW filter on device eth2
mlx4_en: eth2: Link Up
bond-san: EVENT 4 llu 4295238188 slave eth2
bond-san: link status up again after 0 ms for interface eth2
bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex

And this one is more often:
bond-san: EVENT 1 llu 4295242465 slave eth2
8021q: adding VLAN 0 to HW filter on device eth2
bond-san: link status definitely down for interface eth2, disabling it
mlx4_en: eth2: Link Up
bond-san: EVENT 4 llu 4295242465 slave eth2
bond-san: link status up for interface eth2, enabling it in 500 ms
bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex

Bonding works as expected in both cases.

>
> [...]
> >
> >I don't understand a few things here:
> >How could a link be in a different state from time to time during the
> >first NETDEV_* event?
>
>         I'm not sure; possibly a race between the events in the kernel
> and how long it takes for the hardware to establish Ethernet link up.
>
> >And why slave->last_link_up is set when the first NETDEV event occurs?
>
>         slave->last_link_up can be set at enslave time if the carrier
> state of the slave (and thus the initial slave->link) is in a not-down
> state.  There are some paths as well for modes that have an "active"
> slave, but 802.3ad is not one of those.
Thanks for the explanation!

-- 
Best Regards,
Aleksei Zakharov
System administrator

Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

[Aleksei Zakharov]

ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Алексей Захаров wrote:
> [...]
> >Right after reboot one of the slaves hangs with actor port state 71
> >and partner port state 1.
> >It doesn't send lacpdu and seems to be broken.
> >Setting link down and up again fixes slave state.
> [...]
>
>         I think I see what failed in the first patch, could you test the
> following patch?  This one is for net-next, so you'd need to again swap
> slave_err / netdev_err for the Ubuntu 4.15 kernel.
>
>         Thanks,
>
>         -J
>
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 931d9d935686..5e248588259a 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1617,6 +1617,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>         if (bond->params.miimon) {
>                 if (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS) {
>                         if (bond->params.updelay) {
> +/*XXX*/slave_info(bond_dev, slave_dev, "BOND_LINK_BACK initial state\n");
>                                 bond_set_slave_link_state(new_slave,
>                                                           BOND_LINK_BACK,
>                                                           BOND_SLAVE_NOTIFY_NOW);
> @@ -2086,8 +2087,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>         ignore_updelay = !rcu_dereference(bond->curr_active_slave);
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> -               slave->link_new_state = slave->link;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 link_state = bond_check_dev_link(bond, slave->dev, 0);
>
> @@ -2096,8 +2096,6 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         if (link_state)
>                                 continue;
>
> -                       bond_propose_link_state(slave, BOND_LINK_FAIL);
> -                       commit++;
>                         slave->delay = bond->params.downdelay;
>                         if (slave->delay) {
>                                 slave_info(bond->dev, slave->dev, "link status down for %sinterface, disabling it in %d ms\n",
> @@ -2106,6 +2104,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                                             (bond_is_active_slave(slave) ?
>                                              "active " : "backup ") : "",
>                                            bond->params.downdelay * bond->params.miimon);
> +                               slave->link = BOND_LINK_FAIL;
>                         }
>                         /*FALLTHRU*/
>                 case BOND_LINK_FAIL:
> @@ -2121,7 +2120,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         }
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 commit++;
>                                 continue;
>                         }
> @@ -2133,15 +2132,13 @@ static int bond_miimon_inspect(struct bonding *bond)
>                         if (!link_state)
>                                 continue;
>
> -                       bond_propose_link_state(slave, BOND_LINK_BACK);
> -                       commit++;
>                         slave->delay = bond->params.updelay;
> -
>                         if (slave->delay) {
>                                 slave_info(bond->dev, slave->dev, "link status up, enabling it in %d ms\n",
>                                            ignore_updelay ? 0 :
>                                            bond->params.updelay *
>                                            bond->params.miimon);
> +                               slave->link = BOND_LINK_BACK;
>                         }
>                         /*FALLTHRU*/
>                 case BOND_LINK_BACK:
> @@ -2158,7 +2155,7 @@ static int bond_miimon_inspect(struct bonding *bond)
>                                 slave->delay = 0;
>
>                         if (slave->delay <= 0) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                                 ignore_updelay = false;
>                                 continue;
> @@ -2196,7 +2193,7 @@ static void bond_miimon_commit(struct bonding *bond)
>         struct slave *slave, *primary;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         /* For 802.3ad mode, check current slave speed and
>                          * duplex again in case its port was disabled after
> @@ -2268,8 +2265,8 @@ static void bond_miimon_commit(struct bonding *bond)
>
>                 default:
>                         slave_err(bond->dev, slave->dev, "invalid new link %d on slave\n",
> -                                 slave->new_link);
> -                       slave->new_link = BOND_LINK_NOCHANGE;
> +                                 slave->link_new_state);
> +                       bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                         continue;
>                 }
> @@ -2677,13 +2674,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>         bond_for_each_slave_rcu(bond, slave, iter) {
>                 unsigned long trans_start = dev_trans_start(slave->dev);
>
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>                             bond_time_in_interval(bond, slave->last_rx, 1)) {
>
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 slave_state_changed = 1;
>
>                                 /* primary_slave has no meaning in round-robin
> @@ -2708,7 +2705,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         if (!bond_time_in_interval(bond, trans_start, 2) ||
>                             !bond_time_in_interval(bond, slave->last_rx, 2)) {
>
> -                               slave->new_link = BOND_LINK_DOWN;
> +                               bond_propose_link_state(slave, BOND_LINK_DOWN);
>                                 slave_state_changed = 1;
>
>                                 if (slave->link_failure_count < UINT_MAX)
> @@ -2739,8 +2736,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>                         goto re_arm;
>
>                 bond_for_each_slave(bond, slave, iter) {
> -                       if (slave->new_link != BOND_LINK_NOCHANGE)
> -                               slave->link = slave->new_link;
> +                       if (slave->link_new_state != BOND_LINK_NOCHANGE)
> +                               slave->link = slave->link_new_state;
>                 }
>
>                 if (slave_state_changed) {
> @@ -2763,9 +2760,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
>  }
>
>  /* Called to inspect slaves for active-backup mode ARP monitor link state
> - * changes.  Sets new_link in slaves to specify what action should take
> - * place for the slave.  Returns 0 if no changes are found, >0 if changes
> - * to link states must be committed.
> + * changes.  Sets proposed link state in slaves to specify what action
> + * should take place for the slave.  Returns 0 if no changes are found, >0
> + * if changes to link states must be committed.
>   *
>   * Called with rcu_read_lock held.
>   */
> @@ -2777,12 +2774,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>         int commit = 0;
>
>         bond_for_each_slave_rcu(bond, slave, iter) {
> -               slave->new_link = BOND_LINK_NOCHANGE;
> +               bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>                 last_rx = slave_last_rx(bond, slave);
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, last_rx, 1)) {
> -                               slave->new_link = BOND_LINK_UP;
> +                               bond_propose_link_state(slave, BOND_LINK_UP);
>                                 commit++;
>                         }
>                         continue;
> @@ -2810,7 +2807,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (!bond_is_active_slave(slave) &&
>                     !rcu_access_pointer(bond->current_arp_slave) &&
>                     !bond_time_in_interval(bond, last_rx, 3)) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>
> @@ -2823,7 +2820,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
>                 if (bond_is_active_slave(slave) &&
>                     (!bond_time_in_interval(bond, trans_start, 2) ||
>                      !bond_time_in_interval(bond, last_rx, 2))) {
> -                       slave->new_link = BOND_LINK_DOWN;
> +                       bond_propose_link_state(slave, BOND_LINK_DOWN);
>                         commit++;
>                 }
>         }
> @@ -2843,7 +2840,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
>         struct slave *slave;
>
>         bond_for_each_slave(bond, slave, iter) {
> -               switch (slave->new_link) {
> +               switch (slave->link_new_state) {
>                 case BOND_LINK_NOCHANGE:
>                         continue;
>
> @@ -2893,8 +2890,9 @@ static void bond_ab_arp_commit(struct bonding *bond)
>                         continue;
>
>                 default:
> -                       slave_err(bond->dev, slave->dev, "impossible: new_link %d on slave\n",
> -                                 slave->new_link);
> +                       slave_err(bond->dev, slave->dev,
> +                                 "impossible: link_new_state %d on slave\n",
> +                                 slave->link_new_state);
>                         continue;
>                 }
>
> @@ -3133,6 +3131,7 @@ static int bond_slave_netdev_event(unsigned long event,
>                  * let link-monitoring (miimon) set it right when correct
>                  * speeds/duplex are available.
>                  */
> +/*XXX*/slave_info(bond_dev, slave_dev, "EVENT %lu llu %lu\n", event, slave->last_link_up);
>                 if (bond_update_speed_duplex(slave) &&
>                     BOND_MODE(bond) == BOND_MODE_8023AD) {
>                         if (slave->last_link_up)
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index f7fe45689142..d416af72404b 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -159,7 +159,6 @@ struct slave {
>         unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
>         s8     link;            /* one of BOND_LINK_XXXX */
>         s8     link_new_state;  /* one of BOND_LINK_XXXX */
> -       s8     new_link;
>         u8     backup:1,   /* indicates backup slave. Value corresponds with
>                               BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>                inactive:1, /* indicates inactive slave */
> @@ -549,7 +548,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)
>
>  static inline void bond_commit_link_state(struct slave *slave, bool notify)
>  {
> -       if (slave->link == slave->link_new_state)
> +       if (slave->link_new_state == BOND_LINK_NOCHANGE)
>                 return;
>
>         slave->link = slave->link_new_state;
>
>
> ---
>         -Jay Vosburgh, jay.vosburgh@canonical.com

Hi! As we haven't found any issues with this patch it seems to work fine.
Will this patch be applied on net-next?


-- 
Best Regards,
Aleksei Zakharov
System administrator