* [PATCH] bonding/802.3ad: fix slave initialization states race @ 2019-09-18 13:05 Aleksei Zakharov 2019-09-18 14:34 ` Jay Vosburgh 2019-09-24 13:52 ` David Miller 0 siblings, 2 replies; 19+ messages in thread From: Aleksei Zakharov @ 2019-09-18 13:05 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-18 13:05 [PATCH] bonding/802.3ad: fix slave initialization states race Aleksei Zakharov @ 2019-09-18 14:34 ` Jay Vosburgh [not found] ` <CAJYOGF9KZdouvmTxQcTOQgsi-uBxbvW50K3ufW1=8neeW98QVA@mail.gmail.com> 2019-09-24 13:52 ` David Miller 1 sibling, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-18 14:34 UTC (permalink / raw) To: Aleksei Zakharov; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAJYOGF9KZdouvmTxQcTOQgsi-uBxbvW50K3ufW1=8neeW98QVA@mail.gmail.com>]
* Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race [not found] ` <CAJYOGF9KZdouvmTxQcTOQgsi-uBxbvW50K3ufW1=8neeW98QVA@mail.gmail.com> @ 2019-09-18 18:27 ` Алексей Захаров 2019-09-19 8:00 ` Jay Vosburgh 0 siblings, 1 reply; 19+ messages in thread From: Алексей Захаров @ 2019-09-18 18:27 UTC (permalink / raw) To: jay.vosburgh, netdev > >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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-18 18:27 ` Fwd: " Алексей Захаров @ 2019-09-19 8:00 ` Jay Vosburgh 2019-09-19 9:53 ` Алексей Захаров 0 siblings, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-19 8:00 UTC (permalink / raw) To: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0JfQsNGF0LDRgNC+0LI=?=; +Cc: netdev Алексей Захаров 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-19 8:00 ` Jay Vosburgh @ 2019-09-19 9:53 ` Алексей Захаров 2019-09-19 15:27 ` Jay Vosburgh 0 siblings, 1 reply; 19+ messages in thread From: Алексей Захаров @ 2019-09-19 9:53 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev чт, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-19 9:53 ` Алексей Захаров @ 2019-09-19 15:27 ` Jay Vosburgh 2019-09-20 13:52 ` Jay Vosburgh 0 siblings, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-19 15:27 UTC (permalink / raw) To: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0JfQsNGF0LDRgNC+0LI=?=; +Cc: netdev Алексей Захаров 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-19 15:27 ` Jay Vosburgh @ 2019-09-20 13:52 ` Jay Vosburgh 2019-09-20 16:00 ` Алексей Захаров 0 siblings, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-20 13:52 UTC (permalink / raw) Cc: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0JfQsNGF0LDRgNC+0LI=?=, netdev 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-20 13:52 ` Jay Vosburgh @ 2019-09-20 16:00 ` Алексей Захаров 2019-09-21 7:06 ` Jay Vosburgh 0 siblings, 1 reply; 19+ messages in thread From: Алексей Захаров @ 2019-09-20 16:00 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-20 16:00 ` Алексей Захаров @ 2019-09-21 7:06 ` Jay Vosburgh 2019-09-21 11:17 ` Алексей Захаров 0 siblings, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-21 7:06 UTC (permalink / raw) To: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0JfQsNGF0LDRgNC+0LI=?=; +Cc: netdev Алексей Захаров 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-21 7:06 ` Jay Vosburgh @ 2019-09-21 11:17 ` Алексей Захаров 2019-09-25 0:31 ` Jay Vosburgh 0 siblings, 1 reply; 19+ messages in thread From: Алексей Захаров @ 2019-09-21 11:17 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev сб, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-21 11:17 ` Алексей Захаров @ 2019-09-25 0:31 ` Jay Vosburgh 2019-09-25 11:01 ` Aleksei Zakharov 2019-10-22 12:05 ` Aleksei Zakharov 0 siblings, 2 replies; 19+ messages in thread From: Jay Vosburgh @ 2019-09-25 0:31 UTC (permalink / raw) To: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0JfQsNGF0LDRgNC+0LI=?=; +Cc: netdev, zhangsha (A) Алексей Захаров 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-25 0:31 ` Jay Vosburgh @ 2019-09-25 11:01 ` Aleksei Zakharov 2019-09-26 4:38 ` Jay Vosburgh 2019-09-27 9:43 ` zhangsha (A) 2019-10-22 12:05 ` Aleksei Zakharov 1 sibling, 2 replies; 19+ messages in thread From: Aleksei Zakharov @ 2019-09-25 11:01 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, zhangsha (A) ср, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-25 11:01 ` Aleksei Zakharov @ 2019-09-26 4:38 ` Jay Vosburgh 2019-09-26 14:25 ` Aleksei Zakharov 2019-09-27 9:43 ` zhangsha (A) 1 sibling, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-26 4:38 UTC (permalink / raw) To: Aleksei Zakharov; +Cc: netdev, zhangsha (A) 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-26 4:38 ` Jay Vosburgh @ 2019-09-26 14:25 ` Aleksei Zakharov 2019-09-26 20:01 ` Jay Vosburgh 0 siblings, 1 reply; 19+ messages in thread From: Aleksei Zakharov @ 2019-09-26 14:25 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, zhangsha (A) чт, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-26 14:25 ` Aleksei Zakharov @ 2019-09-26 20:01 ` Jay Vosburgh 2019-09-27 11:14 ` Aleksei Zakharov 0 siblings, 1 reply; 19+ messages in thread From: Jay Vosburgh @ 2019-09-26 20:01 UTC (permalink / raw) To: Aleksei Zakharov; +Cc: netdev, zhangsha (A) 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-26 20:01 ` Jay Vosburgh @ 2019-09-27 11:14 ` Aleksei Zakharov 0 siblings, 0 replies; 19+ messages in thread From: Aleksei Zakharov @ 2019-09-27 11:14 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, zhangsha (A) чт, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-25 11:01 ` Aleksei Zakharov 2019-09-26 4:38 ` Jay Vosburgh @ 2019-09-27 9:43 ` zhangsha (A) 1 sibling, 0 replies; 19+ messages in thread From: zhangsha (A) @ 2019-09-27 9:43 UTC (permalink / raw) To: Aleksei Zakharov, Jay Vosburgh; +Cc: netdev > -----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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-25 0:31 ` Jay Vosburgh 2019-09-25 11:01 ` Aleksei Zakharov @ 2019-10-22 12:05 ` Aleksei Zakharov 1 sibling, 0 replies; 19+ messages in thread From: Aleksei Zakharov @ 2019-10-22 12:05 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, zhangsha (A) ср, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] bonding/802.3ad: fix slave initialization states race 2019-09-18 13:05 [PATCH] bonding/802.3ad: fix slave initialization states race Aleksei Zakharov 2019-09-18 14:34 ` Jay Vosburgh @ 2019-09-24 13:52 ` David Miller 1 sibling, 0 replies; 19+ messages in thread From: David Miller @ 2019-09-24 13:52 UTC (permalink / raw) To: zaharov; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-10-22 12:06 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-18 13:05 [PATCH] bonding/802.3ad: fix slave initialization states race Aleksei Zakharov 2019-09-18 14:34 ` Jay Vosburgh [not found] ` <CAJYOGF9KZdouvmTxQcTOQgsi-uBxbvW50K3ufW1=8neeW98QVA@mail.gmail.com> 2019-09-18 18:27 ` Fwd: " Алексей Захаров 2019-09-19 8:00 ` Jay Vosburgh 2019-09-19 9:53 ` Алексей Захаров 2019-09-19 15:27 ` Jay Vosburgh 2019-09-20 13:52 ` Jay Vosburgh 2019-09-20 16:00 ` Алексей Захаров 2019-09-21 7:06 ` Jay Vosburgh 2019-09-21 11:17 ` Алексей Захаров 2019-09-25 0:31 ` Jay Vosburgh 2019-09-25 11:01 ` Aleksei Zakharov 2019-09-26 4:38 ` Jay Vosburgh 2019-09-26 14:25 ` Aleksei Zakharov 2019-09-26 20:01 ` Jay Vosburgh 2019-09-27 11:14 ` Aleksei Zakharov 2019-09-27 9:43 ` zhangsha (A) 2019-10-22 12:05 ` Aleksei Zakharov 2019-09-24 13:52 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).