[PATCH net-next] Bonding: add per port priority support

[PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

Add per port priority support for bonding. A higher number means higher
priority. The primary slave still has the highest priority. This option
also follows the primary_reselect rules.

This option could only be configured via netlink.

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

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index 525e6842dd33..103e292a04a1 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -780,6 +780,15 @@ 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 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 15eddca7b4b6..4211b79ac619 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 ? 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 f427fa1737c7..63066559e7d6 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[],
@@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
 				 struct nlattr *tb[], struct nlattr *data[],
 				 struct netlink_ext_ack *extack)
 {
+	struct slave *slave = bond_slave_get_rtnl(slave_dev);
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct bond_opt_value newval;
 	int err;
@@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
 			return err;
 	}
 
+	/* No need to bother __bond_opt_set as we only support netlink config */
+	if (data[IFLA_BOND_SLAVE_PRIO]) {
+		slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
+		bond_select_active_slave(bond);
+	}
+
 	return 0;
 }
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index b14f4c0b4e9e..4ff093fb2289 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -176,6 +176,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 cc284c048e69..204e342b107a 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -956,6 +956,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 e1ba2d51b717..ee5de9f3700b 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -888,6 +888,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

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

[Hangbin Liu]

Add per port priority support for bonding. 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>
---
 ip/iplink_bond_slave.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
index d488aaab..7a4d89ff 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,12 @@ 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 +157,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 +165,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,
-- 
2.35.1

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

[David Ahern]

On 4/11/22 10:17 PM, Hangbin Liu wrote:
> diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
> index d488aaab..7a4d89ff 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,12 @@ 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 ",

those last 2 lines can go on the first.

Also, seems like a man page should be updated as well.

Re: [PATCH net-next] Bonding: add per port priority support

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Add per port priority support for bonding. A higher number means higher
>priority. The primary slave still has the highest priority. This option
>also follows the primary_reselect rules.

	The above description (and the Subject) should mention that this
apparently refers to priority in interface selection during failover
events.

>This option could only be configured via netlink.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> Documentation/networking/bonding.rst |  9 +++++++++
> drivers/net/bonding/bond_main.c      | 27 +++++++++++++++++++++++++++
> drivers/net/bonding/bond_netlink.c   | 12 ++++++++++++
> include/net/bonding.h                |  1 +
> include/uapi/linux/if_link.h         |  1 +
> tools/include/uapi/linux/if_link.h   |  1 +
> 6 files changed, 51 insertions(+)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index 525e6842dd33..103e292a04a1 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -780,6 +780,15 @@ 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 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 15eddca7b4b6..4211b79ac619 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 ? 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 f427fa1737c7..63066559e7d6 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 },

	Why used signed instead of unsigned?

	Regardless, the valid range for the prio value should be
documented.

	-J

> };
> 
> static int bond_validate(struct nlattr *tb[], struct nlattr *data[],
>@@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> 				 struct nlattr *tb[], struct nlattr *data[],
> 				 struct netlink_ext_ack *extack)
> {
>+	struct slave *slave = bond_slave_get_rtnl(slave_dev);
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct bond_opt_value newval;
> 	int err;
>@@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> 			return err;
> 	}
> 
>+	/* No need to bother __bond_opt_set as we only support netlink config */
>+	if (data[IFLA_BOND_SLAVE_PRIO]) {
>+		slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
>+		bond_select_active_slave(bond);
>+	}
>+
> 	return 0;
> }
> 
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index b14f4c0b4e9e..4ff093fb2289 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -176,6 +176,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 cc284c048e69..204e342b107a 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -956,6 +956,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 e1ba2d51b717..ee5de9f3700b 100644
>--- a/tools/include/uapi/linux/if_link.h
>+++ b/tools/include/uapi/linux/if_link.h
>@@ -888,6 +888,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
>

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

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Mon, Apr 11, 2022 at 09:55:45PM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >Add per port priority support for bonding. A higher number means higher
> >priority. The primary slave still has the highest priority. This option
> >also follows the primary_reselect rules.
> 
> 	The above description (and the Subject) should mention that this
> apparently refers to priority in interface selection during failover
> events.

OK, will update it. How about:

Bonding: add per port priority for current slave re-selection during failover

Add per port priority support for bonding current 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.

> >@@ -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 },
> 
> 	Why used signed instead of unsigned?
> 
> 	Regardless, the valid range for the prio value should be
> documented.

I did this in purpose as team also use singed number. User could use a
negative number for a specific link while other links keep using default 0.

