Netdev List
 help / color / mirror / Atom feed
* [PATCH net v4 0/2] bonding: fix the initial last_rx seed for the ARP monitor
@ 2026-10-28 11:48 Ramses de Norre via B4 Relay
  2026-10-28 11:48 ` [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window Ramses de Norre via B4 Relay
  2026-10-28 11:48 ` [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window Ramses de Norre via B4 Relay
  0 siblings, 2 replies; 8+ messages in thread
From: Ramses de Norre via B4 Relay @ 2026-10-28 11:48 UTC (permalink / raw)
  To: Jay Vosburgh, Andrew Lunn, David S. Miller, Jakub Kicinski,
	Paolo Abeni, Jiri Bohac, Eric Dumazet
  Cc: netdev, linux-kernel, Nikolay Aleksandrov, Hangbin Liu,
	Ramses de Norre

bond_enslave() seeds a new slave's last_rx before it has received
anything. The ARP monitor checks that seed with an up test for a slave
that starts DOWN, and with a down test for a slave that starts UP. The
seed is wrong for both.

Patch 1 fixes a slave that starts DOWN. Since the up test gained
arp_interval/2 of slack, the seed falls inside the up window, so the
slave is reported UP for half an interval without having received
anything.

Patch 2 fixes a slave that starts UP. The seed brings its down deadline
forward by one interval compared to what arp_missed_max implies. In
load-balance mode, which has no grace period after enslavement, a
healthy slave can flap DOWN with arp_missed_max=1.

Jay, on your v3 question: the up test always uses a single interval, so
the fix for a slave that starts DOWN holds for any arp_missed_max. But
v3 also changed the seed of a slave that starts UP, which made the
load-balance flap happen with the default arp_missed_max=2 too. v4 only
uses the older seed for a slave that starts DOWN, and patch 2 gives a
slave that starts UP a full arp_missed_max window.

Signed-off-by: Ramses de Norre <ramses@well-founded.dev>
---
Changes in v4:
- Patch 1: only use the older seed for a slave that starts DOWN. v3
  also applied it to a slave that starts UP, which brought its down
  deadline forward by one interval (Jay).
- New patch 2: seed a slave that starts UP with last_link_up, giving it
  a full arp_missed_max window. Fixes an existing flap in load-balance
  mode with arp_missed_max=1.
- Drop Hangbin's Reviewed-by, patch 1 changed.
- Link to v3: https://patch.msgid.link/20261027-bonding-last-rx-v3-1-54907f1fb042@well-founded.dev

Changes in v3:
- No code changes. Rebase on current net.
- Reword the commit message along the lines of the v2 discussion: lead
  with what the last_rx seed is for, state that the false UP happens
  independent of primary_reselect, and that the initial link state is
  unchanged.
- Pick up Reviewed-by from Hangbin.
- Cc Nikolay. Hangbin asked for a second review, Jay or Nikolay, would
  you have a look?
- Link to v2: https://patch.msgid.link/20260827-bonding-last-rx-v2-1-7c3833822c23@well-founded.dev

Changes in v2:
- No code changes. Resend with my personal name in the From and
  Signed-off-by lines instead of my handle, as requested for the DCO.
- Link to v1: https://patch.msgid.link/20260817-bonding-last-rx-v1-1-9da6f7fdf812@well-founded.dev

---
Ramses de Norre (2):
      bonding: fix initial last_rx vs ARP-monitor slack window
      bonding: give a slave that starts UP a full arp_missed_max window

 drivers/net/bonding/bond_main.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)
---
base-commit: f75062536e2d45ee182828f0e47e0124be2fdc8d
change-id: 20260817-bonding-last-rx-0303fa048f9d

Best regards,
--  
Ramses de Norre <ramses@well-founded.dev>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window
  2026-10-28 11:48 [PATCH net v4 0/2] bonding: fix the initial last_rx seed for the ARP monitor Ramses de Norre via B4 Relay
