Message-ID: <367f9bbb-537b-4828-b8c8-cfc9d8ca8c2c@blackwall.org>
Date: Fri, 25 Jul 2025 11:49:00 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc: Jay Vosburgh <jv@...sburgh.net>, 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>,
 Simon Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>,
 Jonathan Corbet <corbet@....net>, Petr Machata <petrm@...dia.com>,
 Amit Cohen <amcohen@...dia.com>, Vladimir Oltean <vladimir.oltean@....com>,
 Alessandro Zanni <alessandro.zanni87@...il.com>, linux-doc@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/2] bonding: send LACPDUs periodically in passive
 mode after receiving partner's LACPDU

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@...il.com>
> ---
>  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);

Powered by blists - more mailing lists