Re: [PATCH net 2/2] bonding: 3ad: fix mux port state on oper down

[Jay Vosburgh <jv@jvosburgh.net>]

Louis Scalbert <louis.scalbert@6wind.com> wrote:

>When the bonding interface has carrier down due to the absence of
>valid slaves and a slave transitions from down to up, the bonding
>interface briefly goes carrier up, then down again, and finally up
>once LACP negotiates collecting and distributing on the port.
>
>The interface should not transition to carrier up until LACP
>negotiation is complete.
>
>This happens because the actor and partner port states remain in
>collecting (and distributing) when the port goes down. When the port
>comes back up, it temporarily remains in this state until LACP
>renegotiation occurs.
>
>Previously this was mostly cosmetic, but since the bonding carrier
>state now depends on the LACP negotiation state, it causes the
>interface to flap.
>
>Fix this by unsetting the SELECTED flag when a port goes down so that
>the mux state machine transitions through ATTACHED and DETACHED,
>which clears the actor collecting and distributing flags.
>
>Fixes: 655f8919d549 ("bonding: add min links parameter to 802.3ad")
>Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
>---
> drivers/net/bonding/bond_3ad.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 7d7661972c5e..367d89977000 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -716,6 +716,8 @@ static int __agg_ports_are_ready(struct aggregator *aggregator)
> 		for (port = aggregator->lag_ports;
> 		     port;
> 		     port = port->next_port_in_aggregator) {
>+			if (!port->is_enabled)
>+				continue;

	Your commit message mentions clearing SELECTED, further below,
but not the new two tests for port->is_enabled.  In this change,
__agg_ports_are_ready is testing that all ports of an aggregator are
READY_N, but if a port of the aggregator is not is_enabled, then
logically it would not be READY_N, either.  So why exclude it here?

	Is a this is an optimization to settle the aggregator state more
quickly, as an !is_enabled port is likely to be transitioning out of the
aggregator?

> 			if (!(port->sm_vars & AD_PORT_READY_N)) {
> 				retval = 0;
> 				break;
>@@ -1570,6 +1572,9 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
> 	struct slave *slave;
> 	int found = 0;
> 
>+	if (!port->is_enabled)
>+		return;
>+

	Same question as above, why exclude !is_enabled ports here?

	-J

> 	/* if the port is already Selected, do nothing */
> 	if (port->sm_vars & AD_PORT_SELECTED)
> 		return;
>@@ -2794,6 +2799,7 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
> 		/* link has failed */
> 		port->is_enabled = false;
> 		ad_update_actor_keys(port, true);
>+		port->sm_vars &= ~AD_PORT_SELECTED;
> 	}
> 	agg = __get_first_agg(port);
> 	ad_agg_selection_logic(agg, &dummy);
>-- 
>2.39.2
>

---
	-Jay Vosburgh, jv@jvosburgh.net