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

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

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

This issue was observed on a machine in real use. During 802.3ad
bring-up in production, packets were dropped even though the bond's
carrier was up and LACP had enabled ports for distribution. The
transmit path only consults usable_slaves and returns no slave when
the array is NULL or its count is zero.

LACP port eligibility and the TX array are updated separately. The
state machine enables ports and updates carrier before scheduling
slave_arr_work to rebuild and publish the array. The worker requires
RTNL and retries if rtnl_trylock() fails, so RTNL contention can
prolong the interval in which eligible ports exist but the cached
array is empty.

Add a fallback for 802.3ad when the usable array is empty and carrier
is up. Walk the RCU-protected slave list and use ports whose
aggregator is active and which pass bond_slave_can_tx(), without
taking mode_lock or changing LACP state. In 802.3ad mode the
per-slave active flag is set only by the mux machine for ports of the
active aggregator, so these lockless reads identify distributable
ports with the same criteria bond_update_slave_arr() applies when
building the array.

Fix the aggregator object at the first eligible port so counting and
selection use members of the same object. The object pointer is used
only for comparison within the caller's RCU section. Count eligible
members, then select with the existing hash modulo that count in
slave-list order. This preserves the array path's mapping when the
eligible set is stable. Recheck eligibility in the second pass and
use the first member of the pinned object if the selected index is no
longer reachable. Ports of any other aggregator object are never
used; the fallback drops the packet instead.

Use the fallback for skb transmission and ndo_get_xmit_slave queries
with all_slaves=false. For broadcast_neighbor ARP/ND packets, replicate
to all eligible members of the selected object. Keep the nonempty array
selection unchanged and defer hashing until an eligible set is found.

Persistent empty states with carrier up still incur one lockless list
walk per dropped packet. XDP transmit and ndo_sk_get_lower_dev use
separate frame and socket hashing interfaces. Their fallback handling
needs separate implementation and validation, so this fix leaves their
array-based selection unchanged.

Tested on x86-64 KVM against a two-port LACP peer. With array
publication deferred by a test-only gate, UDP, ARP and ND all left
through ports selected by the fallback while both arrays were NULL,
and the same UDP flows kept their port mapping after the array was
published. All 16 bonding selftests passed without the test-only
modifications.

Fixes: ee6377147409 ("bonding: Simplify the xmit function for modes that use xmit_hash")
Signed-off-by: Yun Lu <luyun@kylinos.cn>
---
Changes in v3:
- Drop the pending latch and all TX-path locking (the mode_lock trylock
  and the netpoll guard); the fallback is now lockless.
- Pin one aggregator object per transmit, by pointer comparison within
  the RCU section, for both hash selection and broadcast replication.
  Never select across aggregator objects.
- Replicate broadcast_neighbor packets to all eligible members of the
  pinned object instead of sending a single copy.
- Defer hashing until the fallback finds a non-empty eligible set.
- Read aggregator is_active with READ_ONCE.

v2: https://lore.kernel.org/all/20260918090137.66515-1-luyun_611@163.com/

Changes in v2:
- Track unpublished array updates with a boolean latch, limiting fallback
  locking and list walks to the asynchronous update window.
- Use spin_trylock_bh() so recursive TX keeps the existing drop or NULL
  behavior instead of deadlocking on mode_lock.
- Use the first eligible port if eligibility changes between the two list
  walks.
- Apply the fallback to ndo_get_xmit_slave and broadcast-neighbor skb
  transmission. Keep control-path readers on the published array and leave
  XDP for a separate follow-up patch.
- Document why hash calculation is deferred until the array count is known.

v1: https://lore.kernel.org/netdev/20260915090207.110987-1-luyun_611@163.com/

Testing: two-port LACP bring-up on x86-64 KVM, injected state changes,
and an unsynchronized peer with no eligible port. A test-only kernel
modification deferred usable_slaves and all_slaves publication from bond
creation; with both arrays NULL after convergence, UDP used one eligible
port and ARP/ND reached both. Releasing the gate and repeating the same
UDP flows preserved port mapping. Test-only hooks injected eligibility
shrink and active aggregator changes between selection passes and during
broadcast; these synthetic cases passed.

All 16 bonding selftests passed without the test-only modifications.

At 32 ports, paired KVM measurements showed about 0.4 us/attempt
additional thread CPU time in the persistent-empty-array case. Timings
include userspace overhead.

 drivers/net/bonding/bond_main.c | 93 +++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index de2489c3d9bf..2fed71d7586b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5233,11 +5233,11 @@ static struct slave *bond_xmit_3ad_xor_slave_get(struct bonding *bond,
 	unsigned int count;
 	u32 hash;
 
-	hash = bond_xmit_hash(bond, skb);
 	count = slaves ? READ_ONCE(slaves->count) : 0;
 	if (unlikely(!count))
 		return NULL;
 
+	hash = bond_xmit_hash(bond, skb);
 	slave = slaves->arr[hash % count];
 	return slave;
 }
