[PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Hangbin Liu]

Add per port priority support for bonding active slave re-selection during
failover. A higher number means higher priority in selection. The primary
slave still has the highest priority. This option also follows the
primary_reselect rules.

This option could only be configured via netlink.

v2: using the extant bonding options management stuff instead setting
slave prio in bond_slave_changelink() directly.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 Documentation/networking/bonding.rst | 11 +++++
 drivers/net/bonding/bond_main.c      | 27 +++++++++++
 drivers/net/bonding/bond_netlink.c   | 19 ++++++++
 drivers/net/bonding/bond_options.c   | 67 ++++++++++++++++++++++++++++
 include/net/bond_options.h           |  1 +
 include/net/bonding.h                |  1 +
 include/uapi/linux/if_link.h         |  1 +
 tools/include/uapi/linux/if_link.h   |  1 +
 8 files changed, 128 insertions(+)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index 43be3782e5df..53a18ff7cf23 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -780,6 +780,17 @@ peer_notif_delay
 	value is 0 which means to match the value of the link monitor
 	interval.
 
+prio
+	Slave priority. A higher number means higher priority.
+	The primary slave has the highest priority. This option also
+	follows the primary_reselect rules.
+
+	This option could only be configured via netlink, and is only valid
+	for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
+	The valid value range is a signed 32 bit integer.
+
+	The default value is 0.
+
 primary
 
 	A string (eth0, eth2, etc) specifying which slave is the
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3d427183ec8e..3415a0feea07 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1026,12 +1026,38 @@ static void bond_do_fail_over_mac(struct bonding *bond,
 
 }
 
