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