Message-ID: <1134101.1746883017@vermin>
Date: Sat, 10 May 2025 15:16:57 +0200
From: Jay Vosburgh <jv@...sburgh.net>
To: David J Wilder <wilder@...ibm.com>
cc: netdev@...r.kernel.org, pradeeps@...ux.vnet.ibm.com,
    pradeep@...ibm.com, i.maximets@....org, amorenoz@...hat.com,
    haliu@...hat.com
Subject: Re: [PATCH net-next v1 1/2] bonding: Adding struct arp_target

David J Wilder <wilder@...ibm.com> wrote:

>Replacing the definition of bond_params.arp_targets (__be32 arp_targets[])
>with:
>
>struct arp_target {
>        __be32 target_ip;
>        struct bond_vlan_tag *tags;
>};

	Since this struct is only for bonding, perhaps "struct
bond_arp_target"?

>To provide storage for a list of vlan tags for each target.
>
>All references to arp_target are change to use the new structure.
>
>Signed-off-by: David J Wilder <wilder@...ibm.com>
>---
> drivers/net/bonding/bond_main.c    | 29 ++++++++++++++++-------------
> drivers/net/bonding/bond_netlink.c |  4 ++--
> drivers/net/bonding/bond_options.c | 18 +++++++++---------
> drivers/net/bonding/bond_procfs.c  |  4 ++--
> drivers/net/bonding/bond_sysfs.c   |  4 ++--
> include/net/bonding.h              | 15 ++++++++++-----
> 6 files changed, 41 insertions(+), 33 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index d05226484c64..ab388dab218a 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3151,26 +3151,29 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> {
> 	struct rtable *rt;
> 	struct bond_vlan_tag *tags;
>-	__be32 *targets = bond->params.arp_targets, addr;
>+	struct arp_target *targets = bond->params.arp_targets;
>+	__be32 target_ip, addr;
> 	int i;
> 
>-	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) {
>+	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i].target_ip; i++) {
>+		target_ip = targets[i].target_ip;
>+		tags = targets[i].tags;
>+
> 		slave_dbg(bond->dev, slave->dev, "%s: target %pI4\n",
>-			  __func__, &targets[i]);
>-		tags = NULL;
>+			  __func__, &target_ip);

	Perhaps add the tags to the debug message?  It might be kind of
a pain to format, but seems like it would be useful.

	-J