+/**
+ * bond_choose_primary_or_current - select the primary or high priority slave
+ * @bond: our bonding struct
+ *
+ * - Check if there is a primary link. If the primary link was set and is up,
+ *   go on and do link reselection.
+ *
+ * - If primary link is not set or down, find the highest priority link.
+ *   If the highest priority link is not current slave, set it as primary
+ *   link and do link reselection.
+ */
 static struct slave *bond_choose_primary_or_current(struct bonding *bond)
 {
 	struct slave *prim = rtnl_dereference(bond->primary_slave);
 	struct slave *curr = rtnl_dereference(bond->curr_active_slave);
+	struct slave *slave, *hprio = NULL;
+	struct list_head *iter;
 
 	if (!prim || prim->link != BOND_LINK_UP) {
+		bond_for_each_slave(bond, slave, iter) {
+			if (slave->link == BOND_LINK_UP) {
+				hprio = hprio ?: slave;
+				if (slave->prio > hprio->prio)
+					hprio = slave;
+			}
+		}
+
+		if (hprio && hprio != curr) {
+			prim = hprio;
+			goto link_reselect;
+		}
+
 		if (!curr || curr->link != BOND_LINK_UP)
 			return NULL;
 		return curr;
@@ -1042,6 +1068,7 @@ static struct slave *bond_choose_primary_or_current(struct bonding *bond)
 		return prim;
 	}
 
+link_reselect:
 	if (!curr || curr->link != BOND_LINK_UP)
 		return prim;
 
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 5a6f44455b95..41b3244747fa 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -27,6 +27,7 @@ static size_t bond_get_slave_size(const struct net_device *bond_dev,
 		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_AGGREGATOR_ID */
 		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE */
 		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE */
+		nla_total_size(sizeof(s32)) +	/* IFLA_BOND_SLAVE_PRIO */
 		0;
 }
 
@@ -53,6 +54,9 @@ static int bond_fill_slave_info(struct sk_buff *skb,
 	if (nla_put_u16(skb, IFLA_BOND_SLAVE_QUEUE_ID, slave->queue_id))
 		goto nla_put_failure;
 
+	if (nla_put_s32(skb, IFLA_BOND_SLAVE_PRIO, slave->prio))
+		goto nla_put_failure;
+
 	if (BOND_MODE(slave->bond) == BOND_MODE_8023AD) {
 		const struct aggregator *agg;
 		const struct port *ad_port;
@@ -117,6 +121,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
 
 static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
 	[IFLA_BOND_SLAVE_QUEUE_ID]	= { .type = NLA_U16 },
+	[IFLA_BOND_SLAVE_PRIO]		= { .type = NLA_S32 },
 };
 
 static int bond_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
 			return err;
 	}
 
+	if (data[IFLA_BOND_SLAVE_PRIO]) {
+		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
+		char prio_str[IFNAMSIZ + 7];
+
+		/* prio option setting expects slave_name:prio */
+		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
+			 slave_dev->name, prio);
+
+		bond_opt_initstr(&newval, prio_str);
+		err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 96eef19cffc4..bac3858825b3 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -40,6 +40,8 @@ static int bond_option_arp_validate_set(struct bonding *bond,
 					const struct bond_opt_value *newval);
 static int bond_option_arp_all_targets_set(struct bonding *bond,
 					   const struct bond_opt_value *newval);
+static int bond_option_prio_set(struct bonding *bond,
+				const struct bond_opt_value *newval);
 static int bond_option_primary_set(struct bonding *bond,
 				   const struct bond_opt_value *newval);
 static int bond_option_primary_reselect_set(struct bonding *bond,
@@ -365,6 +367,16 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
 		.values = bond_intmax_tbl,
 		.set = bond_option_miimon_set
 	},
+	[BOND_OPT_PRIO] = {
+		.id = BOND_OPT_PRIO,
+		.name = "prio",
+		.desc = "Link priority for failover re-selection",
+		.flags = BOND_OPTFLAG_RAWVAL,
+		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ACTIVEBACKUP) |
+						BIT(BOND_MODE_TLB) |
+						BIT(BOND_MODE_ALB)),
+		.set = bond_option_prio_set
+	},
 	[BOND_OPT_PRIMARY] = {
 		.id = BOND_OPT_PRIMARY,
 		.name = "primary",
@@ -1306,6 +1318,61 @@ static int bond_option_missed_max_set(struct bonding *bond,
 	return 0;
 }
 
+static int bond_option_prio_set(struct bonding *bond,
+				const struct bond_opt_value *newval)
+{
+	struct slave *slave, *update_slave;
+	struct net_device *sdev;
+	struct list_head *iter;
+	char *delim;
+	int ret = 0;
+	int prio;
+
+	/* delim will point to prio if successful */
+	delim = strchr(newval->string, ':');
+	if (!delim)
+		goto err_no_cmd;
+
+	/* Terminate string that points to device name and bump it
+	 * up one, so we can read the prio there.
+	 */
+	*delim = '\0';
+	if (sscanf(++delim, "%d\n", &prio) != 1)
+		goto err_no_cmd;
+
+	/* Valid ifname */
+	if (!dev_valid_name(newval->string))
+		goto err_no_cmd;
+
+	/* Get the pointer to that interface if it exists */
+	sdev = __dev_get_by_name(dev_net(bond->dev), newval->string);
+	if (!sdev)
+		goto err_no_cmd;
+
+	update_slave = NULL;
+	bond_for_each_slave(bond, slave, iter) {
+		if (sdev == slave->dev)
+			update_slave = slave;
+	}
+
+	if (!update_slave)
+		goto err_no_cmd;
+
+	/* Actually set the prio for the slave */
+	update_slave->prio = prio;
+
+	/* Do reselect after prio update */
+	bond_select_active_slave(bond);
+
+out:
+	return ret;
+
+err_no_cmd:
+	netdev_dbg(bond->dev, "invalid input for slave prio set\n");
+	ret = -EPERM;
+	goto out;
+}
+
 static int bond_option_primary_set(struct bonding *bond,
 				   const struct bond_opt_value *newval)
 {
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index 1618b76f4903..569808933094 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -67,6 +67,7 @@ enum {
 	BOND_OPT_LACP_ACTIVE,
 	BOND_OPT_MISSED_MAX,
 	BOND_OPT_NS_TARGETS,
+	BOND_OPT_PRIO,
 	BOND_OPT_LAST
 };
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index cb904d356e31..6e78d657aa05 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -178,6 +178,7 @@ struct slave {
 	u32    speed;
 	u16    queue_id;
 	u8     perm_hwaddr[MAX_ADDR_LEN];
+	int    prio;
 	struct ad_slave_info *ad_info;
 	struct tlb_slave_info tlb_info;
 #ifdef CONFIG_NET_POLL_CONTROLLER
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5f58dcfe2787..e36d9d2c65a7 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -963,6 +963,7 @@ enum {
 	IFLA_BOND_SLAVE_AD_AGGREGATOR_ID,
 	IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE,
 	IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
+	IFLA_BOND_SLAVE_PRIO,
 	__IFLA_BOND_SLAVE_MAX,
 };
 
diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index b339bf2196ca..0242f31e339c 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -890,6 +890,7 @@ enum {
 	IFLA_BOND_SLAVE_AD_AGGREGATOR_ID,
 	IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE,
 	IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
+	IFLA_BOND_SLAVE_PRIO,
 	__IFLA_BOND_SLAVE_MAX,
 };
 
-- 
2.35.1

[PATCHv2 iproute2-next] iplink: bond_slave: add per port prio support

[Hangbin Liu]

Add per port priority support for active slave re-selection during
bonding failover. A higher number means higher priority.

This option is only valid for active-backup(1), balance-tlb (5) and
balance-alb (6) mode.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: update man page
---
 ip/iplink_bond_slave.c | 12 +++++++++++-
 man/man8/ip-link.8.in  |  8 ++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
index d488aaab..8103704b 100644
--- a/ip/iplink_bond_slave.c
+++ b/ip/iplink_bond_slave.c
@@ -19,7 +19,7 @@
 
 static void print_explain(FILE *f)
 {
-	fprintf(f, "Usage: ... bond_slave [ queue_id ID ]\n");
+	fprintf(f, "Usage: ... bond_slave [ queue_id ID ] [ prio PRIORITY ]\n");
 }
 
 static void explain(void)
@@ -120,6 +120,10 @@ static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *t
 			  "queue_id %d ",
 			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_QUEUE_ID]));
 
