On 7/25/25 09:28, Hangbin Liu wrote:
> When `lacp_active` is set to `off`, the bond operates in passive mode, meaning
> it only "speaks when spoken to." However, the current kernel implementation
> only sends an LACPDU in response when the partner's state changes.
>
> As a result, once LACP negotiation succeeds, the actor stops sending LACPDUs
> until the partner times out and sends an "expired" LACPDU. This causes
> continuous LACP state flapping.
>
> According to IEEE 802.1AX-2014, 6.4.13 Periodic Transmission machine. The
> values of Partner_Oper_Port_State.LACP_Activity and
> Actor_Oper_Port_State.LACP_Activity determine whether periodic transmissions
> take place. If either or both parameters are set to Active LACP, then periodic
> transmissions occur; if both are set to Passive LACP, then periodic
> transmissions do not occur.
>
> To comply with this, we remove the `!bond->params.lacp_active` check in
> `ad_periodic_machine()`. Instead, we initialize the actor's port's
> `LACP_STATE_LACP_ACTIVITY` state based on `lacp_active` setting.
>
> Additionally, we avoid setting the partner's state to
> `LACP_STATE_LACP_ACTIVITY` in the EXPIRED state, since we should not assume
> the partner is active by default.
>
> This ensures that in passive mode, the bond starts sending periodic LACPDUs
> after receiving one from the partner, and avoids flapping due to inactivity.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Shouldn't the fixes tag be lacp_active commit id?
E.g. 3a755cd8b7c6 ("bonding: add new option lacp_active")
> Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxx>
> ---
> drivers/net/bonding/bond_3ad.c | 72 ++++++++++++++++++++++--------
> drivers/net/bonding/bond_options.c | 1 +
> include/net/bond_3ad.h | 1 +
> 3 files changed, 56 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 2fca8e84ab10..aa64b397bb19 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -95,13 +95,13 @@ static int ad_marker_send(struct port *port, struct bond_marker *marker);
> static void ad_mux_machine(struct port *port, bool *update_slave_arr);
> static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port);
> static void ad_tx_machine(struct port *port);
> -static void ad_periodic_machine(struct port *port, struct bond_params *bond_params);
> +static void ad_periodic_machine(struct port *port);
> static void ad_port_selection_logic(struct port *port, bool *update_slave_arr);
> static void ad_agg_selection_logic(struct aggregator *aggregator,
> bool *update_slave_arr);
> static void ad_clear_agg(struct aggregator *aggregator);
> static void ad_initialize_agg(struct aggregator *aggregator);
> -static void ad_initialize_port(struct port *port, int lacp_fast);
> +static void ad_initialize_port(struct port *port, struct bond_params *bond_params);
> static void ad_enable_collecting(struct port *port);
> static void ad_disable_distributing(struct port *port,
> bool *update_slave_arr);
> @@ -1307,10 +1307,16 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> * case of EXPIRED even if LINK_DOWN didn't arrive for
> * the port.
> */
> - port->partner_oper.port_state &= ~LACP_STATE_SYNCHRONIZATION;
> port->sm_vars &= ~AD_PORT_MATCHED;
> + /* Based on IEEE 8021AX-2014, Figure 6-18 - Receive
> + * machine state diagram, the statue should be
> + * Partner_Oper_Port_State.Synchronization = FALSE;
> + * Partner_Oper_Port_State.LACP_Timeout = Short Timeout;
> + * start current_while_timer(Short Timeout);
> + * Actor_Oper_Port_State.Expired = TRUE;
> + */
> + port->partner_oper.port_state &= ~LACP_STATE_SYNCHRONIZATION;
> port->partner_oper.port_state |= LACP_STATE_LACP_TIMEOUT;
> - port->partner_oper.port_state |= LACP_STATE_LACP_ACTIVITY;
> port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(AD_SHORT_TIMEOUT));
> port->actor_oper_port_state |= LACP_STATE_EXPIRED;
> port->sm_vars |= AD_PORT_CHURNED;
> @@ -1417,11 +1423,10 @@ static void ad_tx_machine(struct port *port)
> /**
> * ad_periodic_machine - handle a port's periodic state machine
> * @port: the port we're looking at
> - * @bond_params: bond parameters we will use
> *
> * Turn ntt flag on priodically to perform periodic transmission of lacpdu's.
> */
> -static void ad_periodic_machine(struct port *port, struct bond_params *bond_params)
> +static void ad_periodic_machine(struct port *port)
> {
> periodic_states_t last_state;
>
> @@ -1430,8 +1435,7 @@ static void ad_periodic_machine(struct port *port, struct bond_params *bond_para
>
> /* 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->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY))) {
> port->sm_periodic_state = AD_NO_PERIODIC;
> }
> /* check if state machine should change state */
> @@ -1955,16 +1959,16 @@ static void ad_initialize_agg(struct aggregator *aggregator)
> /**
> * ad_initialize_port - initialize a given port's parameters
> * @port: the port we're looking at
> - * @lacp_fast: boolean. whether fast periodic should be used
> + * @bond_params: bond parameters we will use
> */
> -static void ad_initialize_port(struct port *port, int lacp_fast)
> +static void ad_initialize_port(struct port *port, struct bond_params *bond_params)
> {
> static const struct port_params tmpl = {
> .system_priority = 0xffff,
> .key = 1,
> .port_number = 1,
> .port_priority = 0xff,
> - .port_state = 1,
> + .port_state = 0,
1 == LACP_STATE_ACTIVITY :)
Just noting that this lets LACP_STATE_ACTIVITY to be set based on lacp_active.
> };
> static const struct lacpdu lacpdu = {
> .subtype = 0x01,
> @@ -1982,12 +1986,14 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
> port->actor_port_priority = 0xff;
> port->actor_port_aggregator_identifier = 0;
> port->ntt = false;
> - port->actor_admin_port_state = LACP_STATE_AGGREGATION |
> - LACP_STATE_LACP_ACTIVITY;
> - port->actor_oper_port_state = LACP_STATE_AGGREGATION |
> - LACP_STATE_LACP_ACTIVITY;
> + port->actor_admin_port_state = LACP_STATE_AGGREGATION;
> + port->actor_oper_port_state = LACP_STATE_AGGREGATION;
> + if (bond_params->lacp_active) {
> + port->actor_admin_port_state |= LACP_STATE_LACP_ACTIVITY;
> + port->actor_oper_port_state |= LACP_STATE_LACP_ACTIVITY;
> + }
>
> - if (lacp_fast)
> + if (bond_params->lacp_fast)
> port->actor_oper_port_state |= LACP_STATE_LACP_TIMEOUT;
>
> memcpy(&port->partner_admin, &tmpl, sizeof(tmpl));
> @@ -2201,7 +2207,7 @@ void bond_3ad_bind_slave(struct slave *slave)
> /* port initialization */
> port = &(SLAVE_AD_INFO(slave)->port);
>
> - ad_initialize_port(port, bond->params.lacp_fast);
> + ad_initialize_port(port, &bond->params);
>
> port->slave = slave;
> port->actor_port_number = SLAVE_AD_INFO(slave)->id;
> @@ -2513,7 +2519,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> }
>
> ad_rx_machine(NULL, port);
> - ad_periodic_machine(port, &bond->params);
> + ad_periodic_machine(port);
> ad_port_selection_logic(port, &update_slave_arr);
> ad_mux_machine(port, &update_slave_arr);
> ad_tx_machine(port);
> @@ -2883,6 +2889,36 @@ void bond_3ad_update_lacp_rate(struct bonding *bond)
> spin_unlock_bh(&bond->mode_lock);
> }
>
> +/**
> + * bond_3ad_update_lacp_active - change the lacp active
> + * @bond: bonding struct
> + *
> + * When modify lacp_active parameter via sysfs,
> + * update actor_oper_port_state of each port.
It could also be netlink, right? I'd just say something like:
Update actor_oper_port_state when lacp_active is modified.
> + *
> + * Hold bond->mode_lock,
> + * so we can modify port->actor_oper_port_state,
> + * no matter bond is up or down.
nit: this last part about bond up/down is unnecessary
> + */
> +void bond_3ad_update_lacp_active(struct bonding *bond)
> +{
> + struct port *port = NULL;
> + struct list_head *iter;
> + struct slave *slave;
> + int lacp_active;
> +
> + lacp_active = bond->params.lacp_active;
> + spin_lock_bh(&bond->mode_lock);
> + bond_for_each_slave(bond, slave, iter) {
> + port = &(SLAVE_AD_INFO(slave)->port);
> + if (lacp_active)
> + port->actor_oper_port_state |= LACP_STATE_LACP_ACTIVITY;
> + else
> + port->actor_oper_port_state &= ~LACP_STATE_LACP_ACTIVITY;
> + }
> + spin_unlock_bh(&bond->mode_lock);
> +}
> +
> size_t bond_3ad_stats_size(void)
> {
> return nla_total_size_64bit(sizeof(u64)) + /* BOND_3AD_STAT_LACPDU_RX */
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 1d639a3be6ba..3b6f815c55ff 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1660,6 +1660,7 @@ static int bond_option_lacp_active_set(struct bonding *bond,
> netdev_dbg(bond->dev, "Setting LACP active to %s (%llu)\n",
> newval->string, newval->value);
> bond->params.lacp_active = newval->value;
> + bond_3ad_update_lacp_active(bond);
To me it looks like this fix needs to be in a separate patch.
>
> return 0;
> }
> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
> index 2053cd8e788a..dba369a2cf27 100644
> --- a/include/net/bond_3ad.h
> +++ b/include/net/bond_3ad.h
> @@ -307,6 +307,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
> struct slave *slave);
> int bond_3ad_set_carrier(struct bonding *bond);
> void bond_3ad_update_lacp_rate(struct bonding *bond);
> +void bond_3ad_update_lacp_active(struct bonding *bond);
> void bond_3ad_update_ad_actor_settings(struct bonding *bond);
> int bond_3ad_stats_fill(struct sk_buff *skb, struct bond_3ad_stats *stats);
> size_t bond_3ad_stats_size(void);