> 
> 		/* Find out through which dev should the packet go */
>-		rt = ip_route_output(dev_net(bond->dev), targets[i], 0, 0, 0,
>+		rt = ip_route_output(dev_net(bond->dev), target_ip, 0, 0, 0,
> 				     RT_SCOPE_LINK);
> 		if (IS_ERR(rt)) {
>-			/* there's no route to target - try to send arp
>+			/* there's no route to target_ip - try to send arp
> 			 * probe to generate any traffic (arp_validate=0)
> 			 */
> 			if (bond->params.arp_validate)
> 				pr_warn_once("%s: no route to arp_ip_target %pI4 and arp_validate is set\n",
> 					     bond->dev->name,
>-					     &targets[i]);
>-			bond_arp_send(slave, ARPOP_REQUEST, targets[i],
>+					     &target_ip);
>+			bond_arp_send(slave, ARPOP_REQUEST, target_ip,
> 				      0, tags);
> 			continue;
> 		}
>@@ -3188,15 +3191,15 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> 
> 		/* Not our device - skip */
> 		slave_dbg(bond->dev, slave->dev, "no path to arp_ip_target %pI4 via rt.dev %s\n",
>-			   &targets[i], rt->dst.dev ? rt->dst.dev->name : "NULL");
>+			   &target_ip, rt->dst.dev ? rt->dst.dev->name : "NULL");
> 
> 		ip_rt_put(rt);
> 		continue;
> 
> found:
>-		addr = bond_confirm_addr(rt->dst.dev, targets[i], 0);
>+		addr = bond_confirm_addr(rt->dst.dev, target_ip, 0);
> 		ip_rt_put(rt);
>-		bond_arp_send(slave, ARPOP_REQUEST, targets[i], addr, tags);
>+		bond_arp_send(slave, ARPOP_REQUEST, target_ip, addr, tags);
> 		kfree(tags);
> 	}
> }
>@@ -6102,7 +6105,7 @@ static int __init bond_check_params(struct bond_params *params)
> 	int arp_all_targets_value = 0;
> 	u16 ad_actor_sys_prio = 0;
> 	u16 ad_user_port_key = 0;
>-	__be32 arp_target[BOND_MAX_ARP_TARGETS] = { 0 };
>+	struct arp_target arp_target[BOND_MAX_ARP_TARGETS] = { 0 };
> 	int arp_ip_count;
> 	int bond_mode	= BOND_MODE_ROUNDROBIN;
> 	int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;
>@@ -6296,7 +6299,7 @@ static int __init bond_check_params(struct bond_params *params)
> 			arp_interval = 0;
> 		} else {
> 			if (bond_get_targets_ip(arp_target, ip) == -1)
>-				arp_target[arp_ip_count++] = ip;
>+				arp_target[arp_ip_count++].target_ip = ip;
> 			else
> 				pr_warn("Warning: duplicate address %pI4 in arp_ip_target, skipping\n",
> 					&ip);
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index ac5e402c34bc..1a3d17754c0a 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -688,8 +688,8 @@ static int bond_fill_info(struct sk_buff *skb,
> 
> 	targets_added = 0;
> 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
>-		if (bond->params.arp_targets[i]) {
>-			if (nla_put_be32(skb, i, bond->params.arp_targets[i]))
>+		if (bond->params.arp_targets[i].target_ip) {
>+			if (nla_put_be32(skb, i, bond->params.arp_targets[i].target_ip))
> 				goto nla_put_failure;
> 			targets_added = 1;
> 		}
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 91893c29b899..54940950079e 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1090,7 +1090,7 @@ static int bond_option_arp_interval_set(struct bonding *bond,
> 			netdev_dbg(bond->dev, "ARP monitoring cannot be used with MII monitoring. Disabling MII monitoring\n");
> 			bond->params.miimon = 0;
> 		}
>-		if (!bond->params.arp_targets[0])
>+		if (!bond->params.arp_targets[0].target_ip)
> 			netdev_dbg(bond->dev, "ARP monitoring has been set up, but no ARP targets have been specified\n");
> 	}
> 	if (bond->dev->flags & IFF_UP) {
>@@ -1118,20 +1118,20 @@ static void _bond_options_arp_ip_target_set(struct bonding *bond, int slot,
> 					    __be32 target,
> 					    unsigned long last_rx)
> {
>-	__be32 *targets = bond->params.arp_targets;
>+	struct arp_target *targets = bond->params.arp_targets;
> 	struct list_head *iter;
> 	struct slave *slave;
> 
> 	if (slot >= 0 && slot < BOND_MAX_ARP_TARGETS) {
> 		bond_for_each_slave(bond, slave, iter)
> 			slave->target_last_arp_rx[slot] = last_rx;
>-		targets[slot] = target;
>+		targets[slot].target_ip = target;
> 	}
> }
> 
> static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
> {
>-	__be32 *targets = bond->params.arp_targets;
>+	struct arp_target *targets = bond->params.arp_targets;
> 	int ind;
> 
> 	if (!bond_is_ip_target_ok(target)) {
>@@ -1166,7 +1166,7 @@ static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
> 
> static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
> {
>-	__be32 *targets = bond->params.arp_targets;
>+	struct arp_target *targets = bond->params.arp_targets;
> 	struct list_head *iter;
> 	struct slave *slave;
> 	unsigned long *targets_rx;
>@@ -1185,20 +1185,20 @@ static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
> 		return -EINVAL;
> 	}
> 
>-	if (ind == 0 && !targets[1] && bond->params.arp_interval)
>+	if (ind == 0 && !targets[1].target_ip && bond->params.arp_interval)
> 		netdev_warn(bond->dev, "Removing last arp target with arp_interval on\n");
> 
> 	netdev_dbg(bond->dev, "Removing ARP target %pI4\n", &target);
> 
> 	bond_for_each_slave(bond, slave, iter) {
> 		targets_rx = slave->target_last_arp_rx;
>-		for (i = ind; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
>+		for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++)
> 			targets_rx[i] = targets_rx[i+1];
> 		targets_rx[i] = 0;
> 	}
>-	for (i = ind; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
>+	for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++)
> 		targets[i] = targets[i+1];
>-	targets[i] = 0;
>+	targets[i].target_ip = 0;
> 
> 	return 0;
> }
>diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
>index 7edf72ec816a..94e6fd7041ee 100644
>--- a/drivers/net/bonding/bond_procfs.c
>+++ b/drivers/net/bonding/bond_procfs.c
>@@ -121,11 +121,11 @@ static void bond_info_show_master(struct seq_file *seq)
> 		seq_printf(seq, "ARP IP target/s (n.n.n.n form):");
> 
> 		for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>-			if (!bond->params.arp_targets[i])
>+			if (!bond->params.arp_targets[i].target_ip)
> 				break;
> 			if (printed)
> 				seq_printf(seq, ",");
>-			seq_printf(seq, " %pI4", &bond->params.arp_targets[i]);
>+			seq_printf(seq, " %pI4", &bond->params.arp_targets[i].target_ip);
> 			printed = 1;
> 		}
> 		seq_printf(seq, "\n");
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 1e13bb170515..d7c09e0a14dd 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -290,9 +290,9 @@ static ssize_t bonding_show_arp_targets(struct device *d,
> 	int i, res = 0;
> 
> 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
>-		if (bond->params.arp_targets[i])
>+		if (bond->params.arp_targets[i].target_ip)
> 			res += sysfs_emit_at(buf, res, "%pI4 ",
>-					     &bond->params.arp_targets[i]);
>+					     &bond->params.arp_targets[i].target_ip);
> 	}
> 	if (res)
> 		buf[res-1] = '\n'; /* eat the leftover space */
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 95f67b308c19..709ef9a302dd 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -115,6 +115,11 @@ static inline int is_netpoll_tx_blocked(struct net_device *dev)
> #define is_netpoll_tx_blocked(dev) (0)
> #endif
> 
>+struct arp_target {
>+	__be32 target_ip;
>+	struct bond_vlan_tag *tags;
>+};
>+
> struct bond_params {
> 	int mode;
> 	int xmit_policy;
>@@ -135,7 +140,7 @@ struct bond_params {
> 	int ad_select;
> 	char primary[IFNAMSIZ];
> 	int primary_reselect;
>-	__be32 arp_targets[BOND_MAX_ARP_TARGETS];
>+	struct arp_target arp_targets[BOND_MAX_ARP_TARGETS];
> 	int tx_queues;
> 	int all_slaves_active;
> 	int resend_igmp;
>@@ -522,7 +527,7 @@ static inline unsigned long slave_oldest_target_arp_rx(struct bonding *bond,
> 	int i = 1;
> 	unsigned long ret = slave->target_last_arp_rx[0];
> 
>-	for (; (i < BOND_MAX_ARP_TARGETS) && bond->params.arp_targets[i]; i++)
>+	for (; (i < BOND_MAX_ARP_TARGETS) && bond->params.arp_targets[i].target_ip; i++)
> 		if (time_before(slave->target_last_arp_rx[i], ret))
> 			ret = slave->target_last_arp_rx[i];
> 
>@@ -760,14 +765,14 @@ static inline bool bond_slave_has_mac_rcu(struct bonding *bond, const u8 *mac)
> /* Check if the ip is present in arp ip list, or first free slot if ip == 0
>  * Returns -1 if not found, index if found
>  */
>-static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
>+static inline int bond_get_targets_ip(struct arp_target *targets, __be32 ip)
> {
> 	int i;
> 
> 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
>-		if (targets[i] == ip)
>+		if (targets[i].target_ip == ip)
> 			return i;
>-		else if (targets[i] == 0)
>+		else if (targets[i].target_ip == 0)
> 			break;
> 
> 	return -1;
>-- 
>2.43.5
>

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

Powered by blists - more mailing lists