+	if (tb[IFLA_BOND_SLAVE_PRIO])
+		print_int(PRINT_ANY, "prio", "prio %d ",
+			  rta_getattr_s32(tb[IFLA_BOND_SLAVE_PRIO]));
+
 	if (tb[IFLA_BOND_SLAVE_AD_AGGREGATOR_ID])
 		print_int(PRINT_ANY,
 			  "ad_aggregator_id",
@@ -151,6 +155,7 @@ static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
 				struct nlmsghdr *n)
 {
 	__u16 queue_id;
+	int prio;
 
 	while (argc > 0) {
 		if (matches(*argv, "queue_id") == 0) {
@@ -158,6 +163,11 @@ static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
 			if (get_u16(&queue_id, *argv, 0))
 				invarg("queue_id is invalid", *argv);
 			addattr16(n, 1024, IFLA_BOND_SLAVE_QUEUE_ID, queue_id);
+		} else if (strcmp(*argv, "prio") == 0) {
+			NEXT_ARG();
+			if (get_s32(&prio, *argv, 0))
+				invarg("prio is invalid", *argv);
+			addattr32(n, 1024, IFLA_BOND_SLAVE_PRIO, prio);
 		} else {
 			if (matches(*argv, "help") != 0)
 				fprintf(stderr,
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 6f332645..3dbcdbb6 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -2567,6 +2567,8 @@ the following additional arguments are supported:
 .B "ip link set type bond_slave"
 [
 .BI queue_id " ID"
+] [
+.BI prio " PRIORITY"
 ]
 
 .in +8
@@ -2574,6 +2576,12 @@ the following additional arguments are supported:
 .BI queue_id " ID"
 - set the slave's queue ID (a 16bit unsigned value).
 
+.sp
+.BI prio " PRIORITY"
+- set the slave's priority for active slave re-selection during failover
+(a 32bit signed value). This option only valid for active-backup(1),
+balance-tlb (5) and balance-alb (6) mode.
+
 .in -8
 
 .TP
-- 
2.35.1

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Stephen Hemminger]

On Wed, 15 Jun 2022 11:29:34 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 5a6f44455b95..41b3244747fa 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -27,6 +27,7 @@ static size_t bond_get_slave_size(const struct net_device *bond_dev,
>  		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_AGGREGATOR_ID */
>  		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE */
>  		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE */
> +		nla_total_size(sizeof(s32)) +	/* IFLA_BOND_SLAVE_PRIO */
>  		0;
>  }

Why the choice to make it signed? It would be clearer as unsigned value.
Also, using full 32 bits seems like overkill.

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Hangbin Liu]

On Tue, Jun 14, 2022 at 08:52:58PM -0700, Stephen Hemminger wrote:
> On Wed, 15 Jun 2022 11:29:34 +0800
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> > diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> > index 5a6f44455b95..41b3244747fa 100644
> > --- a/drivers/net/bonding/bond_netlink.c
> > +++ b/drivers/net/bonding/bond_netlink.c
> > @@ -27,6 +27,7 @@ static size_t bond_get_slave_size(const struct net_device *bond_dev,
> >  		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_AGGREGATOR_ID */
> >  		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE */
> >  		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE */
> > +		nla_total_size(sizeof(s32)) +	/* IFLA_BOND_SLAVE_PRIO */
> >  		0;
> >  }
> 
> Why the choice to make it signed? It would be clearer as unsigned value.

Let's say you have a bond with 10 slaves. You want to make 1 slave with
lowest priority. With singed value, you can just set it to -10 while other
slaves keep using the default value 0.

> Also, using full 32 bits seems like overkill.

Yes, it seems too much for just priority. This is to compatible with team
prio option[1].

[1] https://github.com/jpirko/libteam/blob/master/man/teamd.conf.5#L138

Thanks
Hangbin

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Jonathan Toppins]

On 6/14/22 23:29, Hangbin Liu wrote:
> Add per port priority support for bonding active slave re-selection during
> failover. A higher number means higher priority in selection. The primary
> slave still has the highest priority. This option also follows the
> primary_reselect rules.
> 
> This option could only be configured via netlink.
> 
> v2: using the extant bonding options management stuff instead setting
> slave prio in bond_slave_changelink() directly.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   Documentation/networking/bonding.rst | 11 +++++
>   drivers/net/bonding/bond_main.c      | 27 +++++++++++
>   drivers/net/bonding/bond_netlink.c   | 19 ++++++++
>   drivers/net/bonding/bond_options.c   | 67 ++++++++++++++++++++++++++++
>   include/net/bond_options.h           |  1 +
>   include/net/bonding.h                |  1 +
>   include/uapi/linux/if_link.h         |  1 +
>   tools/include/uapi/linux/if_link.h   |  1 +
>   8 files changed, 128 insertions(+)
> 
> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index 43be3782e5df..53a18ff7cf23 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -780,6 +780,17 @@ peer_notif_delay
>   	value is 0 which means to match the value of the link monitor
>   	interval.
>   
> +prio
> +	Slave priority. A higher number means higher priority.
> +	The primary slave has the highest priority. This option also
> +	follows the primary_reselect rules.
> +
> +	This option could only be configured via netlink, and is only valid
> +	for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
> +	The valid value range is a signed 32 bit integer.
> +
> +	The default value is 0.

Why is this a signed 32 bit number? Why not a u8, it would seem 256 
[255,0] options is more than enough to select from. Is there a specific 
reason it needs to be an s32?

If the reason for selecting a signed value is so that the default 
priority could be in the middle of the range, why not just set the 
default to be 128, assuming u8 is wide enough?

> +
>   primary
>   
>   	A string (eth0, eth2, etc) specifying which slave is the
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 3d427183ec8e..3415a0feea07 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1026,12 +1026,38 @@ static void bond_do_fail_over_mac(struct bonding *bond,
>   
>   }
>   
> +/**
> + * bond_choose_primary_or_current - select the primary or high priority slave
> + * @bond: our bonding struct
> + *
> + * - Check if there is a primary link. If the primary link was set and is up,
> + *   go on and do link reselection.
> + *
> + * - If primary link is not set or down, find the highest priority link.
> + *   If the highest priority link is not current slave, set it as primary
> + *   link and do link reselection.
> + */
>   static struct slave *bond_choose_primary_or_current(struct bonding *bond)
>   {
>   	struct slave *prim = rtnl_dereference(bond->primary_slave);
>   	struct slave *curr = rtnl_dereference(bond->curr_active_slave);
> +	struct slave *slave, *hprio = NULL;
> +	struct list_head *iter;
>   
>   	if (!prim || prim->link != BOND_LINK_UP) {
> +		bond_for_each_slave(bond, slave, iter) {
> +			if (slave->link == BOND_LINK_UP) {
> +				hprio = hprio ?: slave;
> +				if (slave->prio > hprio->prio)
> +					hprio = slave;
> +			}
> +		}
> +
> +		if (hprio && hprio != curr) {
> +			prim = hprio;
> +			goto link_reselect;
> +		}
> +
>   		if (!curr || curr->link != BOND_LINK_UP)
>   			return NULL;
>   		return curr;
> @@ -1042,6 +1068,7 @@ static struct slave *bond_choose_primary_or_current(struct bonding *bond)
>   		return prim;
>   	}
>   
> +link_reselect:
>   	if (!curr || curr->link != BOND_LINK_UP)
>   		return prim;
>   
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 5a6f44455b95..41b3244747fa 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -27,6 +27,7 @@ static size_t bond_get_slave_size(const struct net_device *bond_dev,
>   		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_AGGREGATOR_ID */
>   		nla_total_size(sizeof(u8)) +	/* IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE */
>   		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE */
> +		nla_total_size(sizeof(s32)) +	/* IFLA_BOND_SLAVE_PRIO */
>   		0;
>   }
>   
> @@ -53,6 +54,9 @@ static int bond_fill_slave_info(struct sk_buff *skb,
>   	if (nla_put_u16(skb, IFLA_BOND_SLAVE_QUEUE_ID, slave->queue_id))
>   		goto nla_put_failure;
>   
> +	if (nla_put_s32(skb, IFLA_BOND_SLAVE_PRIO, slave->prio))
> +		goto nla_put_failure;
> +
>   	if (BOND_MODE(slave->bond) == BOND_MODE_8023AD) {
>   		const struct aggregator *agg;
>   		const struct port *ad_port;
> @@ -117,6 +121,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
>   
>   static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
>   	[IFLA_BOND_SLAVE_QUEUE_ID]	= { .type = NLA_U16 },
> +	[IFLA_BOND_SLAVE_PRIO]		= { .type = NLA_S32 },
>   };
>   
>   static int bond_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
>   			return err;
>   	}
>   
> +	if (data[IFLA_BOND_SLAVE_PRIO]) { > +		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> +		char prio_str[IFNAMSIZ + 7];
> +
> +		/* prio option setting expects slave_name:prio */
> +		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
> +			 slave_dev->name, prio);
> +
> +		bond_opt_initstr(&newval, prio_str);

It might be less code and a little cleaner to extend struct 
bond_opt_value with a slave pointer.

	struct bond_opt_value {
		char *string;
		u64 value;
		u32 flags;
		union {
			char cextra[BOND_OPT_EXTRA_MAXLEN];
			struct net_device *slave_dev;
		} extra;
	};

Then modify __bond_opt_init to set the slave pointer, basically a set of 
bond_opt_slave_init{} macros. This would remove the need to parse the 
slave interface name in the set function. Setting .flags = 
BOND_OPTFLAG_RAWVAL (already done I see) in the option definition to 
avoid bond_opt_parse() from loosing our extra information by pointing to 
a .values table entry. Now in the option specific set function we can 
just find the slave entry and set the value, no more string parsing code 
needed.

> +		err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval);

I think this patch series needs to be rebased onto latest 
net-next/master as a patch series I sent added two extra parameter list 
arguments to __bond_opt_set().

   2bff369b2354 bonding: netlink error message support for options

Considering my comments above about extending bond_opt_value, I might 
look as sending a fixup patch to remove all the parameter list additions 
and hide the netlink extack pointer in bond_opt_value.

> +		if (err)
> +			return err;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 96eef19cffc4..bac3858825b3 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -40,6 +40,8 @@ static int bond_option_arp_validate_set(struct bonding *bond,
>   					const struct bond_opt_value *newval);
>   static int bond_option_arp_all_targets_set(struct bonding *bond,
>   					   const struct bond_opt_value *newval);
> +static int bond_option_prio_set(struct bonding *bond,
> +				const struct bond_opt_value *newval);
>   static int bond_option_primary_set(struct bonding *bond,
>   				   const struct bond_opt_value *newval);
>   static int bond_option_primary_reselect_set(struct bonding *bond,
> @@ -365,6 +367,16 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
>   		.values = bond_intmax_tbl,
>   		.set = bond_option_miimon_set
>   	},
> +	[BOND_OPT_PRIO] = {
> +		.id = BOND_OPT_PRIO,
> +		.name = "prio",
> +		.desc = "Link priority for failover re-selection",
> +		.flags = BOND_OPTFLAG_RAWVAL,
> +		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ACTIVEBACKUP) |
> +						BIT(BOND_MODE_TLB) |
> +						BIT(BOND_MODE_ALB)),
> +		.set = bond_option_prio_set
> +	},
>   	[BOND_OPT_PRIMARY] = {
>   		.id = BOND_OPT_PRIMARY,
>   		.name = "primary",
> @@ -1306,6 +1318,61 @@ static int bond_option_missed_max_set(struct bonding *bond,
>   	return 0;
>   }
>   
> +static int bond_option_prio_set(struct bonding *bond,
> +				const struct bond_opt_value *newval)
> +{
> +	struct slave *slave, *update_slave;
> +	struct net_device *sdev;
> +	struct list_head *iter;
> +	char *delim;
> +	int ret = 0;
> +	int prio;

Priorities are only considered if there is no primary set, correct? 
Would you not want to issue a netdev_warn here noting the fact that this 
will be ignored if the bond device has a primary set? Much like how 
other options issue warnings, or in miimon's case turn off arp monitor, 
when other configuration influence the effectiveness of the setting?

> +
> +	/* delim will point to prio if successful */
> +	delim = strchr(newval->string, ':');
> +	if (!delim)
> +		goto err_no_cmd;
> +
> +	/* Terminate string that points to device name and bump it
> +	 * up one, so we can read the prio there.
> +	 */
> +	*delim = '\0';
> +	if (sscanf(++delim, "%d\n", &prio) != 1)
> +		goto err_no_cmd;
> +
> +	/* Valid ifname */
> +	if (!dev_valid_name(newval->string))
> +		goto err_no_cmd;
> +
> +	/* Get the pointer to that interface if it exists */
> +	sdev = __dev_get_by_name(dev_net(bond->dev), newval->string);
> +	if (!sdev)
> +		goto err_no_cmd;
> +
> +	update_slave = NULL;
> +	bond_for_each_slave(bond, slave, iter) {
> +		if (sdev == slave->dev)
> +			update_slave = slave;
> +	}
> +
> +	if (!update_slave)
> +		goto err_no_cmd;
> +
> +	/* Actually set the prio for the slave */
> +	update_slave->prio = prio;
> +
> +	/* Do reselect after prio update */
> +	bond_select_active_slave(bond);
> +
> +out:
> +	return ret;
> +
> +err_no_cmd:
> +	netdev_dbg(bond->dev, "invalid input for slave prio set\n");
> +	ret = -EPERM;
> +	goto out;
> +}
> +
>   static int bond_option_primary_set(struct bonding *bond,
>   				   const struct bond_opt_value *newval)
>   {
> diff --git a/include/net/bond_options.h b/include/net/bond_options.h
> index 1618b76f4903..569808933094 100644
> --- a/include/net/bond_options.h
> +++ b/include/net/bond_options.h
> @@ -67,6 +67,7 @@ enum {
>   	BOND_OPT_LACP_ACTIVE,
>   	BOND_OPT_MISSED_MAX,
>   	BOND_OPT_NS_TARGETS,
> +	BOND_OPT_PRIO,
>   	BOND_OPT_LAST
>   };
>   
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index cb904d356e31..6e78d657aa05 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -178,6 +178,7 @@ struct slave {
>   	u32    speed;
>   	u16    queue_id;
>   	u8     perm_hwaddr[MAX_ADDR_LEN];
> +	int    prio;
>   	struct ad_slave_info *ad_info;
>   	struct tlb_slave_info tlb_info;
>   #ifdef CONFIG_NET_POLL_CONTROLLER
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 5f58dcfe2787..e36d9d2c65a7 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -963,6 +963,7 @@ enum {
>   	IFLA_BOND_SLAVE_AD_AGGREGATOR_ID,
>   	IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE,
>   	IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
> +	IFLA_BOND_SLAVE_PRIO,
>   	__IFLA_BOND_SLAVE_MAX,
>   };
>   
> diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
> index b339bf2196ca..0242f31e339c 100644
> --- a/tools/include/uapi/linux/if_link.h
> +++ b/tools/include/uapi/linux/if_link.h
> @@ -890,6 +890,7 @@ enum {
>   	IFLA_BOND_SLAVE_AD_AGGREGATOR_ID,
>   	IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE,
>   	IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
> +	IFLA_BOND_SLAVE_PRIO,
>   	__IFLA_BOND_SLAVE_MAX,
>   };
>   

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Hangbin Liu]

On Wed, Jun 15, 2022 at 01:44:57PM -0400, Jonathan Toppins wrote:
> > diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> > index 43be3782e5df..53a18ff7cf23 100644
> > --- a/Documentation/networking/bonding.rst
> > +++ b/Documentation/networking/bonding.rst
> > @@ -780,6 +780,17 @@ peer_notif_delay
> >   	value is 0 which means to match the value of the link monitor
> >   	interval.
> > +prio
> > +	Slave priority. A higher number means higher priority.
> > +	The primary slave has the highest priority. This option also
> > +	follows the primary_reselect rules.
> > +
> > +	This option could only be configured via netlink, and is only valid
> > +	for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
> > +	The valid value range is a signed 32 bit integer.
> > +
> > +	The default value is 0.
> 
> Why is this a signed 32 bit number? Why not a u8, it would seem 256 [255,0]
> options is more than enough to select from. Is there a specific reason it
> needs to be an s32?

The main reason is to compatible with team prio option, which also use a s32
value.

If you think s32 is too wide, how about s16? As u8 looks a little tight.
And follow are the reasons I prefer using singed value.

> 
> If the reason for selecting a signed value is so that the default priority
> could be in the middle of the range, why not just set the default to be 128,
> assuming u8 is wide enough?

First, 128 looks like a weird default value to me as a user.
0/1 as a default looks more reasonable.

Second. If I'm a user, other than using like 111, 125, 128, 130,
I'd prefer to use a multiple of 10 as priority number. e.g. -10, 20, 100, -200.

I know someone prefer a positive value as priority number. But given the
convenience of using negative value for a less wanted slave. I personally
prefer a singed value for priority setting.

Hi, Jay, what do you think?

> > @@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> >   			return err;
> >   	}
> > +	if (data[IFLA_BOND_SLAVE_PRIO]) { > +		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> > +		char prio_str[IFNAMSIZ + 7];
> > +
> > +		/* prio option setting expects slave_name:prio */
> > +		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
> > +			 slave_dev->name, prio);
> > +
> > +		bond_opt_initstr(&newval, prio_str);
> 
> It might be less code and a little cleaner to extend struct bond_opt_value
> with a slave pointer.
> 
> 	struct bond_opt_value {
> 		char *string;
> 		u64 value;
> 		u32 flags;
> 		union {
> 			char cextra[BOND_OPT_EXTRA_MAXLEN];
> 			struct net_device *slave_dev;
> 		} extra;
> 	};
> 
> Then modify __bond_opt_init to set the slave pointer, basically a set of
> bond_opt_slave_init{} macros. This would remove the need to parse the slave
> interface name in the set function. Setting .flags = BOND_OPTFLAG_RAWVAL
> (already done I see) in the option definition to avoid bond_opt_parse() from
> loosing our extra information by pointing to a .values table entry. Now in
> the option specific set function we can just find the slave entry and set
> the value, no more string parsing code needed.

This looks reasonable to me. It would make all slave options setting easier
for future usage.

> 
> > +		err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval);
> 
> I think this patch series needs to be rebased onto latest net-next/master as
> a patch series I sent added two extra parameter list arguments to
> __bond_opt_set().

