Re: [PATCHv2 net-next 2/3] bonding: support aggregator selection based on port priority

[Jay Vosburgh <jv@jvosburgh.net>]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Add a new ad_select policy 'port_priority' that uses the per-port
>actor priority values (set via ad_actor_port_prio) to determine
>aggregator selection.
>
>This allows administrators to influence which ports are preferred
>for aggregation by assigning different priority values, providing
>more flexible load balancing control in LACP configurations.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> Documentation/networking/bonding.rst |  9 ++++++++-
> drivers/net/bonding/bond_3ad.c       | 27 +++++++++++++++++++++++++++
> drivers/net/bonding/bond_options.c   |  1 +
> include/net/bond_3ad.h               |  1 +
> 4 files changed, 37 insertions(+), 1 deletion(-)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index 874d8a4681ec..151c964562db 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -250,7 +250,14 @@ ad_select
> 		ports (slaves).  Reselection occurs as described under the
> 		"bandwidth" setting, above.
> 
>-	The bandwidth and count selection policies permit failover of
>+	prio or 3
>+
>+		The active aggregator is chosen by the highest total sum of
>+		actor port priorities across its active ports. Note this
>+		priority is ad_actor_port_prio, not per port prio, which is
>+		used for primary reselect.
>+
>+	The bandwidth, count and prio selection policies permit failover of

	Needing to have a caveat here makes me think we should instead
change the nomenclature.  Perhaps "lacp_port_prio"?  The standard hasn't
had "ad" in its name for 20-ish years, so I don't think we should use
"ad" in user facing options, and common usage these days is to just call
it "lacp."

	Simiarly, I don't think we need "ad" in the option name, either;
the standard just calls it "actor_port_priority", is there a good reason
not to use that?

	-J

> 	802.3ad aggregations when partial failure of the active aggregator
> 	occurs.  This keeps the aggregator with the highest availability
> 	(either in bandwidth or in number of ports) active at all times.
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 19b389b81600..dcc1a1750df5 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -747,6 +747,18 @@ static int __agg_active_ports(struct aggregator *agg)
> 	return active;
> }
> 
>+static unsigned int __agg_ports_priority(const struct aggregator *agg)
>+{
>+	struct port *port = agg->lag_ports;
>+	unsigned int prio = 0;
>+
>+	for (; port; port = port->next_port_in_aggregator)
>+		if (port->is_enabled)
>+			prio += port->actor_port_priority;
>+
>+	return prio;
>+}
>+
> /**
>  * __get_agg_bandwidth - get the total bandwidth of an aggregator
>  * @aggregator: the aggregator we're looking at
>@@ -1707,6 +1719,9 @@ static struct aggregator *ad_agg_selection_test(struct aggregator *best,
> 	 * BOND_AD_COUNT: Select by count of ports.  If count is equal,
> 	 *     select by bandwidth.
> 	 *
>+	 * BOND_AD_PRIO: Select by total priority of ports. If priority
>+	 *     is equal, select by count.
>+	 *
> 	 * BOND_AD_STABLE, BOND_AD_BANDWIDTH: Select by bandwidth.
> 	 */
> 	if (!best)
>@@ -1725,6 +1740,14 @@ static struct aggregator *ad_agg_selection_test(struct aggregator *best,
> 		return best;
> 
> 	switch (__get_agg_selection_mode(curr->lag_ports)) {
>+	case BOND_AD_PRIO:
>+		if (__agg_ports_priority(curr) > __agg_ports_priority(best))
>+			return curr;
>+
>+		if (__agg_ports_priority(curr) < __agg_ports_priority(best))
>+			return best;
>+
>+		fallthrough;
> 	case BOND_AD_COUNT:
> 		if (__agg_active_ports(curr) > __agg_active_ports(best))
> 			return curr;
>@@ -1790,6 +1813,10 @@ static int agg_device_up(const struct aggregator *agg)
>  * (slaves), and reselect whenever a link state change takes place or the
>  * set of slaves in the bond changes.
>  *
>+ * BOND_AD_PRIO: select the aggregator with highest total priority of ports
>+ * (slaves), and reselect whenever a link state change takes place or the
>+ * set of slaves in the bond changes.
>+ *
>  * FIXME: this function MUST be called with the first agg in the bond, or
>  * __get_active_agg() won't work correctly. This function should be better
>  * called with the bond itself, and retrieve the first agg from it.
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 5b58326dd24c..4bfff38b9ad0 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -165,6 +165,7 @@ static const struct bond_opt_value bond_ad_select_tbl[] = {
> 	{ "stable",    BOND_AD_STABLE,    BOND_VALFLAG_DEFAULT},
> 	{ "bandwidth", BOND_AD_BANDWIDTH, 0},
> 	{ "count",     BOND_AD_COUNT,     0},
>+	{ "prio",      BOND_AD_PRIO,      0},
> 	{ NULL,        -1,                0},
> };
> 
>diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
>index bf551ca70359..34495df965f0 100644
>--- a/include/net/bond_3ad.h
>+++ b/include/net/bond_3ad.h
>@@ -26,6 +26,7 @@ enum {
> 	BOND_AD_STABLE = 0,
> 	BOND_AD_BANDWIDTH = 1,
> 	BOND_AD_COUNT = 2,
>+	BOND_AD_PRIO = 3,
> };
> 
> /* rx machine states(43.4.11 in the 802.3ad standard) */
>-- 
>2.50.1
>

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