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

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

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