Re: [PATCH v3 net 1/4] net: bonding: replace dev_trans_start() with the jiffies of the last ARP/NS

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

>The bonding driver piggybacks on time stamps kept by the network stack
>for the purpose of the netdev TX watchdog, and this is problematic
>because it does not work with NETIF_F_LLTX devices.
>
>It is hard to say why the driver looks at dev_trans_start() of the
>slave->dev, considering that this is updated even by non-ARP/NS probes
>sent by us, and even by traffic not sent by us at all (for example PTP
>on physical slave devices). ARP monitoring in active-backup mode appears
>to still work even if we track only the last TX time of actual ARP
>probes.

	Because it's the closest it can get to "have we sent an ARP,"
really.  The issue with LLTX is relatively new (the bonding driver has
worked this way for longer than I've been involved, so I don't know what
the original design decisions were).

	FWIW, I've been working with the following, which is closer in
spirit to what Jakub and I discussed previously (i.e., inspecting the
device stats for virtual devices, relying on dev_trans_start for
physical devices with ndo_tx_timeout).

	This WIP includes one unrelated change: including the ifindex in
the route lookup; that would be a separate patch if it ends up being
submitted (it handles the edge case of a route on an interface other
than the bond matching before the bond itself).

	-J

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e75acb14d066..507ff5d50585 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -339,6 +339,36 @@ static bool bond_xdp_check(struct bonding *bond)
 	}
 }
 
+static void bond_poll_tx_packets(struct slave *slave)
+{
+	struct net_device *dev = slave->dev;
+	struct rtnl_link_stats64 stats;
+
+	dev_get_stats(dev, &stats);
+	if (stats.tx_packets != slave->last_tx_packets) {
+		slave->last_tx_change = jiffies;
+		slave->last_tx_packets = stats.tx_packets;
+	}
+}
+
+/* Determine time of last transmit for slave.
+ *
+ * For device drivers that maintain trans_start (presumed to be any device
+ * that implements an ndo_tx_timeout), use dev_trans_start.  For other
+ * devices (typically virtual devices), inspect interface statistics for
+ * recent change to tx_packets.
+ */
+static unsigned long bond_dev_trans_start(struct slave *slave)
+{
+	struct net_device *dev = slave->dev;
+
+	if (dev->netdev_ops->ndo_tx_timeout)
+		return dev_trans_start(dev);
+
+	bond_poll_tx_packets(slave);
+	return slave->last_tx_change;
+}
+
 /*---------------------------------- VLAN -----------------------------------*/
 
 /* In the following 2 functions, bond_vlan_rx_add_vid and bond_vlan_rx_kill_vid,
@@ -2943,7 +2973,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 
 		/* Find out through which dev should the packet go */
 		rt = ip_route_output(dev_net(bond->dev), targets[i], 0,
-				     RTO_ONLINK, 0);
+				     RTO_ONLINK, bond->dev->ifindex);
 		if (IS_ERR(rt)) {
 			/* there's no route to target - try to send arp
 			 * probe to generate any traffic (arp_validate=0)
@@ -3075,7 +3105,7 @@ static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 		bond_validate_arp(bond, slave, tip, sip);
 	else if (curr_arp_slave && (arp->ar_op == htons(ARPOP_REPLY)) &&
 		 bond_time_in_interval(bond,
-				       dev_trans_start(curr_arp_slave->dev), 1))
+				       bond_dev_trans_start(curr_arp_slave), 1))
 		bond_validate_arp(bond, slave, sip, tip);
 
 out_unlock:
@@ -3247,7 +3277,7 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
 		bond_validate_ns(bond, slave, saddr, daddr);
 	else if (curr_arp_slave &&
 		 bond_time_in_interval(bond,
-				       dev_trans_start(curr_arp_slave->dev), 1))
+				       bond_dev_trans_start(curr_arp_slave), 1))
 		bond_validate_ns(bond, slave, saddr, daddr);
 
 out:
@@ -3335,7 +3365,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
 	 *       so it can wait
 	 */
 	bond_for_each_slave_rcu(bond, slave, iter) {
-		unsigned long trans_start = dev_trans_start(slave->dev);
+		unsigned long trans_start = bond_dev_trans_start(slave);
 
 		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
@@ -3482,7 +3512,6 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		 * - (more than missed_max*delta since receive AND
 		 *    the bond has an IP address)
 		 */
-		trans_start = dev_trans_start(slave->dev);
 		if (bond_is_active_slave(slave) &&
 		    (!bond_time_in_interval(bond, trans_start, bond->params.missed_max) ||
 		     !bond_time_in_interval(bond, last_rx, bond->params.missed_max))) {
@@ -3511,7 +3540,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
 			continue;
 
 		case BOND_LINK_UP:
-			trans_start = dev_trans_start(slave->dev);
+			trans_start = bond_dev_trans_start(slave);
 			if (rtnl_dereference(bond->curr_active_slave) != slave ||
 			    (!rtnl_dereference(bond->curr_active_slave) &&
 			     bond_time_in_interval(bond, trans_start, 1))) {
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 6e78d657aa05..6449fe755e8a 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -165,6 +165,8 @@ struct slave {
 	unsigned long last_link_up;
 	unsigned long last_rx;
 	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
+	unsigned long last_tx_packets;
+	unsigned long last_tx_change;
 	s8     link;		/* one of BOND_LINK_XXXX */
 	s8     link_new_state;	/* one of BOND_LINK_XXXX */
 	u8     backup:1,   /* indicates backup slave. Value corresponds with

---
	-Jay Vosburgh, jay.vosburgh@canonical.com