BTW, how to document the valid ranger for a int number, -2^31 ~ 2^31-1 or
INT_MIN ~ INT_MAX.

Thanks
Hangbin

Re: [PATCH net-next] Bonding: add per port priority support

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Mon, Apr 11, 2022 at 09:55:45PM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@gmail.com> wrote:
>> 
>> >Add per port priority support for bonding. A higher number means higher
>> >priority. The primary slave still has the highest priority. This option
>> >also follows the primary_reselect rules.
>> 
>> 	The above description (and the Subject) should mention that this
>> apparently refers to priority in interface selection during failover
>> events.
>
>OK, will update it. How about:
>
>Bonding: add per port priority for current slave re-selection during failover

	That would be better, but something like "add per-port priority
for failover re-selection" would be a bit shorter.

>Add per port priority support for bonding current 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 seems reasonable.

>> >@@ -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 },
>> 
>> 	Why used signed instead of unsigned?
>> 
>> 	Regardless, the valid range for the prio value should be
>> documented.
>
>I did this in purpose as team also use singed number. User could use a
>negative number for a specific link while other links keep using default 0.

	Fair enough; I had been comparing to the LACP port priority and
route metric, both of which are unsigned.

>BTW, how to document the valid ranger for a int number, -2^31 ~ 2^31-1 or
>INT_MIN ~ INT_MAX.

	The documentation can simply state that the value is a signed 32
bit integer (rather than giving the specifics).

	-J

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

Re: [PATCH net-next] Bonding: add per port priority support

[Jonathan Toppins]

On 4/12/22 00:13, Hangbin Liu wrote:
> Add per port priority support for bonding. A higher number means higher
> priority. The primary slave still has the highest priority. This option
> also follows the primary_reselect rules.
> 
> This option could only be configured via netlink.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   Documentation/networking/bonding.rst |  9 +++++++++
>   drivers/net/bonding/bond_main.c      | 27 +++++++++++++++++++++++++++
>   drivers/net/bonding/bond_netlink.c   | 12 ++++++++++++
>   include/net/bonding.h                |  1 +
>   include/uapi/linux/if_link.h         |  1 +
>   tools/include/uapi/linux/if_link.h   |  1 +
>   6 files changed, 51 insertions(+)
> 
> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index 525e6842dd33..103e292a04a1 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -780,6 +780,15 @@ 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
                     ^^ can
> +	for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
> +	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 15eddca7b4b6..4211b79ac619 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 ? 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 f427fa1737c7..63066559e7d6 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[],
> @@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
>   				 struct nlattr *tb[], struct nlattr *data[],
>   				 struct netlink_ext_ack *extack)
>   {
> +	struct slave *slave = bond_slave_get_rtnl(slave_dev);
>   	struct bonding *bond = netdev_priv(bond_dev);
>   	struct bond_opt_value newval;
>   	int err;
> @@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
>   			return err;
>   	}
>   
> +	/* No need to bother __bond_opt_set as we only support netlink config */

Not sure this comment is necessary, it doesn't add any value. Also I 
would recommend using bonding's options management, as it would allow 
for checking if the value is in a defined range. That might not be 
particularly useful in this context since it appears +/-INT_MAX is the 
range.

Also, in the Documentation it is mentioned that this parameter is only 
used in modes active-backup and balance-alb/tlb. Do we need to send an 
error message back preventing the modification of this value when not in 
these modes?