OK, I will.

> 
>   2bff369b2354 bonding: netlink error message support for options
> 
> Considering my comments above about extending bond_opt_value, I might look
> as sending a fixup patch to remove all the parameter list additions and hide
> the netlink extack pointer in bond_opt_value.
> 
> > +		if (err)
> > +			return err;
> > +	}
> > +
> >   	return 0;
> >   }
> > @@ -1306,6 +1318,61 @@ static int bond_option_missed_max_set(struct bonding *bond,
> >   	return 0;
> >   }
> > +static int bond_option_prio_set(struct bonding *bond,
> > +				const struct bond_opt_value *newval)
> > +{
> > +	struct slave *slave, *update_slave;
> > +	struct net_device *sdev;
> > +	struct list_head *iter;
> > +	char *delim;
> > +	int ret = 0;
> > +	int prio;
> 
> Priorities are only considered if there is no primary set, correct? Would
> you not want to issue a netdev_warn here noting the fact that this will be
> ignored if the bond device has a primary set? Much like how other options
> issue warnings, or in miimon's case turn off arp monitor, when other
> configuration influence the effectiveness of the setting?

OK, I will.

Thanks
Hangbin

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[kernel test robot]

Hi Hangbin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Hangbin-Liu/Bonding-add-per-port-priority-for-failover-re-selection/20220615-113309
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 6ac6dc746d70ef9b4ac835d98ac1baf63a810c57
config: i386-randconfig-a005 (https://download.01.org/0day-ci/archive/20220616/202206162221.Oqz0N9WH-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/2ab299e7237ababa2ce05baa6035154fa04b272d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hangbin-Liu/Bonding-add-per-port-priority-for-failover-re-selection/20220615-113309
        git checkout 2ab299e7237ababa2ce05baa6035154fa04b272d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/bonding/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/bonding/bond_netlink.c: In function 'bond_slave_changelink':
>> drivers/net/bonding/bond_netlink.c:174:23: error: too few arguments to function '__bond_opt_set'
     174 |                 err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval);
         |                       ^~~~~~~~~~~~~~
   In file included from include/net/bonding.h:31,
                    from drivers/net/bonding/bond_netlink.c:16:
   include/net/bond_options.h:110:5: note: declared here
     110 | int __bond_opt_set(struct bonding *bond, unsigned int option,
         |     ^~~~~~~~~~~~~~


vim +/__bond_opt_set +174 drivers/net/bonding/bond_netlink.c

   138	
   139	static int bond_slave_changelink(struct net_device *bond_dev,
   140					 struct net_device *slave_dev,
   141					 struct nlattr *tb[], struct nlattr *data[],
   142					 struct netlink_ext_ack *extack)
   143	{
   144		struct bonding *bond = netdev_priv(bond_dev);
   145		struct bond_opt_value newval;
   146		int err;
   147	
   148		if (!data)
   149			return 0;
   150	
   151		if (data[IFLA_BOND_SLAVE_QUEUE_ID]) {
   152			u16 queue_id = nla_get_u16(data[IFLA_BOND_SLAVE_QUEUE_ID]);
   153			char queue_id_str[IFNAMSIZ + 7];
   154	
   155			/* queue_id option setting expects slave_name:queue_id */
   156			snprintf(queue_id_str, sizeof(queue_id_str), "%s:%u\n",
   157				 slave_dev->name, queue_id);
   158			bond_opt_initstr(&newval, queue_id_str);
   159			err = __bond_opt_set(bond, BOND_OPT_QUEUE_ID, &newval,
   160					     data[IFLA_BOND_SLAVE_QUEUE_ID], extack);
   161			if (err)
   162				return err;
   163		}
   164	
   165		if (data[IFLA_BOND_SLAVE_PRIO]) {
   166			int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
   167			char prio_str[IFNAMSIZ + 7];
   168	
   169			/* prio option setting expects slave_name:prio */
   170			snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
   171				 slave_dev->name, prio);
   172	
   173			bond_opt_initstr(&newval, prio_str);
 > 174			err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval);
   175			if (err)
   176				return err;
   177		}
   178	
   179		return 0;
   180	}
   181	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Hangbin Liu]