@@ -5289,9 +5289,60 @@ 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 held and only in 802.3ad mode. */
+static const struct aggregator *bond_3ad_tx_eligible_agg(struct slave *slave)
+{
+	const struct aggregator *agg;
+
+	agg = rcu_dereference(SLAVE_AD_INFO(slave)->port.aggregator);
+	if (!agg || !READ_ONCE(agg->is_active) || !bond_slave_can_tx(slave))
+		return NULL;
+
+	return agg;
+}
+
+/* Called with RCU held when the usable slave array is empty. */
+static struct slave *bond_3ad_xmit_fallback(struct bonding *bond,
+					    struct sk_buff *skb)
+{
+	const struct aggregator *agg = NULL, *slave_agg;
+	struct slave *slave, *first = NULL;
+	struct list_head *iter;
+	unsigned int count = 0, target;
+
+	if (!netif_carrier_ok(bond->dev))
+		return NULL;
+
+	bond_for_each_slave_rcu(bond, slave, iter) {
+		slave_agg = bond_3ad_tx_eligible_agg(slave);
+		if (!slave_agg)
+			continue;
+		if (!agg)
+			agg = slave_agg;
+		if (slave_agg == agg)
+			count++;
+	}
+
+	if (!count)
+		return NULL;
+
+	target = bond_xmit_hash(bond, skb) % count;
+	bond_for_each_slave_rcu(bond, slave, iter) {
+		if (bond_3ad_tx_eligible_agg(slave) != agg)
+			continue;
+		if (!first)
+			first = slave;
+		if (!target)
+			return slave;
+		target--;
+	}
+
+	return first;
+}
+
+/* Use this Xmit function for 3AD as well as XOR modes. The usable
+ * slave array is formed in the control path; 3AD can use current state
+ * while the array is empty.
  */
 static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
 				     struct net_device *dev)
@@ -5302,6 +5353,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, skb);
 	if (likely(slave))
 		return bond_dev_queue_xmit(bond, skb, slave->dev);
 
@@ -5327,6 +5380,35 @@ 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 &&
+	    netif_carrier_ok(bond_dev)) {
+		const struct aggregator *agg = NULL, *slave_agg;
+		struct list_head *iter;
+		struct slave *slave;
+
+		bond_for_each_slave_rcu(bond, slave, iter) {
+			struct sk_buff *skb2;
+
+			slave_agg = bond_3ad_tx_eligible_agg(slave);
+			if (!slave_agg)
+				continue;
+			if (!agg)
+				agg = slave_agg;
+			if (slave_agg != agg)
+				continue;
+
+			skb2 = skb_clone(skb, GFP_ATOMIC);
+			if (!skb2) {
+				net_err_ratelimited("%s: Error: %s: skb_clone() failed\n",
+						    bond_dev->name, __func__);
+				continue;
+			}
+
+			if (bond_dev_queue_xmit(bond, skb2, slave->dev) == NETDEV_TX_OK)
+				xmit_suc = true;
+		}
+	}
 	for (i = 0; i < slaves_count; i++) {
 		struct slave *slave = slaves->arr[i];
 		struct sk_buff *skb2;
@@ -5432,6 +5514,9 @@ 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 (unlikely(!slave) && !all_slaves &&
+		    BOND_MODE(bond) == BOND_MODE_8023AD)
+			slave = bond_3ad_xmit_fallback(bond, skb);
 		break;
 	case BOND_MODE_BROADCAST:
 		break;

base-commit: 6e0022b5ae3dc5b833af0fcc1578dc20a1d6bc71
-- 
2.43.0

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

[Hangbin Liu]

Hi Lu Yun,
On Thu, Oct 08, 2026 at 10:40:31AM +0800, Yun Lu wrote:
> From: Yun Lu <luyun@kylinos.cn>
> 
> This issue was observed on a machine in real use. During 802.3ad
> bring-up in production, packets were dropped even though the bond's
> carrier was up and LACP had enabled ports for distribution. The
> transmit path only consults usable_slaves and returns no slave when
> the array is NULL or its count is zero.
> 
> LACP port eligibility and the TX array are updated separately. The
> state machine enables ports and updates carrier before scheduling
> slave_arr_work to rebuild and publish the array. The worker requires
> RTNL and retries if rtnl_trylock() fails, so RTNL contention can
> prolong the interval in which eligible ports exist but the cached
> array is empty.
> 
> Add a fallback for 802.3ad when the usable array is empty and carrier
> is up. Walk the RCU-protected slave list and use ports whose
> aggregator is active and which pass bond_slave_can_tx(), without
> taking mode_lock or changing LACP state. In 802.3ad mode the
> per-slave active flag is set only by the mux machine for ports of the
> active aggregator, so these lockless reads identify distributable
> ports with the same criteria bond_update_slave_arr() applies when
> building the array.

`bond_update_slave_arr()` is called from `bond_enslave()`, and it should be
very fast. Could we add a check in `bond_3ad_state_machine_handler()` or
another suitable place to ensure usable slaves exist before entering the
distribution state?

The fallback path seems somewhat complex and costly. Or you can wait for
Jay's feedback.

Thanks
Hangbin

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

[luyun]

在 2026/10/10 17:43, Hangbin Liu 写道:
> Hi Lu Yun,
> On Thu, Oct 08, 2026 at 10:40:31AM +0800, Yun Lu wrote:
>> From: Yun Lu <luyun@kylinos.cn>
>>
>> This issue was observed on a machine in real use. During 802.3ad
>> bring-up in production, packets were dropped even though the bond's
>> carrier was up and LACP had enabled ports for distribution. The
>> transmit path only consults usable_slaves and returns no slave when
>> the array is NULL or its count is zero.
>>
>> LACP port eligibility and the TX array are updated separately. The
>> state machine enables ports and updates carrier before scheduling
>> slave_arr_work to rebuild and publish the array. The worker requires
>> RTNL and retries if rtnl_trylock() fails, so RTNL contention can
>> prolong the interval in which eligible ports exist but the cached
>> array is empty.
>>
>> Add a fallback for 802.3ad when the usable array is empty and carrier
>> is up. Walk the RCU-protected slave list and use ports whose
>> aggregator is active and which pass bond_slave_can_tx(), without
>> taking mode_lock or changing LACP state. In 802.3ad mode the
>> per-slave active flag is set only by the mux machine for ports of the
>> active aggregator, so these lockless reads identify distributable
>> ports with the same criteria bond_update_slave_arr() applies when
>> building the array.
> `bond_update_slave_arr()` is called from `bond_enslave()`, and it should be
> very fast. Could we add a check in `bond_3ad_state_machine_handler()` or
> another suitable place to ensure usable slaves exist before entering the
> distribution state?
>
> The fallback path seems somewhat complex and costly. Or you can wait for
> Jay's feedback.

Hi Hangbin,

Thanks for the review and the suggestion.

bond_enslave() does call bond_update_slave_arr() synchronously, but at
that point the newly enslaved 802.3ad port is still inactive and there
is usually no active aggregator yet, so that call typically resets both
arrays to NULL. The problematic window opens later, when LACP
convergence completes, and it comes from the delay stages between port
enablement and array publication:

1) The state machine enables the port and brings carrier up inside the
    sweep (__enable_port() -> bond_3ad_set_carrier()), but slave_arr_work
    is only queued after mode_lock is released.

2) bond->wq is an ordered workqueue shared with ad_work, mcast_work and
    peer_notify_work, so slave_arr_work runs only after the work items
    queued ahead of it finish, on top of normal scheduler latency under
    boot load.

3) The worker needs RTNL. rtnl_lock is global: any concurrent netdev
    operation (e.g. udev / network manager churn during boot) makes
    rtnl_trylock() fail, and the worker requeues itself with a 1 jiffy
    delay, retrying until RTNL becomes available. This was exactly the
    situation on the production machine where we hit the problem.

During this window carrier is up and the ports are already
collecting/distributing, but usable_slaves is still NULL or empty, so
every packet goes through bond_tx_drop() and the sender sees no error.
In production ARP simply never went out.

A control-path check before entering the distribution state would be
circular with the current builder: bond_update_slave_arr() only picks
ports that already pass bond_slave_can_tx(), and that flag is set by
the mux machine itself on entering collecting/distributing. Waiting
for a non-empty array before enabling the first port never converges.

Deferring the mux transition instead would delay carrier-on, be
visible to the LACP partner, and under RTNL contention extend bring-up
by the same delay the fallback covers, so traffic would not flow any
sooner. The builder also needs RTNL, may sleep and takes mode_lock
itself, so it cannot be called from the mux critical section; a
control-path fix means splitting array preparation from port
enablement, with recovery when RTNL or allocation fails.

On cost: the fallback only runs when the array is empty, so the normal
transmit path is untouched. Inside the window it is a lockless RCU list
walk with no new state; the second pass and the broadcast branch are
there to keep the array path's hash mapping and broadcast_neighbor
semantics.

I would also like Jay's feedback on the preferred direction. If a
control-path solution is preferred, I am open to working on separating
array preparation from port enablement, as long as it does not change
LACP convergence behavior.

---

Thanks,
Yun Lu

>
> Thanks
> Hangbin

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

[Hangbin Liu]

