Hangbin Liu liuhangbin@gmail.com wrote:
On Tue, Jul 15, 2025 at 09:19:49PM -0700, Jay Vosburgh wrote:
Hangbin Liu liuhangbin@gmail.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@jvosburgh.net