On Thu, Jun 16, 2022 at 10:58:12AM +0800, Hangbin Liu wrote:
> > > @@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> > >   			return err;
> > >   	}
> > > +	if (data[IFLA_BOND_SLAVE_PRIO]) { > +		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> > > +		char prio_str[IFNAMSIZ + 7];
> > > +
> > > +		/* prio option setting expects slave_name:prio */
> > > +		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
> > > +			 slave_dev->name, prio);
> > > +
> > > +		bond_opt_initstr(&newval, prio_str);
> > 
> > It might be less code and a little cleaner to extend struct bond_opt_value
> > with a slave pointer.
> > 
> > 	struct bond_opt_value {
> > 		char *string;
> > 		u64 value;
> > 		u32 flags;
> > 		union {
> > 			char cextra[BOND_OPT_EXTRA_MAXLEN];
> > 			struct net_device *slave_dev;
> > 		} extra;
> > 	};
> > 
> > Then modify __bond_opt_init to set the slave pointer, basically a set of
> > bond_opt_slave_init{} macros. This would remove the need to parse the slave
> > interface name in the set function. Setting .flags = BOND_OPTFLAG_RAWVAL
> > (already done I see) in the option definition to avoid bond_opt_parse() from
> > loosing our extra information by pointing to a .values table entry. Now in
> > the option specific set function we can just find the slave entry and set
> > the value, no more string parsing code needed.
> 
> This looks reasonable to me. It would make all slave options setting easier
> for future usage.

