netdev - Re: [PATCH net 2/3] bonding: restructure ad_churn

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: <7f7238d8-bf0d-43f3-8474-7798e8b18090@redhat.com>
Date: Thu, 27 Nov 2025 16:29:47 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, 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>, Simon Horman <horms@...nel.org>,
 Mahesh Bandewar <maheshb@...gle.com>, Shuah Khan <shuah@...nel.org>,
 linux-kselftest@...r.kernel.org, Liang Li <liali@...hat.com>
Subject: Re: [PATCH net 2/3] bonding: restructure ad_churn_machine

On 11/27/25 3:06 PM, Hangbin Liu wrote:
> On Thu, Nov 27, 2025 at 11:36:43AM +0100, Paolo Abeni wrote:
>> On 11/24/25 5:33 AM, Hangbin Liu wrote:
>>>  static void ad_churn_machine(struct port *port)
>>>  {
>>> -	if (port->sm_vars & AD_PORT_CHURNED) {
>>> +	bool partner_synced = port->partner_oper.port_state & LACP_STATE_SYNCHRONIZATION;
>>> +	bool actor_synced = port->actor_oper_port_state & LACP_STATE_SYNCHRONIZATION;
>>> +	bool partner_churned = port->sm_vars & AD_PORT_PARTNER_CHURN;
>>> +	bool actor_churned = port->sm_vars & AD_PORT_ACTOR_CHURN;
>>> +
>>> +	/* ---- 1. begin or port not enabled ---- */
>>> +	if ((port->sm_vars & AD_PORT_BEGIN) || !port->is_enabled) {
>>>  		port->sm_vars &= ~AD_PORT_CHURNED;
>>> +
>>>  		port->sm_churn_actor_state = AD_CHURN_MONITOR;
>>>  		port->sm_churn_partner_state = AD_CHURN_MONITOR;
>>> +
>>>  		port->sm_churn_actor_timer_counter =
>>>  			__ad_timer_to_ticks(AD_ACTOR_CHURN_TIMER, 0);
>>>  		port->sm_churn_partner_timer_counter =
>>> -			 __ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
>>> +			__ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
>>> +
>>
>> Please avoid white-space changes only, or if you are going to target
>> net-next, move them to a pre-req patch.
> 
> OK, what's pre-req patch?

I mean: a separate patch, earlier in the series, to keep cosmetic and
functional changes separated and more easily reviewable.
>>> +		if (actor_synced) {
>>> +			port->sm_vars &= ~AD_PORT_ACTOR_CHURN;
>>>  			port->sm_churn_actor_state = AD_NO_CHURN;
>>> -		} else {
>>> -			port->churn_actor_count++;
>>> -			port->sm_churn_actor_state = AD_CHURN;
>>> +			actor_churned = false;
>>>  		}
>>
>> I think this part is not described by the state diagram above?!?
> 
> This part is about path (3), port in monitor or churn, and actor is in sync.
> Then move to state no_churn.
> 
> Do you mean port->sm_vars &= ~AD_PORT_ACTOR_CHURN is not described?
> Hmm, maybe we don't need this after re-organise.

I mean the state change in the else part, I can't map them in the state
machine diagram.

>>>  	}
>>> +
>>> +	/* ---- 4. NO_CHURN + !sync -> MONITOR ---- */
>>> +	if (port->sm_churn_actor_state == AD_NO_CHURN && !actor_churned && !actor_synced) {
>>> +		port->sm_churn_actor_state = AD_CHURN_MONITOR;
>>> +		port->sm_churn_actor_timer_counter =
>>> +			__ad_timer_to_ticks(AD_ACTOR_CHURN_TIMER, 0);
>>
>> Should this clear sm_vars & AD_PORT_ACTOR_CHURN, too?
> 
> Yes, or we can just remove AD_PORT_ACTOR_CHURN as I said above.
Generally speaking less state sounds simpler and better.

/P