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

[Carlos Bilbao <carlos.bilbao.osdev@gmail.com>]

Hello Simon,

On 7/16/25 04:09, Simon Horman wrote:
> On Tue, Jul 15, 2025 at 03:57:33PM -0500, 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(-)
>>
> Hi Carlos,
>
> Some minor feedback from my side.
>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> ...
>
>> @@ -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;
> super-nit: maybe one more tab of indentation for the line above.
>
>> +		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;
> Clang 20.1.8 complains that either a break; or fallthrough; should go here.
> For consistency with the code above I'd suggest the latter.


Thank you for the feedback! I’ll be sure to address it in v2, if we decide
to move forward with this.


>
>
>>   		default:
>>   			break;
>>   		}
> ...


Thanks,

Carlos