Hi Jan, Jay,

I have updated the slave option setting like the following. I didn't add
a extra name for the union, so we don't need to edit the existing code. I think
the slave_dev should be safe as it's protected by rtnl lock. But I'm
not sure if I missed anything. Do you think if it's OK to store/get slave_dev
pointer like this?

diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index 1618b76f4903..f65be547a73d 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -83,7 +83,10 @@ struct bond_opt_value {
 	char *string;
 	u64 value;
 	u32 flags;
-	char extra[BOND_OPT_EXTRA_MAXLEN];
+	union {
+		char extra[BOND_OPT_EXTRA_MAXLEN];
+		struct net_device *slave_dev;
+	};
 };
 
 struct bonding;
@@ -133,13 +136,16 @@ static inline void __bond_opt_init(struct bond_opt_value *optval,
 		optval->value = value;
 	else if (string)
 		optval->string = string;
-	else if (extra_len <= BOND_OPT_EXTRA_MAXLEN)
+
+	if (extra && extra_len <= BOND_OPT_EXTRA_MAXLEN)
 		memcpy(optval->extra, extra, extra_len);
 }
 #define bond_opt_initval(optval, value) __bond_opt_init(optval, NULL, value, NULL, 0)
 #define bond_opt_initstr(optval, str) __bond_opt_init(optval, str, ULLONG_MAX, NULL, 0)
 #define bond_opt_initextra(optval, extra, extra_len) \
 	__bond_opt_init(optval, NULL, ULLONG_MAX, extra, extra_len)
+#define bond_opt_initslave(optval, value, slave_dev) \
+	__bond_opt_init(optval, NULL, value, slave_dev, sizeof(struct net_device *))
 
 void bond_option_arp_ip_targets_clear(struct bonding *bond);
 #if IS_ENABLED(CONFIG_IPV6)


diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 5a6f44455b95..f0d3f36739ea 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -157,6 +162,16 @@ static int bond_slave_changelink(struct net_device *bond_dev,
                        return err;
        }

+       if (data[IFLA_BOND_SLAVE_PRIO]) {
+               int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
+
+               bond_opt_initslave(&newval, prio, &slave_dev);
+               err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval,
+                                    data[IFLA_BOND_SLAVE_PRIO], extack);
+               if (err)
+                       return err;
+       }
+
        return 0;
 }

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 96eef19cffc4..473cedb0cb0b 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
+static int bond_option_prio_set(struct bonding *bond,
+                               const struct bond_opt_value *newval)
+{
+       struct slave *slave;
+
+       slave = bond_slave_get_rtnl(newval->slave_dev);
+       if (!slave) {
+               netdev_dbg(newval->slave_dev, "%s called on NULL slave\n", __func__);
+               return -ENODEV;
+       }
+       slave->prio = newval->value;
+
+       if (rtnl_dereference(bond->primary_slave))
+               slave_warn(bond->dev, slave->dev,
+                          "prio updated, but will not affect failover re-selection as primary slave have been set\n",
+                          slave->prio);
+       else
+               bond_select_active_slave(bond);
+
+       return 0;
+}