> +	if (data[IFLA_BOND_SLAVE_PRIO]) {
> +		slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> +		bond_select_active_slave(bond);
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index b14f4c0b4e9e..4ff093fb2289 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -176,6 +176,7 @@ struct slave {
>   	u32    speed;
>   	u16    queue_id;
>   	u8     perm_hwaddr[MAX_ADDR_LEN];
> +	int    prio;

Do we want a struct slave_params here instead? That would allow us to 
define defaults in a central place and set them once instead of setting 
each parameter.

>   	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 cc284c048e69..204e342b107a 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -956,6 +956,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 e1ba2d51b717..ee5de9f3700b 100644
> --- a/tools/include/uapi/linux/if_link.h
> +++ b/tools/include/uapi/linux/if_link.h
> @@ -888,6 +888,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: [PATCH net-next] Bonding: add per port priority support

[Jay Vosburgh]

Jonathan Toppins <jtoppins@redhat.com> wrote:

>On 4/12/22 00:13, Hangbin Liu wrote:
>> Add per port priority support for bonding. A higher number means higher
>> priority. The primary slave still has the highest priority. This option
>> also follows the primary_reselect rules.
>> This option could only be configured via netlink.
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> ---
>>   Documentation/networking/bonding.rst |  9 +++++++++
>>   drivers/net/bonding/bond_main.c      | 27 +++++++++++++++++++++++++++
>>   drivers/net/bonding/bond_netlink.c   | 12 ++++++++++++
>>   include/net/bonding.h                |  1 +
>>   include/uapi/linux/if_link.h         |  1 +
>>   tools/include/uapi/linux/if_link.h   |  1 +
>>   6 files changed, 51 insertions(+)
>> diff --git a/Documentation/networking/bonding.rst
>> b/Documentation/networking/bonding.rst
>> index 525e6842dd33..103e292a04a1 100644
>> --- a/Documentation/networking/bonding.rst
>> +++ b/Documentation/networking/bonding.rst
>> @@ -780,6 +780,15 @@ 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
>                    ^^ can
>> +	for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
>> +	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 15eddca7b4b6..4211b79ac619 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 ? 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 f427fa1737c7..63066559e7d6 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[],
>> @@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
>>   				 struct nlattr *tb[], struct nlattr *data[],
>>   				 struct netlink_ext_ack *extack)
>>   {
>> +	struct slave *slave = bond_slave_get_rtnl(slave_dev);
>>   	struct bonding *bond = netdev_priv(bond_dev);
>>   	struct bond_opt_value newval;
>>   	int err;
>> @@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
>>   			return err;
>>   	}
>>   +	/* No need to bother __bond_opt_set as we only support netlink
>> config */
>
>Not sure this comment is necessary, it doesn't add any value. Also I would
>recommend using bonding's options management, as it would allow for
>checking if the value is in a defined range. That might not be
>particularly useful in this context since it appears +/-INT_MAX is the
>range.

	Agreed, on both the comment and in regards to using the extant
bonding options management stuff.

>Also, in the Documentation it is mentioned that this parameter is only
>used in modes active-backup and balance-alb/tlb. Do we need to send an
>error message back preventing the modification of this value when not in
>these modes?

	Using the option management stuff would get this for free.

	-J

>> +	if (data[IFLA_BOND_SLAVE_PRIO]) {
>> +		slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
>> +		bond_select_active_slave(bond);
>> +	}
>> +
>>   	return 0;
>>   }
>>   diff --git a/include/net/bonding.h b/include/net/bonding.h
>> index b14f4c0b4e9e..4ff093fb2289 100644
>> --- a/include/net/bonding.h
>> +++ b/include/net/bonding.h
>> @@ -176,6 +176,7 @@ struct slave {
>>   	u32    speed;
>>   	u16    queue_id;
>>   	u8     perm_hwaddr[MAX_ADDR_LEN];
>> +	int    prio;
>
>Do we want a struct slave_params here instead? That would allow us to
>define defaults in a central place and set them once instead of setting
>each parameter.

	Presuming that you mean creating a sub-struct here and moving
some set of members of struct slave into it, I'm not sure I see the
benefit, as it would only exist here and not really be an independent
object.  Am I misunderstanding?

	-J

>>   	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 cc284c048e69..204e342b107a 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -956,6 +956,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 e1ba2d51b717..ee5de9f3700b 100644
>> --- a/tools/include/uapi/linux/if_link.h
>> +++ b/tools/include/uapi/linux/if_link.h
>> @@ -888,6 +888,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,
>>   };
>>   

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

Re: [PATCH net-next] Bonding: add per port priority support

[Jonathan Toppins]

