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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d8c482c-cf4f-4527-8edb-a850d1d538ac@gmail.com>
Date: Wed, 16 Jul 2025 14:45:23 -0500
From: Carlos Bilbao <carlos.bilbao.osdev@...il.com>
To: Simon Horman <horms@...nel.org>, carlos.bilbao@...nel.org
Cc: jv@...sburgh.net, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, sforshee@...nel.org,
 bilbao@...edu
Subject: Re: [PATCH] bonding: Switch periodic LACPDU state machine from
 counter to jiffies

Hello Simon,

On 7/16/25 04:09, Simon Horman wrote:
> On Tue, Jul 15, 2025 at 03:57:33PM -0500, carlos.bilbao@...nel.org wrote:
>> From: Carlos Bilbao <carlos.bilbao@...nel.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@...nel.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