Message-ID: <807057.1752687334@famine>
Date: Wed, 16 Jul 2025 10:35:34 -0700
From: Jay Vosburgh <jv@...sburgh.net>
To: Hangbin Liu <liuhangbin@...il.com>
cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
    "David S. Miller" <davem@...emloft.net>,
    Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
    Paolo Abeni <pabeni@...hat.com>,
    Nikolay Aleksandrov <razor@...ckwall.org>,
    Simon Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>,
    linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/2] bonding: update ntt to true in passive mode

Hangbin Liu <liuhangbin@...il.com> wrote:

>On Tue, Jul 15, 2025 at 09:19:49PM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@...il.com> wrote:
>> 
>> >When lacp_active is set to off, the bond operates in passive mode, meaning it
>> >will only "speak when spoken to." However, the current kernel implementation
>> >only sends an LACPDU in response when the partner's state changes.
>> >
>> >In this situation, once LACP negotiation succeeds, the actor stops sending
>> >LACPDUs until the partner times out and sends an "expired" LACPDU.
>> >This leads to endless LACP state flapping.
>> 
>> 	From the above, I suspect our implementation isn't compliant to
>> the standard.  Per IEEE 802.1AX-2014 6.4.1 LACP design elements:
>> 
>> c)	Active or passive participation in LACP is controlled by
>> 	LACP_Activity, an administrative control associated with each
>> 	Aggregation Port, that can take the value Active LACP or Passive
>> 	LACP. Passive LACP indicates the Aggregation Port’s preference
>> 	for not transmitting LACPDUs unless its Partner’s control value
>> 	is Active LACP (i.e., a preference not to speak unless spoken
>> 	to). Active LACP indicates the Aggregation Port’s preference to
>
>OK, so this means the passive side should start sending LACPDUs when receive
>passive actor's LACPDUs, with the slow/fast rate based on partner's rate?

	Did you mean "receive active actor's LACPDUs"?

	Regardless, the standard requires both sides to initiate
periodic LACPDU transmission if either or both enable LACP_Activity in
their LACPDUs.

	So, if a received LACPDU from the partner has LACP_Activity set,
then, yes, we would enable periodic LACPDU transmission, regardless of
our local setting of "lacp_active" / LACP_Activity.

>Hmm, then when we should stop sending LACPDUs? After
>port->sm_mux_state == AD_MUX_DETACHED ?

	We stop sending when the criteria for NO_PERIODIC in the
periodic state machine is met (IEEE 802.1AX-2014 6.4.13, Figure 6-19).

	Practically speaking, this happens when a BEGIN event occurs,
due to a port being reinitialized.  The ad_mux_machine() will set the
mux state to AD_MUX_DETACHED when BEGIN occurs, so I don't think we need
to test for DETACHED explicitly.

	The NO_PERIODIC check is the first "if" block in
ad_periodic_machine() that I referenced below.  The code currently tests
all of the criteria from Figure 6-19, but adds a test of "!lacp_active",
which is why I suspect that removing that bit and managing the
lacp_active option via the LACP_Activity in the actor port state would
do the right thing.

	-J

>> 	participate in the protocol regardless of the Partner’s control
>> 	value (i.e., a preference to speak regardless).
>> 
>> d)	Periodic transmission of LACPDUs occurs if the LACP_Activity
>> 	control of either the Actor or the Partner is Active LACP. These
>> 	periodic transmissions will occur at either a slow or fast
>> 	transmission rate depending upon the expressed LACP_Timeout
>> 	preference (Long Timeout or Short Timeout) of the Partner
>> 	System.
>> 
>> 	Which, in summary, means that if either end (actor or partner)
>> has LACP_Activity set, both ends must send periodic LACPDUs at the rate
>> specified by their respective partner's LACP_Timeout rate.
>> 
>> >To avoid this, we need update ntt to true once received an LACPDU from the
>> >partner, ensuring an immediate reply. With this fix, the link becomes stable
>> >in most cases, except for one specific scenario:
>> >
>> >Actor: lacp_active=off, lacp_rate=slow
>> >Partner: lacp_active=on, lacp_rate=fast
>> >
>> >In this case, the partner expects frequent LACPDUs (every 1 second), but the
>> >actor only responds after receiving an LACPDU, which, in this setup, the
>> >partner sends every 30 seconds due to the actor's lacp_rate=slow. By the time
>> >the actor replies, the partner has already timed out and sent an "expired"
>> >LACPDU.
>> 
>> 	Presuming that I'm correct that we're not implementing 6.4.1 d),
>> above, correctly, then I don't think this is a proper fix, as it kind of
>> band-aids over the problem a bit.
>> 
>> 	Looking at the code, I suspect the problem revolves around the
>> "lacp_active" check in ad_periodic_machine():
>> 
>> static void ad_periodic_machine(struct port *port, struct bond_params *bond_params)
>> {
>> 	periodic_states_t last_state;
>> 
>> 	/* keep current state machine state to compare later if it was changed */
>> 	last_state = port->sm_periodic_state;
>> 
>> 	/* check if port was reinitialized */
>> 	if (((port->sm_vars & AD_PORT_BEGIN) || !(port->sm_vars & AD_PORT_LACP_ENABLED) || !port->is_enabled) ||
>> 	    (!(port->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY)) ||
>> 	    !bond_params->lacp_active) {
>> 		port->sm_periodic_state = AD_NO_PERIODIC;
>> 	}
>> 
>> 	In the above, because all the tests are chained with ||, the
>> lacp_active test overrides the two correct-looking
>> LACP_STATE_LACP_ACTIVITY tests.
>> 
>> 	It looks like ad_initialize_port() always sets
>> LACP_STATE_LACP_ACTIVITY in the port->actor_oper_port_state, and nothing
>> ever clears it.
>> 
>> 	Thinking out loud, perhaps this could be fixed by
>> 
>> 	a) remove the test of bond_params->lacp_active here, and,
>> 
>> 	b) The lacp_active option setting controls whether LACP_ACTIVITY
>> is set in port->actor_oper_port_state.
>> 
>> 	Thoughts?
>
>As the upper question. When should we stop sending the LACPDUs?
>
>Thanks
>Hangbin

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

Powered by blists - more mailing lists