Re: [PATCH net v4 2/2] bonding: give a slave that starts UP a full arp_missed_max window

[netdev-bot+sashiko]

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