[PATCH net] selftests: bonding: feed link monitor to deflake prio_test

[PATCH net] selftests: bonding: feed link monitor to deflake prio_test

[Bjoern Doebel]

With the bond idle between test steps, the arp/ns link monitor can
declare a healthy slave down (~300ms without RX/TX). If this hits a
failover step, the bond picks a different active slave than expected,
and primary_reselect=failure/better never reverts it:

  TEST: prio (active-backup ns_ip6_target primary_reselect 2) [FAIL]
      Current active slave is eth0 but not eth2

Reproducible when running tests under load (2-4 / 100 runs).

Fix by pinging a non-existent neighbor for the duration of the test to
keep traffic coming. 100 runs under the same load with the patch
applied: 0 failures.

Fixes: 42a8d4aaea84 ("selftests: bonding: add bonding prio option test")
Signed-off-by: Bjoern Doebel <doebel@amazon.de>
---
 .../selftests/drivers/net/bonding/bond_options.sh   | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
index 187b478d0ddf2..f599a2a8ef156 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
@@ -74,6 +74,16 @@ prio_test()
 
 	# create bond
 	bond_reset "${param}"
+
+	# Keep the arp/ns link monitor fed for the duration of the test:
+	# ping a non-existent neighbor, which generates steady broadcast
+	# ARP requests sourced from the bond's own address. These egress on
+	# the active slave and are flooded onto the backup slaves' links,
+	# where they validate as RX activity. Without traffic, idle slaves
+	# are declared down by the monitor between test steps and the
+	# failover checks below race with the monitor.
+	ip netns exec ${s_ns} ping -i 0.1 -W 1 192.0.2.99 &> /dev/null &
+	local ping_pid=$!
 	# set active_slave to primary eth1 specifically
 	ip -n ${s_ns} link set bond0 type bond active_slave eth1
 
@@ -157,6 +167,9 @@ prio_test()
 		check_active_slave "eth0"
 		bond_check_connection "change slave prio"
 	fi
+
+	kill $ping_pid 2>/dev/null
+	wait $ping_pid 2>/dev/null
 }
 
 prio_miimon()

base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d
-- 
2.50.1

Re: [PATCH net] selftests: bonding: feed link monitor to deflake prio_test

[Hangbin Liu]

Hi Bjoern,
On Wed, Nov 04, 2026 at 02:20:18PM +0000, Bjoern Doebel wrote:
> With the bond idle between test steps, the arp/ns link monitor can
> declare a healthy slave down (~300ms without RX/TX). If this hits a
> failover step, the bond picks a different active slave than expected,
> and primary_reselect=failure/better never reverts it:
> 
>   TEST: prio (active-backup ns_ip6_target primary_reselect 2) [FAIL]
>       Current active slave is eth0 but not eth2


Thanks for the fix. I also saw this issue but haven't checked it yet.

> 
> Reproducible when running tests under load (2-4 / 100 runs).
> 
> Fix by pinging a non-existent neighbor for the duration of the test to
> keep traffic coming. 100 runs under the same load with the patch
> applied: 0 failures.
> 
> Fixes: 42a8d4aaea84 ("selftests: bonding: add bonding prio option test")
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> ---
>  .../selftests/drivers/net/bonding/bond_options.sh   | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
> index 187b478d0ddf2..f599a2a8ef156 100755
> --- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
> +++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
> @@ -74,6 +74,16 @@ prio_test()
>  
>  	# create bond
>  	bond_reset "${param}"
> +
> +	# Keep the arp/ns link monitor fed for the duration of the test:
> +	# ping a non-existent neighbor, which generates steady broadcast
> +	# ARP requests sourced from the bond's own address. These egress on
> +	# the active slave and are flooded onto the backup slaves' links,
> +	# where they validate as RX activity. Without traffic, idle slaves
> +	# are declared down by the monitor between test steps and the
> +	# failover checks below race with the monitor.
> +	ip netns exec ${s_ns} ping -i 0.1 -W 1 192.0.2.99 &> /dev/null &
> +	local ping_pid=$!
>  	# set active_slave to primary eth1 specifically
>  	ip -n ${s_ns} link set bond0 type bond active_slave eth1
>  
> @@ -157,6 +167,9 @@ prio_test()
>  		check_active_slave "eth0"
>  		bond_check_connection "change slave prio"
>  	fi
> +
> +	kill $ping_pid 2>/dev/null
> +	wait $ping_pid 2>/dev/null