@ 2026-10-28 11:48 ` Ramses de Norre via B4 Relay
  2026-10-30  2:59   ` Hangbin Liu
  2026-10-30 11:50   ` netdev-bot+sashiko
  2026-10-28 11:48 ` [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window Ramses de Norre via B4 Relay
  1 sibling, 2 replies; 8+ messages in thread
From: Ramses de Norre via B4 Relay @ 2026-10-28 11:48 UTC (permalink / raw)
  To: Jay Vosburgh, Andrew Lunn, David S. Miller, Jakub Kicinski,
	Paolo Abeni, Jiri Bohac, Eric Dumazet
  Cc: netdev, linux-kernel, Nikolay Aleksandrov, Hangbin Liu,
	Ramses de Norre

From: Ramses de Norre <ramses@well-founded.dev>

bond_enslave() seeds a new slave's last_rx so that the ARP monitor does
not report it UP before a real ARP reply arrives, see commit
f31c7937c254 ("bonding: start slaves with link down for ARP monitor").

The seed is one arp_interval (plus a tick) in the past. That was old
enough when the up test was "jiffies <= last_rx + arp_interval". Commit
da210f559019 ("bonding: add some slack to arp monitoring time limits")
added arp_interval/2 of slack to that test without moving the seed, so
the seed now falls inside the up window. For the first half interval
after enslavement, bond_ab_arp_inspect() proposes a slave that starts
DOWN as UP although it has received nothing. The up test always uses a
single interval, so this does not depend on primary_reselect or
arp_missed_max.

If that slave is the configured primary, force_primary makes it the
active slave. ARP validation fails it shortly after (on SFP+ ports the
PHY is often still negotiating) and the bond falls back to the backup.
With primary_reselect=always the bond recovers once the port really
comes up, but drops traffic until then. With primary_reselect=failure,
force_primary is spent and the bond stays on the backup.

Reproducer:

  ip link add bond0 type bond mode active-backup arp_interval 1000 \
      arp_validate all arp_ip_target 192.0.2.1 \
      primary eth0 primary_reselect failure
  # eth0: SFP+ (slow link-up), eth1: RJ45 (fast link-up)
  ip link set eth0 master bond0
  ip link set eth1 master bond0
  ip link set bond0 up
  # bond0 lands on eth0 via force_primary, ARP-fails it before the
  # SFP+ has carrier, falls to eth1, and stays there.

Move the seed below the initial link state and seed a slave that starts
DOWN two intervals in the past, outside the up window. A slave that
starts UP keeps its current seed: the up test does not apply to it, and
an older seed would only bring its down deadline forward by one
interval. The initial link state is unchanged.

Fixes: da210f559019 ("bonding: add some slack to arp monitoring time limits")
Signed-off-by: Ramses de Norre <ramses@well-founded.dev>
---
 drivers/net/bonding/bond_main.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7019e5d8fdc8e..bfa4cb97e647c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2122,13 +2122,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (res && bond_needs_speed_duplex(bond))
 		new_slave->link = BOND_LINK_DOWN;
 
-	new_slave->last_rx = jiffies -
-		(msecs_to_jiffies(bond->params.arp_interval) + 1);
-	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
-		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
-
-	new_slave->last_tx = new_slave->last_rx;
-
 	/* check for initial state */
 	new_slave->link = BOND_LINK_NOCHANGE;
 	if (bond->params.miimon) {
@@ -2159,6 +2152,22 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	if (new_slave->link != BOND_LINK_DOWN)
 		new_slave->last_link_up = jiffies;
+
+	/* The ARP monitor's up test includes arp_interval/2 of slack, see
+	 * bond_time_in_interval(). Seed a slave that starts DOWN outside
+	 * that window, so it stays DOWN until a real reply arrives.
+	 */
+	if (new_slave->link == BOND_LINK_DOWN)
+		new_slave->last_rx = jiffies -
+			(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);
+	else
+		new_slave->last_rx = jiffies -
+			(msecs_to_jiffies(bond->params.arp_interval) + 1);
+	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
+		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
+
+	new_slave->last_tx = new_slave->last_rx;
+
 	slave_dbg(bond_dev, slave_dev, "Initial state of slave is BOND_LINK_%s\n",
 		  new_slave->link == BOND_LINK_DOWN ? "DOWN" :
 		  (new_slave->link == BOND_LINK_UP ? "UP" : "BACK"));

-- 
2.56.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window
  2026-10-28 11:48 [PATCH net v4 0/2] bonding: fix the initial last_rx seed for the ARP monitor Ramses de Norre via B4 Relay
  2026-10-28 11:48 ` [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window Ramses de Norre via B4 Relay
@ 2026-10-28 11:48 ` Ramses de Norre via B4 Relay
  2026-10-30 11:50   ` netdev-bot+sashiko
  1 sibling, 1 reply; 8+ messages in thread
From: Ramses de Norre via B4 Relay @ 2026-10-28 11:48 UTC (permalink / raw)
  To: Jay Vosburgh, Andrew Lunn, David S. Miller, Jakub Kicinski,
	Paolo Abeni, Jiri Bohac, Eric Dumazet
  Cc: netdev, linux-kernel, Nikolay Aleksandrov, Hangbin Liu,
	Ramses de Norre

From: Ramses de Norre <ramses@well-founded.dev>

The ARP monitor marks an UP slave DOWN once jiffies passes
last_rx + arp_missed_max * arp_interval + arp_interval/2. bond_enslave()
seeds last_rx one arp_interval (plus a tick) in the past, which brings
the down deadline of a slave that starts UP forward by one interval
compared to what arp_missed_max promises.

Active-backup mode hides this with a grace period after enslavement in
bond_ab_arp_inspect(). Load-balance mode has no grace period after
enslavement. With arp_missed_max=1 the deadline is half an interval
after enslavement, and the first monitor run checks the slave before it
sends the first probe. If that run lands in the second half of the
interval, a healthy slave is reported "interface is now down" and comes
back UP on the next run.

Seed a slave that starts UP with last_link_up instead, so it gets a full
arp_missed_max window for its first reply. bond_arp_rcv() and
bond_na_rcv() only trust ARPs seen on backup slaves once the active
slave's last_rx is after its last_link_up. A seed equal to last_link_up
keeps that check false until a real reply arrives, as it is today.

Fixes: 5944b5abd864 ("Bonding: add arp_missed_max option")
Signed-off-by: Ramses de Norre <ramses@well-founded.dev>
---
 drivers/net/bonding/bond_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bfa4cb97e647c..de5b196f4a1ee 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2160,9 +2160,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (new_slave->link == BOND_LINK_DOWN)
 		new_slave->last_rx = jiffies -
 			(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);
+	/* Give a slave that starts UP a full arp_missed_max window. Not
+	 * seeding it later than last_link_up keeps bond_arp_rcv() from
+	 * trusting ARPs on backup slaves before the active one got a reply.
+	 */
 	else
-		new_slave->last_rx = jiffies -
-			(msecs_to_jiffies(bond->params.arp_interval) + 1);
+		new_slave->last_rx = new_slave->last_link_up;
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
 		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
 

-- 
2.56.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window
  2026-10-28 11:48 ` [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window Ramses de Norre via B4 Relay
@ 2026-10-30  2:59   ` Hangbin Liu
  2026-10-30 11:50   ` netdev-bot+sashiko
  1 sibling, 0 replies; 8+ messages in thread
From: Hangbin Liu @ 2026-10-30  2:59 UTC (permalink / raw)
  To: ramses
  Cc: Jay Vosburgh, Andrew Lunn, David S. Miller, Jakub Kicinski,
	Paolo Abeni, Jiri Bohac, Eric Dumazet, netdev, linux-kernel,
	Nikolay Aleksandrov

On Wed, Oct 28, 2026 at 12:48:07PM +0100, Ramses de Norre via B4 Relay wrote:
> From: Ramses de Norre <ramses@well-founded.dev>
> 
> bond_enslave() seeds a new slave's last_rx so that the ARP monitor does
> not report it UP before a real ARP reply arrives, see commit
> f31c7937c254 ("bonding: start slaves with link down for ARP monitor").
> 
> The seed is one arp_interval (plus a tick) in the past. That was old
> enough when the up test was "jiffies <= last_rx + arp_interval". Commit
> da210f559019 ("bonding: add some slack to arp monitoring time limits")
> added arp_interval/2 of slack to that test without moving the seed, so
> the seed now falls inside the up window. For the first half interval
> after enslavement, bond_ab_arp_inspect() proposes a slave that starts
> DOWN as UP although it has received nothing. The up test always uses a
> single interval, so this does not depend on primary_reselect or
> arp_missed_max.
> 
> If that slave is the configured primary, force_primary makes it the
> active slave. ARP validation fails it shortly after (on SFP+ ports the
> PHY is often still negotiating) and the bond falls back to the backup.
> With primary_reselect=always the bond recovers once the port really
> comes up, but drops traffic until then. With primary_reselect=failure,
> force_primary is spent and the bond stays on the backup.
> 
> Reproducer:
> 
>   ip link add bond0 type bond mode active-backup arp_interval 1000 \
>       arp_validate all arp_ip_target 192.0.2.1 \
>       primary eth0 primary_reselect failure
>   # eth0: SFP+ (slow link-up), eth1: RJ45 (fast link-up)
>   ip link set eth0 master bond0
>   ip link set eth1 master bond0
>   ip link set bond0 up
>   # bond0 lands on eth0 via force_primary, ARP-fails it before the
>   # SFP+ has carrier, falls to eth1, and stays there.
> 
> Move the seed below the initial link state and seed a slave that starts
> DOWN two intervals in the past, outside the up window. A slave that
> starts UP keeps its current seed: the up test does not apply to it, and
> an older seed would only bring its down deadline forward by one
> interval. The initial link state is unchanged.
> 
> Fixes: da210f559019 ("bonding: add some slack to arp monitoring time limits")
> Signed-off-by: Ramses de Norre <ramses@well-founded.dev>
> ---
>  drivers/net/bonding/bond_main.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7019e5d8fdc8e..bfa4cb97e647c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2122,13 +2122,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	if (res && bond_needs_speed_duplex(bond))
>  		new_slave->link = BOND_LINK_DOWN;
>  
> -	new_slave->last_rx = jiffies -
> -		(msecs_to_jiffies(bond->params.arp_interval) + 1);
> -	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
> -		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
> -
> -	new_slave->last_tx = new_slave->last_rx;
> -
>  	/* check for initial state */
>  	new_slave->link = BOND_LINK_NOCHANGE;
>  	if (bond->params.miimon) {
> @@ -2159,6 +2152,22 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  
>  	if (new_slave->link != BOND_LINK_DOWN)
>  		new_slave->last_link_up = jiffies;

Nit: This part could move down and merge with the "else" part.

> +
> +	/* The ARP monitor's up test includes arp_interval/2 of slack, see
> +	 * bond_time_in_interval(). Seed a slave that starts DOWN outside
> +	 * that window, so it stays DOWN until a real reply arrives.
> +	 */
> +	if (new_slave->link == BOND_LINK_DOWN)
> +		new_slave->last_rx = jiffies -
> +			(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);
> +	else
> +		new_slave->last_rx = jiffies -
> +			(msecs_to_jiffies(bond->params.arp_interval) + 1);
> +	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
> +		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
> +
> +	new_slave->last_tx = new_slave->last_rx;
> +
>  	slave_dbg(bond_dev, slave_dev, "Initial state of slave is BOND_LINK_%s\n",
>  		  new_slave->link == BOND_LINK_DOWN ? "DOWN" :
>  		  (new_slave->link == BOND_LINK_UP ? "UP" : "BACK"));
> 
> -- 
> 2.56.0
> 
> 

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window
  2026-10-28 11:48 ` [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window Ramses de Norre via B4 Relay
  2026-10-30  2:59   ` Hangbin Liu
@ 2026-10-30 11:50   ` netdev-bot+sashiko
  2026-10-30 14:46     ` Ramses
  1 sibling, 1 reply; 8+ messages in thread
From: netdev-bot+sashiko @ 2026-10-30 11:50 UTC (permalink / raw)
  To: ramses
  Cc: jv, andrew+netdev, davem, kuba, pabeni, jbohac, edumazet, netdev,
	linux-kernel, razor, hangbin.liu

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

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

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] The new DOWN-slave seed in bond_enslave()
  (drivers/net/bonding/bond_main.c:2160-2162, `last_rx = jiffies - (2 *…
- [Low] The new comment in bond_enslave() (bond_main.c:2156-2159) says
  the DOWN seed keeps the slave DOWN 'until a real reply arrives'.

--

bonding: fix initial last_rx vs ARP-monitor slack window

This patch moves the last_rx, target_last_arp_rx[] and last_tx seeding in
bond_enslave() so it runs after the initial link state is set. A slave
that starts DOWN is now seeded two arp_intervals in the past, which puts
it outside the arp_interval/2 slack that bond_time_in_interval() adds.
Slaves that start UP keep the old seed.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7019e5d8fdc8e..bfa4cb97e647c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c

[ ... ]

> @@ -2159,6 +2152,22 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  
>  	if (new_slave->link != BOND_LINK_DOWN)
>  		new_slave->last_link_up = jiffies;
> +
> +	/* The ARP monitor's up test includes arp_interval/2 of slack, see
> +	 * bond_time_in_interval(). Seed a slave that starts DOWN outside
> +	 * that window, so it stays DOWN until a real reply arrives.
> +	 */

[Severity: Low]
This isn't a bug, but is "until a real reply arrives" accurate? More than
ARP replies can move last_rx forward.

With arp_validate=none (the default), bond_rcv_validate() updates last_rx
for any received frame:

drivers/net/bonding/bond_main.c:bond_rcv_validate() {
    ...
	if (!slave_do_arp_validate(bond, slave)) {
		if ((slave_do_arp_validate_only(bond) && is_arp) ||
    ...
		    !slave_do_arp_validate_only(bond))
			WRITE_ONCE(slave->last_rx, jiffies);
    ...
}

With backup validation, bond_arp_rcv() also accepts broadcast ARP requests
on backup slaves, with sip and tip swapped:

drivers/net/bonding/bond_main.c:bond_arp_rcv() {
    ...
	else if (curr_active_slave &&
		 time_after(slave_last_rx(bond, curr_active_slave),
			    curr_active_slave->last_link_up))
		bond_validate_arp(bond, slave, tip, sip);
    ...
}

Would wording like "until it receives qualifying traffic" fit better? The
commit message makes the same claim and may need the same change.

> +	if (new_slave->link == BOND_LINK_DOWN)
> +		new_slave->last_rx = jiffies -
> +			(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);

[Severity: Medium]
This seed uses the arp_interval in effect at enslave time. What happens if
ARP monitoring is turned on later, or the interval is raised?

Here are two ways the seed can end up inside the up window.

If the slave is enslaved under miimon (arp_interval == 0) with no carrier,
it starts BOND_LINK_DOWN and the seed is just jiffies - 1.

If the slave is enslaved with, say, arp_interval=100 and the interval is
then raised to 1000, the seed (t0 - (2 * d_old + 1)) still falls inside
the window that bond_time_in_interval() builds from the new interval:

drivers/net/bonding/bond_main.c:bond_time_in_interval() {
	int delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);

	return time_in_range(jiffies,
			     last_act - delta_in_ticks,
			     last_act + mod * delta_in_ticks + delta_in_ticks/2);
}

In both cases bond_option_arp_interval_set() writes the new interval and
starts arp_work right away. It does not reseed last_rx,
target_last_arp_rx[] or last_tx on existing slaves:

drivers/net/bonding/bond_options.c:bond_option_arp_interval_set() {
    ...
			WRITE_ONCE(bond->recv_probe, bond_rcv_validate);
			cancel_delayed_work_sync(&bond->mii_work);
			queue_delayed_work(bond->wq, &bond->arp_work, 0);
    ...
}

The path then runs bond_activebackup_arp_mon() -> bond_ab_arp_inspect(),
and bond_time_in_interval(bond, last_rx, 1) returns true. That proposes
BOND_LINK_UP for a slave with no carrier that has received nothing.

If that slave is the configured primary, force_primary may still be
pending. Under miimon the primary was DOWN, so
bond_choose_primary_or_current() never used up force_primary. The primary
then becomes active, fails ARP validation, and with
primary_reselect=failure the bond stays on the backup. That is the same
result as the reproducer in the commit message.

arp_interval has no BOND_OPTFLAG_NOSLAVES, and bond_open() does not reseed
either. The follow-up patch in this series ("bonding: give a slave that
starts UP a full arp_missed_max window") only changes the UP branch.

Should bond_option_arp_interval_set() also reseed slaves that are DOWN?

> +	else
> +		new_slave->last_rx = jiffies -
> +			(msecs_to_jiffies(bond->params.arp_interval) + 1);
> +	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
> +		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
> +
> +	new_slave->last_tx = new_slave->last_rx;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-bonding-last-rx-v4-0-4d8e4cd47467%40well-founded.dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window
  2026-10-28 11:48 ` [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window Ramses de Norre via B4 Relay
@ 2026-10-30 11:50   ` netdev-bot+sashiko
  2026-10-30 14:47     ` Ramses
  0 siblings, 1 reply; 8+ messages in thread
From: netdev-bot+sashiko @ 2026-10-30 11:50 UTC (permalink / raw)
  To: ramses
  Cc: jv, andrew+netdev, davem, kuba, pabeni, jbohac, edumazet, netdev,
	linux-kernel, razor, hangbin.liu

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

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

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 patch only covers a slave enslaved while the ARP monitor is
  already running.

--

bonding: give a slave that starts UP a full arp_missed_max window

This commit changes bond_enslave() so that a slave that starts UP has
last_rx seeded from last_link_up. Before, it was seeded one arp_interval
in the past. The goal is a full arp_missed_max window before the
load-balance ARP monitor can report the slave down.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index bfa4cb97e647c..de5b196f4a1ee 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2160,9 +2160,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	if (new_slave->link == BOND_LINK_DOWN)
>  		new_slave->last_rx = jiffies -
>  			(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);
> +	/* Give a slave that starts UP a full arp_missed_max window. Not
> +	 * seeding it later than last_link_up keeps bond_arp_rcv() from
> +	 * trusting ARPs on backup slaves before the active one got a reply.
> +	 */
>  	else
> -		new_slave->last_rx = jiffies -
> -			(msecs_to_jiffies(bond->params.arp_interval) + 1);
> +		new_slave->last_rx = new_slave->last_link_up;

[Severity: High]
Does this fix still miss the case where the ARP monitor starts some time
after enslavement?

The new seed is the enslave time. It goes into last_rx, every
target_last_arp_rx[i] and, a few lines below, last_tx. If the bond is down
when the slave is enslaved, recv_probe is NULL and arp_work is not queued.
So last_rx is not updated by bond_rcv_validate(), and last_tx is not
updated by bond_arp_send()/bond_ns_send().

Later, bond_open() starts the monitor right away without re-seeding these
fields:

drivers/net/bonding/bond_main.c:bond_open() {
    ...
	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
		queue_delayed_work(bond->wq, &bond->arp_work, 0);
		WRITE_ONCE(bond->recv_probe, bond_rcv_validate);
	}
    ...
}

bond_option_arp_interval_set() does the same thing when arp_interval is
turned on at runtime.

The first bond_loadbalance_arp_mon() run then checks the enslave-time
values, before bond_send_validate() has sent any probe on the slave:

drivers/net/bonding/bond_main.c:bond_loadbalance_arp_mon() {
    ...
			if (!bond_time_in_interval(bond, last_tx,
						   bond->params.missed_max) ||
			    !bond_time_in_interval(bond, READ_ONCE(slave->last_rx),
						   bond->params.missed_max)) {

				bond_propose_link_state(slave, BOND_LINK_DOWN);
    ...
				slave_info(bond->dev, slave->dev, "interface is now down\n");
    ...
}

Take missed_max=1 and arp_interval=1000. If more than 1.5s passes between
enslave and "ip link set bondX up", a healthy slave is marked down. Its
link_failure_count goes up, and failover runs if it was curr_active_slave.
The next run brings it back up.

If arp_interval is enabled on a running bond whose slaves were enslaved
long ago, every UP slave could go down at the same time for one interval.

The old seed behaved the same way here. Still, the commit message says the
slave "gets a full arp_missed_max window for its first reply". That does
not seem to hold when monitoring starts after enslavement.

Would it make sense to also re-seed last_rx, last_tx, target_last_arp_rx[]
and last_link_up when the ARP monitor is started in bond_open() and
bond_option_arp_interval_set()?

>  	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
>  		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-bonding-last-rx-v4-0-4d8e4cd47467%40well-founded.dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window
  2026-10-30 11:50   ` netdev-bot+sashiko
@ 2026-10-30 14:46     ` Ramses
  0 siblings, 0 replies; 8+ messages in thread
From: Ramses @ 2026-10-30 14:46 UTC (permalink / raw)
  To: Netdev Bot+sashiko
  Cc: Jv, Andrew+netdev, Davem, Kuba, Pabeni, Jbohac, Edumazet, Netdev,
	Linux Kernel Mailing List, Razor, Hangbin Liu

> [Severity: Low]
> This isn't a bug, but is "until a real reply arrives" accurate? More than
> ARP replies can move last_rx forward.

Right, with arp_validate=none any received frame updates last_rx. v5
says "until it receives traffic".

> [Severity: Medium]
> This seed uses the arp_interval in effect at enslave time. What happens if
> ARP monitoring is turned on later, or the interval is raised?

That issue already existed before this series. The seed was already
 computed from arp_interval at enslave time and is not refreshed when the
 ARP monitor starts, from bond_open() or bond_option_arp_interval_set().
 This patch does not make it worse. Reseeding the slaves when the monitor
 starts touches different code paths, so I would rather do that in a separate
follow-up than grow the current patch series.

pw-bot: cr


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window
  2026-10-30 11:50   ` netdev-bot+sashiko
@ 2026-10-30 14:47     ` Ramses
  0 siblings, 0 replies; 8+ messages in thread
From: Ramses @ 2026-10-30 14:47 UTC (permalink / raw)
  To: Netdev Bot+sashiko
  Cc: Jv, Andrew+netdev, Davem, Kuba, Pabeni, Jbohac, Edumazet, Netdev,
	Linux Kernel Mailing List, Razor, Hangbin Liu

> [Severity: High]
> Does this fix still miss the case where the ARP monitor starts some time
> after enslavement?

Yes. As the review notes, the old seed behaved the same way there, so
this is not a regression. v5 narrows the commit message to a slave
enslaved while the ARP monitor is running. Reseeding when the monitor
starts later will be a separate follow-up, see my reply on patch 1.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-10-30 14:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-28 11:48 [PATCH net v4 0/2] bonding: fix the initial last_rx seed for the ARP monitor Ramses de Norre via B4 Relay
2026-10-28 11:48 ` [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window Ramses de Norre via B4 Relay
2026-10-30  2:59   ` Hangbin Liu
2026-10-30 11:50   ` netdev-bot+sashiko
2026-10-30 14:46     ` Ramses
2026-10-28 11:48 ` [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window Ramses de Norre via B4 Relay
2026-10-30 11:50   ` netdev-bot+sashiko
2026-10-30 14:47     ` Ramses

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox