Re: [PATCH] bonding: Switch periodic LACPDU state machine from counter to jiffies

[Jay Vosburgh <jv@jvosburgh.net>]

Carlos Bilbao <bilbao@vt.edu> wrote:

>FYI, I was able to test this locally but couldn’t find any kselftests to
>stress the bonding state machine. If anyone knows of additional ways to
>test it, I’d be happy to run them.

	Your commit message says this change will "help reduce drift
under contention," but above you say you're unable to stress the state
machine.

	How do you induce "drift under contention" to test that your
patch actually improves something?  What testing has been done to insure
that the new code doesn't change the behavior in other ways (regressions)?

	Without a specific reproducable bug scenario that this change
fixes, I'm leery of applying such a refactor to code that has seemingly
been working fine for 20+ years.

	I gather that what this is intending to do is reduce the current
dependency on the scheduling accuracy of the workqueue event that runs
the state machines.  The current implementation works on a "number of
invocations" basis, assuming that the event is invoked every 100 msec,
and computes various timeouts based on the number of times the state
machine runs.

	-J

>Thanks!
>
>Carlos
>
>On 7/15/25 15:57, carlos.bilbao@kernel.org wrote:
>> From: Carlos Bilbao <carlos.bilbao@kernel.org>
>>
>> Replace the bonding periodic state machine for LACPDU transmission of
>> function ad_periodic_machine() with a jiffies-based mechanism, which is
>> more accurate and can help reduce drift under contention.
>>
>> Signed-off-by: Carlos Bilbao (DigitalOcean) <carlos.bilbao@kernel.org>
>> ---
>>   drivers/net/bonding/bond_3ad.c | 79 +++++++++++++---------------------
>>   include/net/bond_3ad.h         |  2 +-
>>   2 files changed, 32 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index c6807e473ab7..8654a51266a3 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -1421,44 +1421,24 @@ static void ad_periodic_machine(struct port *port, struct bond_params *bond_para
>>   	    (!(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;
>> -	}
>> -	/* check if state machine should change state */
>> -	else if (port->sm_periodic_timer_counter) {
>> -		/* check if periodic state machine expired */
>> -		if (!(--port->sm_periodic_timer_counter)) {
>> -			/* if expired then do tx */
>> -			port->sm_periodic_state = AD_PERIODIC_TX;
>> -		} else {
>> -			/* If not expired, check if there is some new timeout
>> -			 * parameter from the partner state
>> -			 */
>> -			switch (port->sm_periodic_state) {
>> -			case AD_FAST_PERIODIC:
>> -				if (!(port->partner_oper.port_state
>> -				      & LACP_STATE_LACP_TIMEOUT))
>> -					port->sm_periodic_state = AD_SLOW_PERIODIC;
>> -				break;
>> -			case AD_SLOW_PERIODIC:
>> -				if ((port->partner_oper.port_state & LACP_STATE_LACP_TIMEOUT)) {
>> -					port->sm_periodic_timer_counter = 0;
>> -					port->sm_periodic_state = AD_PERIODIC_TX;
>> -				}
>> -				break;
>> -			default:
>> -				break;
>> -			}
>> -		}
>> +	} else if (port->sm_periodic_state == AD_NO_PERIODIC)
>> +		port->sm_periodic_state = AD_FAST_PERIODIC;
>> +	/* check if periodic state machine expired */
>> +	else if (time_after_eq(jiffies, port->sm_periodic_next_jiffies)) {
>> +		/* if expired then do tx */
>> +		port->sm_periodic_state = AD_PERIODIC_TX;
>>   	} else {
>> +		/* If not expired, check if there is some new timeout
>> +		 * parameter from the partner state
>> +		 */
>>   		switch (port->sm_periodic_state) {
>> -		case AD_NO_PERIODIC:
>> -			port->sm_periodic_state = AD_FAST_PERIODIC;
>> -			break;
>> -		case AD_PERIODIC_TX:
>> -			if (!(port->partner_oper.port_state &
>> -			    LACP_STATE_LACP_TIMEOUT))
>> +		case AD_FAST_PERIODIC:
>> +			if (!(port->partner_oper.port_state & LACP_STATE_LACP_TIMEOUT))
>>   				port->sm_periodic_state = AD_SLOW_PERIODIC;
>> -			else
>> -				port->sm_periodic_state = AD_FAST_PERIODIC;
>> +			break;
>> +		case AD_SLOW_PERIODIC:
>> +			if ((port->partner_oper.port_state & LACP_STATE_LACP_TIMEOUT))
>> +				port->sm_periodic_state = AD_PERIODIC_TX;
>>   			break;
>>   		default:
>>   			break;
>> @@ -1471,21 +1451,24 @@ static void ad_periodic_machine(struct port *port, struct bond_params *bond_para
>>   			  "Periodic Machine: Port=%d, Last State=%d, Curr State=%d\n",
>>   			  port->actor_port_number, last_state,
>>   			  port->sm_periodic_state);
>> +
>>   		switch (port->sm_periodic_state) {
>> -		case AD_NO_PERIODIC:
>> -			port->sm_periodic_timer_counter = 0;
>> -			break;
>> -		case AD_FAST_PERIODIC:
>> -			/* decrement 1 tick we lost in the PERIODIC_TX cycle */
>> -			port->sm_periodic_timer_counter = __ad_timer_to_ticks(AD_PERIODIC_TIMER, (u16)(AD_FAST_PERIODIC_TIME))-1;
>> -			break;
>> -		case AD_SLOW_PERIODIC:
>> -			/* decrement 1 tick we lost in the PERIODIC_TX cycle */
>> -			port->sm_periodic_timer_counter = __ad_timer_to_ticks(AD_PERIODIC_TIMER, (u16)(AD_SLOW_PERIODIC_TIME))-1;
>> -			break;
>>   		case AD_PERIODIC_TX:
>>   			port->ntt = true;
>> -			break;
>> +			if (!(port->partner_oper.port_state &
>> +						LACP_STATE_LACP_TIMEOUT))
>> +				port->sm_periodic_state = AD_SLOW_PERIODIC;
>> +			else
>> +				port->sm_periodic_state = AD_FAST_PERIODIC;
>> +		fallthrough;
>> +		case AD_SLOW_PERIODIC:
>> +		case AD_FAST_PERIODIC:
>> +			if (port->sm_periodic_state == AD_SLOW_PERIODIC)
>> +				port->sm_periodic_next_jiffies = jiffies
>> +					+ HZ * AD_SLOW_PERIODIC_TIME;
>> +			else /* AD_FAST_PERIODIC */
>> +				port->sm_periodic_next_jiffies = jiffies
>> +					+ HZ * AD_FAST_PERIODIC_TIME;
>>   		default:
>>   			break;
>>   		}
>> @@ -1987,7 +1970,7 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
>>   		port->sm_rx_state = 0;
>>   		port->sm_rx_timer_counter = 0;
>>   		port->sm_periodic_state = 0;
>> -		port->sm_periodic_timer_counter = 0;
>> +		port->sm_periodic_next_jiffies = 0;
>>   		port->sm_mux_state = 0;
>>   		port->sm_mux_timer_counter = 0;
>>   		port->sm_tx_state = 0;
>> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
>> index 2053cd8e788a..aabb8c97caf4 100644
>> --- a/include/net/bond_3ad.h
>> +++ b/include/net/bond_3ad.h
>> @@ -227,7 +227,7 @@ typedef struct port {
>>   	rx_states_t sm_rx_state;	/* state machine rx state */
>>   	u16 sm_rx_timer_counter;	/* state machine rx timer counter */
>>   	periodic_states_t sm_periodic_state;	/* state machine periodic state */
>> -	u16 sm_periodic_timer_counter;	/* state machine periodic timer counter */
>> +	unsigned long sm_periodic_next_jiffies;	/* state machine periodic next expected sent */
>>   	mux_states_t sm_mux_state;	/* state machine mux state */
>>   	u16 sm_mux_timer_counter;	/* state machine mux timer counter */
>>   	tx_states_t sm_tx_state;	/* state machine tx state */

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