On 4/12/22 11:55, Jay Vosburgh wrote:
>>>    diff --git a/include/net/bonding.h b/include/net/bonding.h
>>> index b14f4c0b4e9e..4ff093fb2289 100644
>>> --- a/include/net/bonding.h
>>> +++ b/include/net/bonding.h
>>> @@ -176,6 +176,7 @@ struct slave {
>>>    	u32    speed;
>>>    	u16    queue_id;
>>>    	u8     perm_hwaddr[MAX_ADDR_LEN];
>>> +	int    prio;
>> Do we want a struct slave_params here instead? That would allow us to
>> define defaults in a central place and set them once instead of setting
>> each parameter.
> 	Presuming that you mean creating a sub-struct here and moving
> some set of members of struct slave into it, I'm not sure I see the
> benefit, as it would only exist here and not really be an independent
> object.  Am I misunderstanding?

You are understanding correctly. The goal of this work is to eventually 
port the majority of the per-port parameters that exist in teaming to 
bonding, we have not determined the entire set that make sense. Thus 
there will be more than just port priority as a userspace configurable 
option. So I was attempting to ask if modeling the initial setting of 
these parameters like how `bonding_defaults` is used, made sense.

file: drivers/net/bonding/bond_main.c:
	void bond_setup(struct net_device *bond_dev)
	{
		struct bonding *bond = netdev_priv(bond_dev);

		spin_lock_init(&bond->mode_lock);
		bond->params = bonding_defaults;
	...


We can always refactor this area when there is another option that needs 
setting.

-Jon

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Tue, Apr 12, 2022 at 01:04:46PM -0400, Jonathan Toppins wrote:
> > 	Presuming that you mean creating a sub-struct here and moving
> > some set of members of struct slave into it, I'm not sure I see the
> > benefit, as it would only exist here and not really be an independent
> > object.  Am I misunderstanding?
> 
> You are understanding correctly. The goal of this work is to eventually port
> the majority of the per-port parameters that exist in teaming to bonding, we
> have not determined the entire set that make sense. Thus there will be more

Hi Jay,

As Jon said, I'm working to implement/import teaming specific features to
bonding, so users could have more choice. One import feature teaming has is
per-port parameters/configurations. A part of the per-port configs are
queue_id, prio, lacp_prio, lacp_key, etc. Most of the configs are link_watch
parameters. Which means each port/slave has it's own delay up, delay down,
interval, arp targets, etc. We are still discussing if bonding need all of
them or just a part.

Do you see if it's valuable to add all the per-port link watch configurations
to bonding?

Thanks
Hangbin
> than just port priority as a userspace configurable option. So I was
> attempting to ask if modeling the initial setting of these parameters like
> how `bonding_defaults` is used, made sense.
> 
> file: drivers/net/bonding/bond_main.c:
> 	void bond_setup(struct net_device *bond_dev)
> 	{
> 		struct bonding *bond = netdev_priv(bond_dev);
> 
> 		spin_lock_init(&bond->mode_lock);
> 		bond->params = bonding_defaults;
> 	...
> 
> 
> We can always refactor this area when there is another option that needs
> setting.
> 
> -Jon
> 

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Tue, Apr 12, 2022 at 08:55:41AM -0700, Jay Vosburgh wrote:
> >> @@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> >>   				 struct nlattr *tb[], struct nlattr *data[],
> >>   				 struct netlink_ext_ack *extack)
> >>   {
> >> +	struct slave *slave = bond_slave_get_rtnl(slave_dev);
> >>   	struct bonding *bond = netdev_priv(bond_dev);
> >>   	struct bond_opt_value newval;
> >>   	int err;
> >> @@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> >>   			return err;
> >>   	}
> >>   +	/* No need to bother __bond_opt_set as we only support netlink
> >> config */
> >
> >Not sure this comment is necessary, it doesn't add any value. Also I would
> >recommend using bonding's options management, as it would allow for
> >checking if the value is in a defined range. That might not be
> >particularly useful in this context since it appears +/-INT_MAX is the
> >range.
> 
> 	Agreed, on both the comment and in regards to using the extant
> bonding options management stuff.
> 
> >Also, in the Documentation it is mentioned that this parameter is only
> >used in modes active-backup and balance-alb/tlb. Do we need to send an
> >error message back preventing the modification of this value when not in
> >these modes?
> 
> 	Using the option management stuff would get this for free.

Hi Jav, Jon,

I remembered the reason why I didn't use bond default option management.

It's because the bonding options management only take bond and values. We
need to create an extra string to save the slave name and option values.
Then in bond option setting function we extract the info from the string
and do setting again, like the bond_option_queue_id_set().

I think this is too heavy for just an int value setting for slave.
As we only support netlink for new options. There is no need to handle
string setting via sysfs. For mode checking, we do just do like:

if (!bond_uses_primary(bond))
	return -EACCES;

So why bother the bonding options management? What do you think?
Do you have a easier way to get the slave name in options management?
If yes, I'm happy to use the default option management.

Thanks
Hangbin
> 
> 	-J
> 
> >> +	if (data[IFLA_BOND_SLAVE_PRIO]) {
> >> +		slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> >> +		bond_select_active_slave(bond);
> >> +	}
> >> +
> >>   	return 0;

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Mon, Apr 18, 2022 at 06:20:52PM +0800, Hangbin Liu wrote:
> > 	Agreed, on both the comment and in regards to using the extant
> > bonding options management stuff.
> > 
> > >Also, in the Documentation it is mentioned that this parameter is only
> > >used in modes active-backup and balance-alb/tlb. Do we need to send an
> > >error message back preventing the modification of this value when not in
> > >these modes?
> > 
> > 	Using the option management stuff would get this for free.
> 
> Hi Jav, Jon,
> 
> I remembered the reason why I didn't use bond default option management.
> 
> It's because the bonding options management only take bond and values. We
> need to create an extra string to save the slave name and option values.
> Then in bond option setting function we extract the info from the string
> and do setting again, like the bond_option_queue_id_set().
> 
> I think this is too heavy for just an int value setting for slave.
> As we only support netlink for new options. There is no need to handle
> string setting via sysfs. For mode checking, we do just do like:
> 
> if (!bond_uses_primary(bond))
> 	return -EACCES;
> 
> So why bother the bonding options management? What do you think?
> Do you have a easier way to get the slave name in options management?
> If yes, I'm happy to use the default option management.

Hi Jan,

Any comments?

Thanks
Hangbin

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Fri, Apr 22, 2022 at 06:24:05PM +0800, Hangbin Liu wrote:
> On Mon, Apr 18, 2022 at 06:20:52PM +0800, Hangbin Liu wrote:
> > > 	Agreed, on both the comment and in regards to using the extant
> > > bonding options management stuff.
> > > 
> > > >Also, in the Documentation it is mentioned that this parameter is only
> > > >used in modes active-backup and balance-alb/tlb. Do we need to send an
> > > >error message back preventing the modification of this value when not in
> > > >these modes?
> > > 
> > > 	Using the option management stuff would get this for free.
> > 
> > Hi Jav, Jon,
> > 
> > I remembered the reason why I didn't use bond default option management.
> > 
> > It's because the bonding options management only take bond and values. We
> > need to create an extra string to save the slave name and option values.
> > Then in bond option setting function we extract the info from the string
> > and do setting again, like the bond_option_queue_id_set().
> > 
> > I think this is too heavy for just an int value setting for slave.
> > As we only support netlink for new options. There is no need to handle
> > string setting via sysfs. For mode checking, we do just do like:
> > 
> > if (!bond_uses_primary(bond))
> > 	return -EACCES;
> > 
> > So why bother the bonding options management? What do you think?
> > Do you have a easier way to get the slave name in options management?
> > If yes, I'm happy to use the default option management.
> 
> Hi Jay,
> 
> Any comments?
> 

Hi Jay,

I'm still waiting for your comments before post v2 patch. Appreciate if you
could have a better way about handling the slave name in options management.

Thanks
Hangbin

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Fri, May 06, 2022 at 04:12:09PM +0800, Hangbin Liu wrote:
> Hi Jay,
> 
> I'm still waiting for your comments before post v2 patch. Appreciate if you
> could have a better way about handling the slave name in options management.

Hi Jay,

Would you please help reply when have time?

Thanks
Hangbin

Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu]

On Fri, May 06, 2022 at 04:12:02PM +0800, Hangbin Liu wrote:
> On Fri, Apr 22, 2022 at 06:24:05PM +0800, Hangbin Liu wrote:
> > On Mon, Apr 18, 2022 at 06:20:52PM +0800, Hangbin Liu wrote:
> > > > 	Agreed, on both the comment and in regards to using the extant
> > > > bonding options management stuff.
> > > > 
> > > > >Also, in the Documentation it is mentioned that this parameter is only
> > > > >used in modes active-backup and balance-alb/tlb. Do we need to send an
> > > > >error message back preventing the modification of this value when not in
> > > > >these modes?
> > > > 
> > > > 	Using the option management stuff would get this for free.
> > > 
> > > Hi Jav, Jon,
> > > 
> > > I remembered the reason why I didn't use bond default option management.
> > > 
> > > It's because the bonding options management only take bond and values. We
> > > need to create an extra string to save the slave name and option values.
> > > Then in bond option setting function we extract the info from the string
> > > and do setting again, like the bond_option_queue_id_set().
> > > 
> > > I think this is too heavy for just an int value setting for slave.
> > > As we only support netlink for new options. There is no need to handle
> > > string setting via sysfs. For mode checking, we do just do like:
> > > 
> > > if (!bond_uses_primary(bond))
> > > 	return -EACCES;
> > > 
> > > So why bother the bonding options management? What do you think?
> > > Do you have a easier way to get the slave name in options management?
> > > If yes, I'm happy to use the default option management.
> > 
> > Hi Jay,
> > 
> > Any comments?
> > 
> 
> Hi Jay,
> 
> I'm still waiting for your comments before post v2 patch. Appreciate if you
> could have a better way about handling the slave name in options management.

Hi Jay, ping?