Received-SPF: pass (google.com: domain of linux-kernel+bounces-138912-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 10 Apr 2024 17:50:52 +0200
From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
To: Jay Vosburgh <jay.vosburgh@canonical.com>
Cc: Andy Gospodarek <andy@greyhouse.net>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
 <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] bonding: 802.3ad: Avoid packet loss when switching
 aggregator
Message-ID: <20240410175052.25ac7638@samweis>
In-Reply-To: <21529.1712592371@famine>
References: <20240404114908.134034-1-tbogendoerfer@suse.de>
	<21529.1712592371@famine>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, 08 Apr 2024 09:06:11 -0700
Jay Vosburgh <jay.vosburgh@canonical.com> wrote:

> Thomas Bogendoerfer <tbogendoerfer@suse.de> wrote:
>=20
> >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 <tbogendoerfer@suse.de>
> >---
> > 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_3=
ad.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 aggrega=
tor *agg,
> > 				__disable_port(port);
> > 			}
> > 		}
> >+
> >+		/* enable ports on new active aggregator */
> >+		for (port =3D best->lag_ports; port;
> >+			port =3D port->next_port_in_aggregator) {
> >+			__enable_port(port);
> >+		}
> >+ =20
>=20
> 	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).
>=20
>=20
> 	As it happens, this situation, except for the case of individual
> ports, is handled just below this code:
>=20
> 	/* if the selected aggregator is of join individuals
> 	 * (partner_system is NULL), enable their ports
> 	 */
> 	active =3D __get_active_agg(origin);
>=20
> 	if (active) {
> 		if (!__agg_has_partner(active)) {
> 			for (port =3D active->lag_ports; port;
> 			     port =3D port->next_port_in_aggregator) {
> 				__enable_port(port);
> 			}
> 			*update_slave_arr =3D true;
> 		}
> 	}
>=20
> 	rcu_read_unlock();
>=20
> 	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.
>=20
> 	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):
>=20
> 	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 aggregat=
or.

> 			for (port =3D active->lag_ports; port;
> 			     port =3D 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 =3D true;
> 					break;
> 				case AD_MUX_COLLECTING:
> 					ad_enable_collecting(port);
> 					ad_disable_distributing(port, update_slave_arr);
> 					port->ntt =3D 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_WAITI=
NG...

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.

--=20
SUSE Software Solutions Germany GmbH
HRB 36809 (AG N=C3=BCrnberg)
Gesch=C3=A4ftsf=C3=BChrer: Ivo Totev, Andrew McDonald, Werner Knoblich