Hangbin Liu <[email protected]> wrote:
>On Tue, Jul 15, 2025 at 09:19:49PM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <[email protected]> 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, [email protected]