If the kill fails for some reason, will we wait here forever and hang the
entire test?

Thanks
Hangbin

Re: [PATCH net] selftests: bonding: feed link monitor to deflake prio_test

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 3

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).

- [High] The commit message and the new comment in prio_test()
  (tools/testing/selftests/drivers/net/bonding/bond_options.sh:78-86)…
- [Low] prio_test() starts the background ping unconditionally.
- [Low] The new background `ip netns exec ${s_ns} ping -i 0.1 -W 1
  192.0.2.99 &` in prio_test() has no -c or -w, so it never exits on its…
- [Low] selftests: bonding: inaccurate Fixes tag for prio_test() link
  monitor race

--

selftests: bonding: feed link monitor to deflake prio_test

This patch starts a background ping to a non-existent neighbor
(192.0.2.99) in the server namespace for the duration of prio_test() in
bond_options.sh. The goal is to generate traffic so the arp/ns link
monitor stops declaring healthy slaves down between test steps, which
made failover checks fail intermittently under load.

> diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
> index 187b478d0ddf2..f599a2a8ef156 100755
> --- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
> +++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
> @@ -74,6 +74,16 @@ prio_test()
>  
>  	# create bond
>  	bond_reset "${param}"
> +
> +	# Keep the arp/ns link monitor fed for the duration of the test:
> +	# ping a non-existent neighbor, which generates steady broadcast
> +	# ARP requests sourced from the bond's own address. These egress on
> +	# the active slave and are flooded onto the backup slaves' links,
> +	# where they validate as RX activity. Without traffic, idle slaves
> +	# are declared down by the monitor between test steps and the
> +	# failover checks below race with the monitor.
> +	ip netns exec ${s_ns} ping -i 0.1 -W 1 192.0.2.99 &> /dev/null &
> +	local ping_pid=$!

[Severity: Low]
Is 42a8d4aaea84 the right commit for the Fixes tag? As far as I can
tell, that commit added option_prio.sh with arp_interval 1000, which
gives a monitor timeout of roughly 3.5 seconds. It had no
ns_ip6_target variant. It also ran ping -c5 before check_active_slave
on every failover step. The ~300ms idle window described in the
commit message doesn't seem to exist in that version of the test.

