If selection logic decides to switch to a new aggregator it disables
all ports of the old aggregator, but doesn't enable ports on
the new aggregator. These ports will eventually be enabled when
the next LACPDU is received, which might take some time and without an
active port transmitted frames are dropped. Avoid this by enabling
already collected ports of the new aggregator immediately.
Signed-off-by: Thomas Bogendoerfer <[email protected]>
---
drivers/net/bonding/bond_3ad.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c6807e473ab7..529e2a7c51e2 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1876,6 +1876,13 @@ static void ad_agg_selection_logic(struct aggregator *agg,
__disable_port(port);
}
}
+
+ /* enable ports on new active aggregator */
+ for (port = best->lag_ports; port;
+ port = port->next_port_in_aggregator) {
+ __enable_port(port);
+ }
+
/* Slave array needs update. */
*update_slave_arr = true;
}
--
2.35.3
On Thu, Apr 04, 2024 at 01:49:08PM +0200, Thomas Bogendoerfer wrote:
> If selection logic decides to switch to a new aggregator it disables
> all ports of the old aggregator, but doesn't enable ports on
> the new aggregator. These ports will eventually be enabled when
> the next LACPDU is received, which might take some time and without an
> active port transmitted frames are dropped. Avoid this by enabling
> already collected ports of the new aggregator immediately.
>
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
Hi Thomas,
I will leave the technical review to Jay and others. But as a fix, I think
this patch warrants a Fixes tag. It should be sufficient to respond to this
email thread with an appropriate tag.
> ---
> drivers/net/bonding/bond_3ad.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index c6807e473ab7..529e2a7c51e2 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -1876,6 +1876,13 @@ static void ad_agg_selection_logic(struct aggregator *agg,
> __disable_port(port);
> }
> }
> +
> + /* enable ports on new active aggregator */
> + for (port = best->lag_ports; port;
> + port = port->next_port_in_aggregator) {
> + __enable_port(port);
> + }
> +
> /* Slave array needs update. */
> *update_slave_arr = true;
> }
> --
> 2.35.3
>
>
Thomas Bogendoerfer <[email protected]> wrote:
>If selection logic decides to switch to a new aggregator it disables
>all ports of the old aggregator, but doesn't enable ports on
>the new aggregator. These ports will eventually be enabled when
>the next LACPDU is received, which might take some time and without an
>active port transmitted frames are dropped. Avoid this by enabling
>already collected ports of the new aggregator immediately.
>
>Signed-off-by: Thomas Bogendoerfer <[email protected]>
>---
> drivers/net/bonding/bond_3ad.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index c6807e473ab7..529e2a7c51e2 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -1876,6 +1876,13 @@ static void ad_agg_selection_logic(struct aggregator *agg,
> __disable_port(port);
> }
> }
>+
>+ /* enable ports on new active aggregator */
>+ for (port = best->lag_ports; port;
>+ port = port->next_port_in_aggregator) {
>+ __enable_port(port);
>+ }
>+
I think this will do the wrong thing if the port in question is
not in a valid state to send or receive (i.e., it is not one of
COLLECTING_DISTRIBUTING, COLLECTING, or DISTRIBUTING).
As it happens, this situation, except for the case of individual
ports, is handled just below this code:
/* if the selected aggregator is of join individuals
* (partner_system is NULL), enable their ports
*/
active = __get_active_agg(origin);
if (active) {
if (!__agg_has_partner(active)) {
for (port = active->lag_ports; port;
port = port->next_port_in_aggregator) {
__enable_port(port);
}
*update_slave_arr = true;
}
}
rcu_read_unlock();
FWIW, looking at it, I'm not sure that "__agg_has_partner" is
the proper test for invididual-ness, but I'd have to do a bit of poking
to confirm that. In any event, that's not what you want to change right
now.
Instead of adding another block that does more or less the same
thing, I'd suggest updating this logic to include tests for C_D, C, or D
states, and enabling the ports if that is the case. Probably something
like (I have not tested or compiled this at all):
if (active) {
if (!__agg_has_partner(active)) {
[ ... the current !__agg_has_partner() stuff ]
} else {
for (port = active->lag_ports; port;
port = port->next_port_in_aggregator) {
switch (port->sm_mux_state) {
case AD_MUX_DISTRIBUTING:
case AD_MUX_COLLECTING_DISTRIBUTING:
ad_enable_collecting_distributing(port,
update_slave_arr);
port->ntt = true;
break;
case AD_MUX_COLLECTING:
ad_enable_collecting(port);
ad_disable_distributing(port, update_slave_arr);
port->ntt = true;
break;
default:
break;
}
Using the wrapper functions (instead of calling __enable_port,
et al, directly) enables logging for the transitions.
-J
> /* Slave array needs update. */
> *update_slave_arr = true;
> }
>--
>2.35.3
>
>
On Sat, 6 Apr 2024 17:03:54 +0100
Simon Horman <[email protected]> wrote:
> On Thu, Apr 04, 2024 at 01:49:08PM +0200, Thomas Bogendoerfer wrote:
> > If selection logic decides to switch to a new aggregator it disables
> > all ports of the old aggregator, but doesn't enable ports on
> > the new aggregator. These ports will eventually be enabled when
> > the next LACPDU is received, which might take some time and without an
> > active port transmitted frames are dropped. Avoid this by enabling
> > already collected ports of the new aggregator immediately.
> >
> > Signed-off-by: Thomas Bogendoerfer <[email protected]>
>
> Hi Thomas,
>
> I will leave the technical review to Jay and others. But as a fix, I think
> this patch warrants a Fixes tag. It should be sufficient to respond to this
> email thread with an appropriate tag.
current behavior is older than our git tree. So should I really add
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
?
Thomas.
--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich
On Mon, 08 Apr 2024 09:06:11 -0700
Jay Vosburgh <[email protected]> wrote:
> Thomas Bogendoerfer <[email protected]> wrote:
>
> >If selection logic decides to switch to a new aggregator it disables
> >all ports of the old aggregator, but doesn't enable ports on
> >the new aggregator. These ports will eventually be enabled when
> >the next LACPDU is received, which might take some time and without an
> >active port transmitted frames are dropped. Avoid this by enabling
> >already collected ports of the new aggregator immediately.
> >
> >Signed-off-by: Thomas Bogendoerfer <[email protected]>
> >---
> > drivers/net/bonding/bond_3ad.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> >index c6807e473ab7..529e2a7c51e2 100644
> >--- a/drivers/net/bonding/bond_3ad.c
> >+++ b/drivers/net/bonding/bond_3ad.c
> >@@ -1876,6 +1876,13 @@ static void ad_agg_selection_logic(struct aggregator *agg,
> > __disable_port(port);
> > }
> > }
> >+
> >+ /* enable ports on new active aggregator */
> >+ for (port = best->lag_ports; port;
> >+ port = port->next_port_in_aggregator) {
> >+ __enable_port(port);
> >+ }
> >+
>
> I think this will do the wrong thing if the port in question is
> not in a valid state to send or receive (i.e., it is not one of
> COLLECTING_DISTRIBUTING, COLLECTING, or DISTRIBUTING).
>
>
> As it happens, this situation, except for the case of individual
> ports, is handled just below this code:
>
> /* if the selected aggregator is of join individuals
> * (partner_system is NULL), enable their ports
> */
> active = __get_active_agg(origin);
>
> if (active) {
> if (!__agg_has_partner(active)) {
> for (port = active->lag_ports; port;
> port = port->next_port_in_aggregator) {
> __enable_port(port);
> }
> *update_slave_arr = true;
> }
> }
>
> rcu_read_unlock();
>
> FWIW, looking at it, I'm not sure that "__agg_has_partner" is
> the proper test for invididual-ness, but I'd have to do a bit of poking
> to confirm that. In any event, that's not what you want to change right
> now.
>
> Instead of adding another block that does more or less the same
> thing, I'd suggest updating this logic to include tests for C_D, C, or D
> states, and enabling the ports if that is the case. Probably something
> like (I have not tested or compiled this at all):
>
> if (active) {
> if (!__agg_has_partner(active)) {
> [ ... the current !__agg_has_partner() stuff ]
> } else {
moving it here will run this part on every call of ad_agg_selection_logic(),
but it would be only relevant, if there is a switch to a different aggregator.
> for (port = active->lag_ports; port;
> port = port->next_port_in_aggregator) {
> switch (port->sm_mux_state) {
> case AD_MUX_DISTRIBUTING:
> case AD_MUX_COLLECTING_DISTRIBUTING:
> ad_enable_collecting_distributing(port,
> update_slave_arr);
> port->ntt = true;
> break;
> case AD_MUX_COLLECTING:
> ad_enable_collecting(port);
> ad_disable_distributing(port, update_slave_arr);
> port->ntt = true;
> break;
> default:
> break;
> }
I've tried this in my test environment and it doesn't fixed the issue
I'm seeing, because the port of the new aggregator is still in AD_MUX_WAITING...
The issue is that after bringing the bond up it happens that the bond link
is up, but no slave can transmit. This happens exactly when the aggregator
is changed due to timing of the received lacpdu. So if enabling the port
in AD_MUX_WAITING is wrong, what are other ways to fix this problem ?
Thomas.
--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich
Thomas Bogendoerfer <[email protected]> wrote:
>On Mon, 08 Apr 2024 09:06:11 -0700
>Jay Vosburgh <[email protected]> wrote:
>
>> Thomas Bogendoerfer <[email protected]> wrote:
>>
>> >If selection logic decides to switch to a new aggregator it disables
>> >all ports of the old aggregator, but doesn't enable ports on
>> >the new aggregator. These ports will eventually be enabled when
>> >the next LACPDU is received, which might take some time and without an
>> >active port transmitted frames are dropped. Avoid this by enabling
>> >already collected ports of the new aggregator immediately.
>> >
>> >Signed-off-by: Thomas Bogendoerfer <[email protected]>
>> >---
>> > drivers/net/bonding/bond_3ad.c | 7 +++++++
>> > 1 file changed, 7 insertions(+)
>> >
>> >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> >index c6807e473ab7..529e2a7c51e2 100644
>> >--- a/drivers/net/bonding/bond_3ad.c
>> >+++ b/drivers/net/bonding/bond_3ad.c
>> >@@ -1876,6 +1876,13 @@ static void ad_agg_selection_logic(struct aggregator *agg,
>> > __disable_port(port);
>> > }
>> > }
>> >+
>> >+ /* enable ports on new active aggregator */
>> >+ for (port = best->lag_ports; port;
>> >+ port = port->next_port_in_aggregator) {
>> >+ __enable_port(port);
>> >+ }
>> >+
>>
>> I think this will do the wrong thing if the port in question is
>> not in a valid state to send or receive (i.e., it is not one of
>> COLLECTING_DISTRIBUTING, COLLECTING, or DISTRIBUTING).
>>
>>
>> As it happens, this situation, except for the case of individual
>> ports, is handled just below this code:
>>
>> /* if the selected aggregator is of join individuals
>> * (partner_system is NULL), enable their ports
>> */
>> active = __get_active_agg(origin);
>>
>> if (active) {
>> if (!__agg_has_partner(active)) {
>> for (port = active->lag_ports; port;
>> port = port->next_port_in_aggregator) {
>> __enable_port(port);
>> }
>> *update_slave_arr = true;
>> }
>> }
>>
>> rcu_read_unlock();
>>
>> FWIW, looking at it, I'm not sure that "__agg_has_partner" is
>> the proper test for invididual-ness, but I'd have to do a bit of poking
>> to confirm that. In any event, that's not what you want to change right
>> now.
>>
>> Instead of adding another block that does more or less the same
>> thing, I'd suggest updating this logic to include tests for C_D, C, or D
>> states, and enabling the ports if that is the case. Probably something
>> like (I have not tested or compiled this at all):
>>
>> if (active) {
>> if (!__agg_has_partner(active)) {
>> [ ... the current !__agg_has_partner() stuff ]
>> } else {
>
>moving it here will run this part on every call of ad_agg_selection_logic(),
>but it would be only relevant, if there is a switch to a different aggregator.
True; that could be tested for, though, as the original
aggregator is stored in the variable "origin". This is probably moot in
light of my comments below.
>> for (port = active->lag_ports; port;
>> port = port->next_port_in_aggregator) {
>> switch (port->sm_mux_state) {
>> case AD_MUX_DISTRIBUTING:
>> case AD_MUX_COLLECTING_DISTRIBUTING:
>> ad_enable_collecting_distributing(port,
>> update_slave_arr);
>> port->ntt = true;
>> break;
>> case AD_MUX_COLLECTING:
>> ad_enable_collecting(port);
>> ad_disable_distributing(port, update_slave_arr);
>> port->ntt = true;
>> break;
>> default:
>> break;
>> }
>
>I've tried this in my test environment and it doesn't fixed the issue
>I'm seeing, because the port of the new aggregator is still in AD_MUX_WAITING...
>
>The issue is that after bringing the bond up it happens that the bond link
>is up, but no slave can transmit. This happens exactly when the aggregator
>is changed due to timing of the received lacpdu. So if enabling the port
>in AD_MUX_WAITING is wrong, what are other ways to fix this problem ?
Ok, I've looked through the code a bit more and I understand at
least some of what's going on. I recall testing this some years ago to
insure that failover between aggregators functions correctly, although I
don't recall looking into loss rates during the failover.
First, I'm not sure why your port is in WAITING state, unless
it's simply that your test is happening very quickly after the port is
added to the bond. The standard (IEEE 802.1AX-2014 6.4.15) requires
ports to remain in WAITING state for 2 seconds when transitioning from
DETACHED to ATTACHED state (to limit thrashing when multiple ports are
added in a short span of time).
You mention the issue happens when the aggregator changes; do
you have a detailed sequence of events that describe how the issue is
induced?
I also see a potential issue in the handling of READY_N and
READY, although I'd need your test case to determine if it's an actual
problem or just something that looks odd but behaves correctly.
As for the rest, if your issue revolves around failover between
aggregators in an established bond, then I'd expect the ports to remain
in ATTACHED state when their aggregator is not the active aggregator, as
the state machine logic in ad_mux_machine() won't advance beyond
ATTACHED state in this case, e.g.,
static void ad_mux_machine(struct port *port, bool *update_slave_arr)
{
[...]
case AD_MUX_ATTACHED:
[...]
if (port->aggregator->is_active) {
int state = AD_MUX_COLLECTING_DISTRIBUTING;
When an aggregator's ports move to COLLECTING, DISTRIBUTING or
COLLECTING_DISTRIBUTING state, the link partner will logically expect
that it may send and receive traffic across the ports in the aggregator.
The standard permits an arbitrary number of aggregators to be active
simultaneously, but bonding isn't able to operate more than one
aggregator at a time within the context of a single bonding interface.
If this is the crux of the problem, we could potentially change
the state machine logic to run the complete state machine on all ports.
This would need to insure that the "inactive" flag logic works correctly
if ports of an inactive aggregator are in C, D or C_D state. This
should operate similarly to how the inactive bond interfaces are treated
in active-backup mode. The LACPDU packets should already be passed
through by bond_handle_frame(), so the question would really be whether
ordinary traffic is handled correctly on the inactive aggregators.
I think the main code change would largely be removing most or
all of the tests (like the sample above) against aggregator->is_active
in ad_mux_machine(), ad_enable_collecting(), and
ad_enable_collecting_distributing(). I haven't tested this at all, this
is just my speculation from looking at the code.
-J
---
-Jay Vosburgh, [email protected]
On Thu, Apr 04, 2024 at 01:49:08PM +0200, Thomas Bogendoerfer wrote:
> If selection logic decides to switch to a new aggregator it disables
> all ports of the old aggregator, but doesn't enable ports on
> the new aggregator. These ports will eventually be enabled when
> the next LACPDU is received, which might take some time and without an
> active port transmitted frames are dropped. Avoid this by enabling
> already collected ports of the new aggregator immediately.
>
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
> ---
> drivers/net/bonding/bond_3ad.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index c6807e473ab7..529e2a7c51e2 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -1876,6 +1876,13 @@ static void ad_agg_selection_logic(struct aggregator *agg,
> __disable_port(port);
> }
> }
> +
> + /* enable ports on new active aggregator */
> + for (port = best->lag_ports; port;
> + port = port->next_port_in_aggregator) {
> + __enable_port(port);
> + }
> +
> /* Slave array needs update. */
> *update_slave_arr = true;
> }
> --
> 2.35.3
>
Reviewed-by: Hangbin Liu <[email protected]>
On Wed, 10 Apr 2024 17:28:29 -0700
Jay Vosburgh <[email protected]> wrote:
> First, I'm not sure why your port is in WAITING state, unless
> it's simply that your test is happening very quickly after the port is
> added to the bond. The standard (IEEE 802.1AX-2014 6.4.15) requires
> ports to remain in WAITING state for 2 seconds when transitioning from
> DETACHED to ATTACHED state (to limit thrashing when multiple ports are
> added in a short span of time).
>
> You mention the issue happens when the aggregator changes; do
> you have a detailed sequence of events that describe how the issue is
> induced?
setup is one Linux server with 2 dual port ethernet cards connected to
a HP 5710 Flexfabric switch with two modules. Using MC-LAG is probably the
key to trigger the issue, at least I couldn't reproduce without it.
1. create bond0
2. enslave 4 ports to it
3. wait for link up
4. do duplicate address detection
most of the time this works without problems, but in the error case
DAD fails with an ENOBUFS for the send call to the packet socket,
which correlates with the tx dropped in the bond statistic counters.
I've enabled debug print for the ad_agg_selection_logic() and in
error case the look like this:
[ 4488.603417] bond0: (slave eth6): best Agg=1; P=1; a k=0; p k=1; Ind=1; Act=0
[ 4488.603428] bond0: (slave eth6): best ports 0000000019ca9537 slave 00000000ee0c58b9
[ 4488.603433] bond0: (slave eth6): Agg=1; P=1; a k=0; p k=1; Ind=1; Act=0
[ 4488.603437] bond0: (slave eth7): Agg=2; P=0; a k=0; p k=0; Ind=0; Act=0
[ 4488.603441] bond0: (slave eth8): Agg=3; P=0; a k=0; p k=0; Ind=0; Act=0
[ 4488.603444] bond0: (slave eth9): Agg=4; P=0; a k=0; p k=0; Ind=0; Act=0
[ 4488.603447] bond0: Warning: No 802.3ad response from the link partner for any adapters in the bond
[ 4488.603449] bond0: (slave eth6): LAG 1 chosen as the active LAG
[ 4488.603452] bond0: (slave eth6): Agg=1; P=1; a k=0; p k=1; Ind=1; Act=1
[ 4488.610481] 8021q: adding VLAN 0 to HW filter on device bond0
[ 4488.618756] bond0: (slave eth6): link status definitely up, 10000 Mbps full duplex
[ 4488.618795] bond0: (slave eth7): link status definitely up, 10000 Mbps full duplex
[ 4488.618831] bond0: (slave eth8): link status definitely up, 10000 Mbps full duplex
[ 4488.618836] bond0: active interface up!
[ 4488.678822] ixgbe 0000:81:00.1 eth9: detected SFP+: 6
[ 4488.706715] bond0: (slave eth6): best Agg=1; P=1; a k=15; p k=1; Ind=0; Act=0
[ 4488.706726] bond0: (slave eth6): best ports 0000000019ca9537 slave 00000000ee0c58b9
[ 4488.706732] bond0: (slave eth6): Agg=1; P=1; a k=15; p k=1; Ind=0; Act=0
[ 4488.706737] bond0: (slave eth7): Agg=2; P=1; a k=0; p k=1; Ind=1; Act=0
[ 4488.706740] bond0: (slave eth8): Agg=3; P=1; a k=0; p k=1; Ind=1; Act=0
[ 4488.706744] bond0: (slave eth9): Agg=4; P=1; a k=0; p k=1; Ind=1; Act=0
[ 4488.706747] bond0: (slave eth6): LAG 1 chosen as the active LAG
[ 4488.706750] bond0: (slave eth6): Agg=1; P=1; a k=15; p k=1; Ind=0; Act=1
[ 4488.814731] ixgbe 0000:81:00.1 eth9: NIC Link is Up 10 Gbps, Flow Control: RX/TX
[ 4488.826760] bond0: (slave eth9): link status definitely up, 10000 Mbps full duplex
[ 4488.914672] bond0: (slave eth7): best Agg=2; P=1; a k=15; p k=1; Ind=0; Act=0
[ 4488.914682] bond0: (slave eth7): best ports 00000000413bcc63 slave 00000000931f59f6
[ 4488.914687] bond0: (slave eth6): Agg=1; P=1; a k=15; p k=1; Ind=0; Act=0
[ 4488.914692] bond0: (slave eth7): Agg=2; P=1; a k=15; p k=1; Ind=0; Act=0
[ 4488.914695] bond0: (slave eth8): Agg=3; P=1; a k=15; p k=1; Ind=0; Act=0
[ 4488.914698] bond0: (slave eth9): Agg=4; P=1; a k=0; p k=1; Ind=1; Act=0
[ 4488.914701] bond0: (slave eth7): LAG 2 chosen as the active LAG
[ 4488.914704] bond0: (slave eth7): Agg=2; P=1; a k=15; p k=1; Ind=0; Act=1
I've added a debug statement to find out why Agg 2 is better than Agg 1 in
this case and it's because Agg 2 has a partner (__agg_has_partner() is true)
while Agg 1 doesn't.
Wouldn't it make sense to also check for slaves in COLLECTING|DISTRIBUTING
state before switching to a new aggregator ?
Thomas.
--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich