Re: [PATCH net v2] bonding: 3ad: select a port when the TX array is empty

[luyun <luyun_611@163.com>]

Thanks for the review. The v3 has been posted:
https://lore.kernel.org/all/20261008024031.16224-1-luyun_611@163.com/

Main changes in v3:

- Drop the pending latch and all mode_lock usage in the transmit path.
   The fallback is now lockless, so both the contention drops and the
   recursive-TX concern are gone, and the latch handoff race no longer
   exists.
- Eligibility uses the same criteria as bond_update_slave_arr() (port's
   aggregator active + bond_slave_can_tx), with one aggregator object
   pinned per transmit, so selection never spans two objects.
- broadcast_neighbor packets are replicated to all eligible members of
   the pinned object instead of a single copy.
- The commit message now states the remaining scope: XDP transmit and
   ndo_sk_get_lower_dev are left for follow-up changes.

---
Thanks,
Yun Lu

在 2026/9/22 17:17, netdev-bot+sashiko@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] Incomplete fix: the new empty-array fallback is wired only
>    into bond_3ad_xor_xmit(), bond_xmit_broadcast() (broadcast-neighbor…
> - [Medium] bond_3ad_xmit_fallback() abandons selection (returns NULL ->
>    bond_tx_drop() in bond_3ad_xor_xmit(), or a NULL ndo_get_xmit_slave…
> - [Low] Documented behaviour vs implementation mismatch for
>    broadcast_neighbor.
> - [Low] Residual drop window from read/write order asymmetry between the
>    array publication and the latch clear.
>
> --
>
> bonding: 3ad: select a port when the TX array is empty
>
> This adds a boolean latch (bond->slave_arr_update_pending) set by the 802.3ad
> state machine and cleared by the slave array worker, and uses it to let the
> transmit paths pick a port straight from the active aggregator while the
> usable_slaves array update is still unpublished.  The fallback is wired into
> bond_3ad_xor_xmit(), the broadcast-neighbor case of bond_xmit_broadcast() and
> bond_xmit_get_slave().
>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index b8e4b4d68dd6..1fe499ade439 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2614,6 +2614,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>>   			break;
>>   		}
>>   	}
>> +	if (update_slave_arr)
>> +		WRITE_ONCE(bond->slave_arr_update_pending, true);
>>   	rcu_read_unlock();
>>   	spin_unlock_bh(&bond->mode_lock);
>>   
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index a9bff7663eec..1144281912d3 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
> [ ... ]
>
>> @@ -5093,6 +5094,8 @@ static void bond_slave_arr_handler(struct work_struct *work)
>>   		pr_warn_ratelimited("Failed to update slave array from WT\n");
>>   		goto err;
>>   	}
>> +	/* bond->wq is ordered, so no newer request can race this clear. */
>> +	WRITE_ONCE(bond->slave_arr_update_pending, false);
>>   	return;
>>   
>> err:
> [Severity: Low]
> Can a packet still be dropped in the window this patch closes?  The worker
> publishes the new array first and clears the latch second, and the transmit
> side reads those two pieces of state in the same order:
>
> CPU0 bond_slave_arr_handler()
>      bond_update_slave_arr(bond, NULL);   /* publishes non-empty array */
>      WRITE_ONCE(bond->slave_arr_update_pending, false);
>
> CPU1 bond_3ad_xor_xmit()
>      slaves = rcu_dereference(bond->usable_slaves);   /* old, count == 0 */
>      bond_xmit_hash(bond, skb);                       /* full dissection */
>      bond_3ad_xmit_fallback()
>          if (!READ_ONCE(bond->slave_arr_update_pending))
>              return NULL;                             /* latch now clear */
>      return bond_tx_drop(dev, skb);
>
> The gap between the two reads on CPU1 spans a full bond_xmit_hash() call, so
> CPU1 can observe the stale empty array and then the already cleared latch.
>
> The re-check of the latch under mode_lock inside bond_3ad_xmit_fallback()
> does not cover this, since the clearing side (bond_slave_arr_handler()) never
> takes mode_lock; only the setting side in bond_3ad_state_machine_handler()
> does.
>
> Would re-reading bond->usable_slaves after observing a cleared latch, or
> reading the latch before sampling the array and re-validating, close this
> remainder?
>
>> @@ -5289,9 +5292,67 @@ static bool bond_should_broadcast_neighbor(struct sk_buff *skb,
>>   	return false;
>>   }
>>   
>> -/* Use this Xmit function for 3AD as well as XOR modes. The current
>> - * usable slave array is formed in the control path. The xmit function
>> - * just calculates hash and sends the packet out.
>> +/* Called with RCU and bond->mode_lock held. */
>> +static bool bond_3ad_slave_is_eligible(struct slave *slave)
>> +{
>> +	const struct aggregator *agg;
>> +
>> +	agg = rcu_dereference(SLAVE_AD_INFO(slave)->port.aggregator);
>> +	return agg && agg->is_active && bond_slave_can_tx(slave);
>> +}
>> +
>> +/* Called with RCU held. */
>> +static struct slave *bond_3ad_xmit_fallback(struct bonding *bond, u32 hash)
>> +{
>> +	struct slave *selected = NULL;
>> +	unsigned int eligible = 0;
>> +	unsigned int target;
>> +	struct list_head *iter;
>> +	struct slave *slave;
>> +
>> +	/* Limit the list walk to the array-update window. */
>> +	if (!netif_carrier_ok(bond->dev) ||
>> +	    !READ_ONCE(bond->slave_arr_update_pending))
>> +		return NULL;
>> +
>> +	/* TX may recurse while mode_lock is already held. */
>> +	if (unlikely(netpoll_tx_running(bond->dev)) ||
>> +	    !spin_trylock_bh(&bond->mode_lock))
>> +		return NULL;
> [Severity: Medium]
> The trylock is needed for the recursive TX case, but does it also turn plain
> remote-CPU contention into a packet drop during exactly the convergence
> period the patch targets?
>
> mode_lock is held for the whole body of bond_3ad_state_machine_handler(),
> walking every slave and running ad_rx_machine(), ad_periodic_machine(),
> ad_port_selection_logic(), ad_mux_machine(), ad_tx_machine() and
> ad_churn_machine(), with the latch published only at the very end:
>
>      spin_lock_bh(&bond->mode_lock);
>      ...
>      if (update_slave_arr)
>          WRITE_ONCE(bond->slave_arr_update_pending, true);
>      rcu_read_unlock();
>      spin_unlock_bh(&bond->mode_lock);
>
> It is also taken by the LACPDU receive path in bond_3ad_rx_indication():
>
>      /* Protect against concurrent state machines */
>      spin_lock(&slave->bond->mode_lock);
>      ad_rx_machine(lacpdu, port);
>      spin_unlock(&slave->bond->mode_lock);
>
> and by bond_3ad_handle_link_change().  Since bond_setup() sets
> bond_dev->lltx = true, transmitters are not serialized by the device TX lock,
> so a transmit on another CPU landing in any of those sections (or racing
> another CPU already inside the fallback's own two list walks) fails the
> trylock and bond_3ad_xor_xmit() drops the skb even though the latch is set
> and eligible ports exist.
>
> Would having the state machine publish a fallback slave via RCU, so the TX
> fast path never needs the LACP state-machine spinlock, avoid both the
> recursion problem and the contention drops?  As written, the new code also
> puts a mode_lock acquisition plus a full lower-device list walk on the
> per-packet TX path for the duration of the window.
>
>> +
>> +	if (!READ_ONCE(bond->slave_arr_update_pending))
>> +		goto out;
>> +
>> +	bond_for_each_slave_rcu(bond, slave, iter)
>> +		if (bond_3ad_slave_is_eligible(slave))
>> +			eligible++;
>> +
>> +	if (!eligible)
>> +		goto out;
>> +
>> +	target = hash % eligible;
>> +	bond_for_each_slave_rcu(bond, slave, iter) {
>> +		if (!bond_3ad_slave_is_eligible(slave))
>> +			continue;
>> +
>> +		if (!selected)
>> +			selected = slave;
>> +
>> +		if (!target) {
>> +			selected = slave;
>> +			break;
>> +		}
>> +		target--;
>> +	}
>> +
>> +out:
>> +	spin_unlock_bh(&bond->mode_lock);
>> +	return selected;
>> +}
>> +
>> +/* Use this Xmit function for 3AD as well as XOR modes. The usable slave
>> + * array is formed in the control path. In 3AD mode, fall back to the current
>> + * port state while an empty array update is pending.
>>    */
>>   static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
>>   				     struct net_device *dev)
>> @@ -5302,6 +5363,8 @@ static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
>>   
>>   	slaves = rcu_dereference(bond->usable_slaves);
>>   	slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
>> +	if (unlikely(!slave) && BOND_MODE(bond) == BOND_MODE_8023AD)
>> +		slave = bond_3ad_xmit_fallback(bond, bond_xmit_hash(bond, skb));
>>   	if (likely(slave))
>>   		return bond_dev_queue_xmit(bond, skb, slave->dev);
>>   
>> @@ -5327,6 +5390,16 @@ static netdev_tx_t bond_xmit_broadcast(struct sk_buff *skb,
>>   		slaves = rcu_dereference(bond->usable_slaves);
>>   
>>   	slaves_count = slaves ? READ_ONCE(slaves->count) : 0;
>> +	if (!slaves_count && !all_slaves &&
>> +	    BOND_MODE(bond) == BOND_MODE_8023AD) {
>> +		struct slave *slave;
>> +
>> +		slave = bond_3ad_xmit_fallback(bond,
>> +					       bond_xmit_hash(bond, skb));
>> +		if (slave)
>> +			return bond_dev_queue_xmit(bond, skb, slave->dev);
>> +	}
>> +
> [Severity: Low]
> Does this branch match the documented meaning of broadcast_neighbor?
>
> Documentation/networking/bonding.rst describes the option as:
>
> 	Option specifying whether to broadcast ARP/ND packets to all
> 	active slaves.  This option has no effect in modes other than
> 	802.3ad mode.  The default is off (0).
>
> and the descriptor for BOND_OPT_BROADCAST_NEIGH in bond_options.c says
> "Broadcast neighbor packets to all active slaves".
>
> bond_3ad_xmit_fallback() is a hash-selecting single-port helper
> (target = hash % eligible), so this sends one copy on one port and returns
> before the replication loop.  Since the port choice is
> bond_xmit_hash(bond, skb) % eligible, the same neighbour deterministically
> leaves through the same port and the other eligible members of the active
> aggregator see no ARP/ND for the whole update window.
>
> bond_3ad_xmit_fallback() already enumerates the full eligible set under
> mode_lock.  Could the broadcast caller replicate to every eligible port
> instead, to match the array path it mirrors?  Otherwise, should the
> documentation describe the degraded behaviour during the window?
>
>>   	for (i = 0; i < slaves_count; i++) {
>>   		struct slave *slave = slaves->arr[i];
>>   		struct sk_buff *skb2;
>> @@ -5432,6 +5505,10 @@ static struct net_device *bond_xmit_get_slave(struct net_device *master_dev,
>>   		else
>>   			slaves = rcu_dereference(bond->usable_slaves);
>>   		slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
>> +		if (!slave && !all_slaves &&
>> +		    BOND_MODE(bond) == BOND_MODE_8023AD)
>> +			slave = bond_3ad_xmit_fallback(bond,
>> +						       bond_xmit_hash(bond, skb));
>>   		break;
> [Severity: Medium]
> Should the XDP transmit selector get the same treatment?
> bond_xdp_xmit_3ad_xor_slave_get() reads the same bond->usable_slaves array
> and still has the unconditional empty-array return:
>
> 	count = slaves ? READ_ONCE(slaves->count) : 0;
> 	if (unlikely(!count))
> 		return NULL;
>
> so during the same window bond_xdp_get_xmit_slave() returns NULL for
> BOND_MODE_8023AD and bond_xdp_xmit() aborts the batch:
>
> 		slave_dev = bond_xdp_get_xmit_slave(bond_dev, &xdp);
> 		if (!slave_dev) {
> 			err = -ENXIO;
> 			break;
> 		}
>
> __bond_sk_get_lower_dev() has the same count == 0 NULL return for the
> TLS/socket lower-device lookup, so two consumers of the same emptiness
> condition now answer differently.
>
> The commit message says:
>
>      If an skb transmit or ndo_get_xmit_slave query finds an empty usable array
>      while the latch is set, select a port directly from the active aggregator.
>
> but it does not state that the XDP transmit path keeps dropping frames in
> that window.  The only mention, "leave XDP for a separate follow-up patch",
> is in the v2 changelog below the --- separator, which git does not record.
> Could that limitation be stated in the commit message itself, given the
> Fixes: tag makes this a backport candidate?
>
> [ ... ]
>