The failing configuration in the report is prio_ns() calling
prio_test() with arp_interval 100 and ns_ip6_target. That appears to
come from 481b56e0391e ("selftests: bonding: re-format bond option
tests"), which moved the prio tests into bond_options.sh.

Later, 45bf79bc56c4 ("selftests: bonding: reduce
garp_test/arp_validate test time") moved check_active_slave ahead of
bond_check_connection in the failover steps. That removed the traffic
that used to run just before each check.

Should the Fixes tag point at 481b56e0391e instead? Or at
45bf79bc56c4, if the reordering is what exposed the race? Stable
backports rely on this tag to pick target trees. Trees that have
42a8d4aaea84 but not 481b56e0391e have no bond_options.sh, so this
patch can't apply there anyway.

[Severity: High]
Does this ping change what the monitor sees? The comment and the commit
message describe a mechanism that doesn't match what the bonding and
neighbour code do.

First, is the bond really idle between steps? With arp_interval 100,
bond_activebackup_arp_mon() calls bond_ab_arp_probe() every 100ms. That
function already sends broadcast ARP/NS on the active slave:

drivers/net/bonding/bond_main.c:bond_ab_arp_probe() {
    ...
	if (curr_active_slave) {
		bond_send_validate(bond, curr_active_slave);
		return should_notify_rtnl;
	}
    ...
}

br0 floods these probes to the backup ports. With the default
arp_validate=none, bond_rcv_validate() refreshes last_rx for any frame
it receives:

			WRITE_ONCE(slave->last_rx, jiffies);

Since no validation is configured, "validate as RX activity" doesn't
apply here.

Second, does ping -i 0.1 produce steady ARP? For an unresolved
neighbour, __neigh_event_send() sends a single probe and arms the timer
at RETRANS_TIME. Echoes sent while the entry is INCOMPLETE are only
queued on arp_queue. After that, neigh_timer_handler() re-probes at:

		/* NUD_PROBE|NUD_INCOMPLETE */
		next = now + max(NEIGH_VAR(neigh->parms, RETRANS_TIME), HZ/100);

and arp_tbl defaults to:

	[NEIGH_VAR_RETRANS_TIME] = 1 * HZ,

That works out to about 1 ARP per second. The monitor itself sends 10
per second, and the detection windows are about 250-350ms.

Third, the ping does nothing for the active slave. slave->last_tx is
only updated by the bond's own probes in bond_arp_send() and
bond_ns_send(), not by ARP generated by the IP stack:

		slave_update_last_tx(slave);
		arp_xmit(skb);

Nothing answers these ARPs, so the active slave's last_rx doesn't get
refreshed either. bond_ab_arp_inspect() checks both values:

		last_tx = slave_last_tx(slave);
		if (bond_is_active_slave(slave) &&
		    (!bond_time_in_interval(bond, last_tx, bond->params.missed_max) ||
		     !bond_time_in_interval(bond, last_rx, bond->params.missed_max))) {

Finally, in the fail over step, "ip link set eth1 down" leaves eth1 as
curr_active_slave until the monitor declares it down. Until then, all
bond0 TX goes to eth1 and is dropped, including the ping's ARPs.

That means the backups get no RX in exactly the window where two checks
race: the active slave goes down after missed_max * delta, and the
backups after (missed_max + 1) * delta. Under load, monitor latency or
rtnl_trylock retries can still merge or reorder those passes, the same
as before the patch.

Is 0/100 against 2-4/100 enough to show the fix works? 0.98^100 is
about 13% and 0.97^100 is about 5%. So 0 failures in 100 runs is quite
plausible without the patch.

Could the root cause be looked at again, and the comment corrected so
the tree doesn't keep this explanation?

[Severity: Low]
Should the ping be started only for the arp_ip_target and ns_ip6_target
runs? prio_test() is also called from prio_miimon() for active-backup,
balance-tlb and balance-alb. miimon is carrier-based, so the monitor
starvation rationale doesn't apply to those runs.

In balance-alb, every ARP request the ping triggers goes through
bond_alb_xmit()->rlb_arp_xmit(). The ARPOP_REQUEST branch creates an
rx_hashtbl entry for 192.0.2.99 and resets the RLB update delay:

		tx_slave = rlb_choose_channel(skb, bond, arp);

		/* The ARP reply packets must be delayed so that
		 * they can cancel out the influence of the ARP request.
		 */
		bond->alb_info.rlb_update_delay_counter = RLB_UPDATE_DELAY;

bond_alb_monitor() only calls rlb_update_rx_clients() after that counter
has counted down to zero:

			if (bond_info->rlb_update_delay_counter) {
				--bond_info->rlb_update_delay_counter;
			} else {
				rlb_update_rx_clients(bond);

RLB_UPDATE_DELAY is 2 seconds, and the ping sends a new ARP request
about once a second, so the counter never gets to zero. The RLB client
updates that follow a slave failure are then held off for the whole
balance-alb prio run.

rlb_choose_channel() returns __rlb_next_rx_slave(), so the request can
also go out on a non-active slave carrying the bond MAC. That
contradicts "These egress on the active slave" in the new comment.

This changes what the balance-alb prio tests exercise, and the commit
message doesn't mention it.

>  	# set active_slave to primary eth1 specifically
>  	ip -n ${s_ns} link set bond0 type bond active_slave eth1
>  

[ ... ]

> @@ -157,6 +167,9 @@ prio_test()
>  		check_active_slave "eth0"
>  		bond_check_connection "change slave prio"
>  	fi
> +
> +	kill $ping_pid 2>/dev/null
> +	wait $ping_pid 2>/dev/null
>  }

[Severity: Low]
Can the background ping outlive the script? It has no -c or -w, so it
never exits by itself. ping_pid is a local variable in the function,
and this kill only runs when prio_test() returns normally.

The EXIT trap handler, cleanup() in bond_topo_2d1c.sh, doesn't kill
background jobs:

cleanup()
{
	pre_cleanup

	client_destroy
	server_destroy
	gateway_destroy
}

The ping gets orphaned if a signal reaches only the script's PID while
prio_test() is running, for example a manual kill or a harness that
signals only the main PID. It then runs forever and keeps s_ns alive
after ip netns del.

The kselftest runner's process-group timeout and Ctrl-C both reach the
ping, so the common paths are fine.

Would adding -w, or registering kill_process() from net/lib.sh through
defer or the trap, avoid this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104142018.552135-1-doebel%40amazon.de