Re: [net-next v6 3/4] net: bonding: send peer notify when failure recovery

[Jay Vosburgh <jv@jvosburgh.net>]

Tonghao Zhang <tonghao@bamaicloud.com> wrote:

>After LACP protocol recovery, the port can transmit packets.
>However, if the bond port doesn't send gratuitous ARP/ND
>packets to the switch, the switch won't return packets through
>the current interface. This causes traffic imbalance. To resolve
>this issue, when LACP protocol recovers, send ARP/ND packets.

	I think the description above needs to mention that the
gratuitous ARP/ND only happens if broadcast_neighbor is enabled.

	I'll note that the documentation update does include this
caveat.

>Cc: Jay Vosburgh <jv@jvosburgh.net>
>Cc: "David S. Miller" <davem@davemloft.net>
>Cc: Eric Dumazet <edumazet@google.com>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Cc: Paolo Abeni <pabeni@redhat.com>
>Cc: Simon Horman <horms@kernel.org>
>Cc: Jonathan Corbet <corbet@lwn.net>
>Cc: Andrew Lunn <andrew+netdev@lunn.ch>
>Cc: Steven Rostedt <rostedt@goodmis.org>
>Cc: Masami Hiramatsu <mhiramat@kernel.org>
>Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>Cc: Nikolay Aleksandrov <razor@blackwall.org>
>Signed-off-by: Tonghao Zhang <tonghao@bamaicloud.com>
>Signed-off-by: Zengbing Tu <tuzengbing@didiglobal.com>
>Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
>---
> Documentation/networking/bonding.rst |  5 +++--
> drivers/net/bonding/bond_3ad.c       | 13 +++++++++++++
> drivers/net/bonding/bond_main.c      | 21 ++++++++++++++++-----
> 3 files changed, 32 insertions(+), 7 deletions(-)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index 14f7593d888d..f8f5766703d4 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -773,8 +773,9 @@ num_unsol_na
> 	greater than 1.
> 
> 	The valid range is 0 - 255; the default value is 1.  These options
>-	affect only the active-backup mode.  These options were added for
>-	bonding versions 3.3.0 and 3.4.0 respectively.
>+	affect the active-backup or 802.3ad (broadcast_neighbor enabled) mode.
>+	These options were added for bonding versions 3.3.0 and 3.4.0
>+	respectively.
> 
> 	From Linux 3.0 and bonding version 3.7.1, these notifications
> 	are generated by the ipv4 and ipv6 code and the numbers of
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index c6807e473ab7..d1c2d416ac87 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -982,6 +982,17 @@ static int ad_marker_send(struct port *port, struct bond_marker *marker)
> 	return 0;
> }
> 
>+static void ad_cond_set_peer_notif(struct port *port)
>+{
>+	struct bonding *bond = port->slave->bond;
>+
>+	if (bond->params.broadcast_neighbor && rtnl_trylock()) {
>+		bond->send_peer_notif = bond->params.num_peer_notif *
>+			max(1, bond->params.peer_notif_delay);
>+		rtnl_unlock();
>+	}
>+}
>+
> /**
>  * ad_mux_machine - handle a port's mux state machine
>  * @port: the port we're looking at
>@@ -2061,6 +2072,8 @@ static void ad_enable_collecting_distributing(struct port *port,
> 		__enable_port(port);
> 		/* Slave array needs update */
> 		*update_slave_arr = true;
>+		/* Should notify peers if possible */
>+		ad_cond_set_peer_notif(port);
> 	}
> }
> 
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 12046ef51569..0acece55d9cb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1237,17 +1237,28 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
> /* must be called in RCU critical section or with RTNL held */
> static bool bond_should_notify_peers(struct bonding *bond)
> {
>-	struct slave *slave = rcu_dereference_rtnl(bond->curr_active_slave);
>+	struct bond_up_slave *usable;
>+	struct slave *slave = NULL;
> 
>-	if (!slave || !bond->send_peer_notif ||
>+	if (!bond->send_peer_notif ||
> 	    bond->send_peer_notif %
> 	    max(1, bond->params.peer_notif_delay) != 0 ||
>-	    !netif_carrier_ok(bond->dev) ||
>-	    test_bit(__LINK_STATE_LINKWATCH_PENDING, &slave->dev->state))
>+	    !netif_carrier_ok(bond->dev))
> 		return false;
> 
>+	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>+		usable = rcu_dereference_rtnl(bond->usable_slaves);
>+		if (!usable || !READ_ONCE(usable->count))
>+			return false;
>+	} else {
>+		slave = rcu_dereference_rtnl(bond->curr_active_slave);
>+		if (!slave || test_bit(__LINK_STATE_LINKWATCH_PENDING,
>+				       &slave->dev->state))
>+			return false;
>+	}
>+
> 	netdev_dbg(bond->dev, "bond_should_notify_peers: slave %s\n",
>-		   slave ? slave->dev->name : "NULL");
>+		   slave ? slave->dev->name : "all");

	Is it actually correct that if slave == NULL, the notify peers
logic will send to all ports?  I'm not sure why this changed.

	-J

> 
> 	return true;
> }
>-- 
>2.34.1

---
	-Jay Vosburgh, jv@jvosburgh.net