Thanks
Hangbin

Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

[Jonathan Toppins]

On 6/17/22 04:04, Hangbin Liu wrote:
> On Thu, Jun 16, 2022 at 10:58:12AM +0800, Hangbin Liu wrote:
>>>> @@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
>>>>    			return err;
>>>>    	}
>>>> +	if (data[IFLA_BOND_SLAVE_PRIO]) { > +		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
>>>> +		char prio_str[IFNAMSIZ + 7];
>>>> +
>>>> +		/* prio option setting expects slave_name:prio */
>>>> +		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
>>>> +			 slave_dev->name, prio);
>>>> +
>>>> +		bond_opt_initstr(&newval, prio_str);
>>>
>>> It might be less code and a little cleaner to extend struct bond_opt_value
>>> with a slave pointer.
>>>
>>> 	struct bond_opt_value {
>>> 		char *string;
>>> 		u64 value;
>>> 		u32 flags;
>>> 		union {
>>> 			char cextra[BOND_OPT_EXTRA_MAXLEN];
>>> 			struct net_device *slave_dev;
>>> 		} extra;
>>> 	};
>>>
>>> Then modify __bond_opt_init to set the slave pointer, basically a set of
>>> bond_opt_slave_init{} macros. This would remove the need to parse the slave
>>> interface name in the set function. Setting .flags = BOND_OPTFLAG_RAWVAL
>>> (already done I see) in the option definition to avoid bond_opt_parse() from
>>> loosing our extra information by pointing to a .values table entry. Now in
>>> the option specific set function we can just find the slave entry and set
>>> the value, no more string parsing code needed.
>>
>> This looks reasonable to me. It would make all slave options setting easier
>> for future usage.
> 
> Hi Jan, Jay,
> 
> I have updated the slave option setting like the following. I didn't add
> a extra name for the union, so we don't need to edit the existing code. I think
> the slave_dev should be safe as it's protected by rtnl lock. But I'm
> not sure if I missed anything. Do you think if it's OK to store/get slave_dev
> pointer like this?
> 
> diff --git a/include/net/bond_options.h b/include/net/bond_options.h
> index 1618b76f4903..f65be547a73d 100644
> --- a/include/net/bond_options.h
> +++ b/include/net/bond_options.h
> @@ -83,7 +83,10 @@ struct bond_opt_value {
>   	char *string;
>   	u64 value;
>   	u32 flags;
> -	char extra[BOND_OPT_EXTRA_MAXLEN];
> +	union {
> +		char extra[BOND_OPT_EXTRA_MAXLEN];
> +		struct net_device *slave_dev;
> +	};
>   };
>   
>   struct bonding;
> @@ -133,13 +136,16 @@ static inline void __bond_opt_init(struct bond_opt_value *optval,
>   		optval->value = value;
>   	else if (string)
>   		optval->string = string;
> -	else if (extra_len <= BOND_OPT_EXTRA_MAXLEN)
> +
> +	if (extra && extra_len <= BOND_OPT_EXTRA_MAXLEN)
>   		memcpy(optval->extra, extra, extra_len); >   }
>   #define bond_opt_initval(optval, value) __bond_opt_init(optval, NULL, value, NULL, 0)
>   #define bond_opt_initstr(optval, str) __bond_opt_init(optval, str, ULLONG_MAX, NULL, 0)
>   #define bond_opt_initextra(optval, extra, extra_len) \
>   	__bond_opt_init(optval, NULL, ULLONG_MAX, extra, extra_len)
> +#define bond_opt_initslave(optval, value, slave_dev) \
> +	__bond_opt_init(optval, NULL, value, slave_dev, sizeof(struct net_device *))

To keep the naming consistent with the other helpers I would have chosen:

#define bond_opt_slave_initval(optval, slave_dev, value) \

>   
>   void bond_option_arp_ip_targets_clear(struct bonding *bond);
>   #if IS_ENABLED(CONFIG_IPV6)
> 
> 

[...]

The rest looks good to me.

-Jon