Hi Lu Yun,

On Mon, Oct 12, 2026 at 03:20:09PM +0800, luyun wrote:
> 
> 在 2026/10/10 17:43, Hangbin Liu 写道:
> > Hi Lu Yun,
> > On Thu, Oct 08, 2026 at 10:40:31AM +0800, Yun Lu wrote:
> > > From: Yun Lu <luyun@kylinos.cn>
> > > 
> > > This issue was observed on a machine in real use. During 802.3ad
> > > bring-up in production, packets were dropped even though the bond's
> > > carrier was up and LACP had enabled ports for distribution. The
> > > transmit path only consults usable_slaves and returns no slave when
> > > the array is NULL or its count is zero.
> > > 
> > > LACP port eligibility and the TX array are updated separately. The
> > > state machine enables ports and updates carrier before scheduling
> > > slave_arr_work to rebuild and publish the array. The worker requires
> > > RTNL and retries if rtnl_trylock() fails, so RTNL contention can
> > > prolong the interval in which eligible ports exist but the cached
> > > array is empty.
> > > 
> > > Add a fallback for 802.3ad when the usable array is empty and carrier
> > > is up. Walk the RCU-protected slave list and use ports whose
> > > aggregator is active and which pass bond_slave_can_tx(), without
> > > taking mode_lock or changing LACP state. In 802.3ad mode the
> > > per-slave active flag is set only by the mux machine for ports of the
> > > active aggregator, so these lockless reads identify distributable
> > > ports with the same criteria bond_update_slave_arr() applies when
> > > building the array.
> > `bond_update_slave_arr()` is called from `bond_enslave()`, and it should be
> > very fast. Could we add a check in `bond_3ad_state_machine_handler()` or
> > another suitable place to ensure usable slaves exist before entering the
> > distribution state?
> > 
> > The fallback path seems somewhat complex and costly. Or you can wait for
> > Jay's feedback.
> 
> Hi Hangbin,
> 
> Thanks for the review and the suggestion.
> 
> bond_enslave() does call bond_update_slave_arr() synchronously, but at
> that point the newly enslaved 802.3ad port is still inactive and there
> is usually no active aggregator yet, so that call typically resets both
> arrays to NULL. The problematic window opens later, when LACP

Ah, yes.

> convergence completes, and it comes from the delay stages between port
> enablement and array publication:
> 
> 1) The state machine enables the port and brings carrier up inside the
>    sweep (__enable_port() -> bond_3ad_set_carrier()), but slave_arr_work
>    is only queued after mode_lock is released.
> 
> 2) bond->wq is an ordered workqueue shared with ad_work, mcast_work and
>    peer_notify_work, so slave_arr_work runs only after the work items
>    queued ahead of it finish, on top of normal scheduler latency under
>    boot load.
> 
> 3) The worker needs RTNL. rtnl_lock is global: any concurrent netdev
>    operation (e.g. udev / network manager churn during boot) makes
>    rtnl_trylock() fail, and the worker requeues itself with a 1 jiffy
>    delay, retrying until RTNL becomes available. This was exactly the
>    situation on the production machine where we hit the problem.
> 
> During this window carrier is up and the ports are already
> collecting/distributing, but usable_slaves is still NULL or empty, so
> every packet goes through bond_tx_drop() and the sender sees no error.
> In production ARP simply never went out.
> 
> A control-path check before entering the distribution state would be
> circular with the current builder: bond_update_slave_arr() only picks
> ports that already pass bond_slave_can_tx(), and that flag is set by
> the mux machine itself on entering collecting/distributing. Waiting
> for a non-empty array before enabling the first port never converges.

I feel calling bond_update_slave_arr after collecting/distributing is a bit
late. Maybe we should start updating slave array after entering
LACP_STATE_SYNCHRONIZATION, cause after that we start sending
LACP sync messages, which means the port is already up and usable.

> 
> Deferring the mux transition instead would delay carrier-on, be
> visible to the LACP partner, and under RTNL contention extend bring-up
> by the same delay the fallback covers, so traffic would not flow any
> sooner. The builder also needs RTNL, may sleep and takes mode_lock
> itself, so it cannot be called from the mux critical section; a
> control-path fix means splitting array preparation from port
> enablement, with recovery when RTNL or allocation fails.
> 
> On cost: the fallback only runs when the array is empty, so the normal
> transmit path is untouched. Inside the window it is a lockless RCU list
> walk with no new state; the second pass and the broadcast branch are
> there to keep the array path's hash mapping and broadcast_neighbor
> semantics.
> 
> I would also like Jay's feedback on the preferred direction. If a
> control-path solution is preferred, I am open to working on separating
> array preparation from port enablement, as long as it does not change
> LACP convergence behavior.

+1

Thanks
Hangbin