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

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

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