[PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[Hangbin Liu]

Add support for the actor_port_prio option for bond slaves.
This per-port priority can be used by the bonding driver in ad_select to
choose the higher-priority aggregator during failover.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v4: no update
v3: rename ad_actor_port_prio to actor_port_prio
v2: no update
---
 ip/iplink_bond.c       |  1 +
 ip/iplink_bond_slave.c | 18 ++++++++++++++++--
 man/man8/ip-link.8.in  |  6 ++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
index d6960f6d9b03..1a2c1b3042a0 100644
--- a/ip/iplink_bond.c
+++ b/ip/iplink_bond.c
@@ -91,6 +91,7 @@ static const char *ad_select_tbl[] = {
 	"stable",
 	"bandwidth",
 	"count",
+	"prio",
 	NULL,
 };
 
diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
index ad6875006950..c88100e248dd 100644
--- a/ip/iplink_bond_slave.c
+++ b/ip/iplink_bond_slave.c
@@ -15,7 +15,9 @@
 
 static void print_explain(FILE *f)
 {
-	fprintf(f, "Usage: ... bond_slave [ queue_id ID ] [ prio PRIORITY ]\n");
+	fprintf(f, "Usage: ... bond_slave [ queue_id ID ] [ prio PRIORITY ]\n"
+		   "                      [ actor_port_prio PRIORITY ]\n"
+	);
 }
 
 static void explain(void)
@@ -145,12 +147,18 @@ static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *t
 			  state);
 		print_slave_oper_state(f, "ad_partner_oper_port_state_str", state);
 	}
+
+	if (tb[IFLA_BOND_SLAVE_ACTOR_PORT_PRIO])
+		print_int(PRINT_ANY,
+			  "actor_port_prio",
+			  "actor_port_prio %d ",
+			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_ACTOR_PORT_PRIO]));
 }
 
 static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
 				struct nlmsghdr *n)
 {
-	__u16 queue_id;
+	__u16 queue_id, actor_port_prio;
 	int prio;
 
 	while (argc > 0) {
@@ -164,6 +172,12 @@ static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
 			if (get_s32(&prio, *argv, 0))
 				invarg("prio is invalid", *argv);
 			addattr32(n, 1024, IFLA_BOND_SLAVE_PRIO, prio);
+		} else if (strcmp(*argv, "actor_port_prio") == 0) {
+			NEXT_ARG();
+			if (get_u16(&actor_port_prio, *argv, 0))
+				invarg("actor prio is invalid", *argv);
+			addattr16(n, 1024, IFLA_BOND_SLAVE_ACTOR_PORT_PRIO,
+				  actor_port_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 e3297c577152..7995943ab11f 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -2846,6 +2846,12 @@ the following additional arguments are supported:
 (a 32bit signed value). This option only valid for active-backup(1),
 balance-tlb (5) and balance-alb (6) mode.
 
+.sp
+.BI actor_port_prio " PRIORITY"
+- set the slave's ad actor port priority for 802.3ad aggregation selection
+logic during failover (a 16bit unsigned value). This option only valid for
+802.3ad (4) mode.
+
 .in -8
 
 .TP
-- 
2.50.1

Re: [PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Add support for the actor_port_prio option for bond slaves.
>This per-port priority can be used by the bonding driver in ad_select to
>choose the higher-priority aggregator during failover.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
>v4: no update
>v3: rename ad_actor_port_prio to actor_port_prio
>v2: no update
>---
> ip/iplink_bond.c       |  1 +
> ip/iplink_bond_slave.c | 18 ++++++++++++++++--
> man/man8/ip-link.8.in  |  6 ++++++
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
>diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
>index d6960f6d9b03..1a2c1b3042a0 100644
>--- a/ip/iplink_bond.c
>+++ b/ip/iplink_bond.c
>@@ -91,6 +91,7 @@ static const char *ad_select_tbl[] = {
> 	"stable",
> 	"bandwidth",
> 	"count",
>+	"prio",

	Should this be actor_port_prio?

	-J

> 	NULL,
> };
> 
>diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
>index ad6875006950..c88100e248dd 100644
>--- a/ip/iplink_bond_slave.c
>+++ b/ip/iplink_bond_slave.c
>@@ -15,7 +15,9 @@
> 
> static void print_explain(FILE *f)
> {
>-	fprintf(f, "Usage: ... bond_slave [ queue_id ID ] [ prio PRIORITY ]\n");
>+	fprintf(f, "Usage: ... bond_slave [ queue_id ID ] [ prio PRIORITY ]\n"
>+		   "                      [ actor_port_prio PRIORITY ]\n"
>+	);
> }
> 
> static void explain(void)
>@@ -145,12 +147,18 @@ static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *t
> 			  state);
> 		print_slave_oper_state(f, "ad_partner_oper_port_state_str", state);
> 	}
>+
>+	if (tb[IFLA_BOND_SLAVE_ACTOR_PORT_PRIO])
>+		print_int(PRINT_ANY,
>+			  "actor_port_prio",
>+			  "actor_port_prio %d ",
>+			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_ACTOR_PORT_PRIO]));
> }
> 
> static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
> 				struct nlmsghdr *n)
> {
>-	__u16 queue_id;
>+	__u16 queue_id, actor_port_prio;
> 	int prio;
> 
> 	while (argc > 0) {
>@@ -164,6 +172,12 @@ static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
> 			if (get_s32(&prio, *argv, 0))
> 				invarg("prio is invalid", *argv);
> 			addattr32(n, 1024, IFLA_BOND_SLAVE_PRIO, prio);
>+		} else if (strcmp(*argv, "actor_port_prio") == 0) {
>+			NEXT_ARG();
>+			if (get_u16(&actor_port_prio, *argv, 0))
>+				invarg("actor prio is invalid", *argv);
>+			addattr16(n, 1024, IFLA_BOND_SLAVE_ACTOR_PORT_PRIO,
>+				  actor_port_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 e3297c577152..7995943ab11f 100644
>--- a/man/man8/ip-link.8.in
>+++ b/man/man8/ip-link.8.in
>@@ -2846,6 +2846,12 @@ the following additional arguments are supported:
> (a 32bit signed value). This option only valid for active-backup(1),
> balance-tlb (5) and balance-alb (6) mode.
> 
>+.sp
>+.BI actor_port_prio " PRIORITY"
>+- set the slave's ad actor port priority for 802.3ad aggregation selection
>+logic during failover (a 16bit unsigned value). This option only valid for
>+802.3ad (4) mode.
>+
> .in -8
> 
> .TP
>-- 
>2.50.1
>

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

Re: [PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[Hangbin Liu]

On Wed, Aug 27, 2025 at 11:43:19AM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >Add support for the actor_port_prio option for bond slaves.
> >This per-port priority can be used by the bonding driver in ad_select to
> >choose the higher-priority aggregator during failover.
> >
> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> >---
> >v4: no update
> >v3: rename ad_actor_port_prio to actor_port_prio
> >v2: no update
> >---
> > ip/iplink_bond.c       |  1 +
> > ip/iplink_bond_slave.c | 18 ++++++++++++++++--
> > man/man8/ip-link.8.in  |  6 ++++++
> > 3 files changed, 23 insertions(+), 2 deletions(-)
> >
> >diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
> >index d6960f6d9b03..1a2c1b3042a0 100644
> >--- a/ip/iplink_bond.c
> >+++ b/ip/iplink_bond.c
> >@@ -91,6 +91,7 @@ static const char *ad_select_tbl[] = {
> > 	"stable",
> > 	"bandwidth",
> > 	"count",
> >+	"prio",
> 
> 	Should this be actor_port_prio?

hmm, actor_port_prio correspond to the ip link option name, which is also
acceptable.

While in kernel, we defined the select policy as

        { "stable",    BOND_AD_STABLE,    BOND_VALFLAG_DEFAULT},
        { "bandwidth", BOND_AD_BANDWIDTH, 0},
        { "count",     BOND_AD_COUNT,     0},
+       { "prio",      BOND_AD_PRIO,      0},

So I think the prio here should also be OK.

You can decide which one to use.

Thanks
Hangbin

Re: [PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Wed, Aug 27, 2025 at 11:43:19AM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@gmail.com> wrote:
>> 
>> >Add support for the actor_port_prio option for bond slaves.
>> >This per-port priority can be used by the bonding driver in ad_select to
>> >choose the higher-priority aggregator during failover.
>> >
>> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> >---
>> >v4: no update
>> >v3: rename ad_actor_port_prio to actor_port_prio
>> >v2: no update
>> >---
>> > ip/iplink_bond.c       |  1 +
>> > ip/iplink_bond_slave.c | 18 ++++++++++++++++--
>> > man/man8/ip-link.8.in  |  6 ++++++
>> > 3 files changed, 23 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
>> >index d6960f6d9b03..1a2c1b3042a0 100644
>> >--- a/ip/iplink_bond.c
>> >+++ b/ip/iplink_bond.c
>> >@@ -91,6 +91,7 @@ static const char *ad_select_tbl[] = {
>> > 	"stable",
>> > 	"bandwidth",
>> > 	"count",
>> >+	"prio",
>> 
>> 	Should this be actor_port_prio?
>
>hmm, actor_port_prio correspond to the ip link option name, which is also
>acceptable.

	Isn't this the text of the ip link option name right here (in
the sense of what goes on the "ip link" command line)?

>While in kernel, we defined the select policy as
>
>        { "stable",    BOND_AD_STABLE,    BOND_VALFLAG_DEFAULT},
>        { "bandwidth", BOND_AD_BANDWIDTH, 0},
>        { "count",     BOND_AD_COUNT,     0},
>+       { "prio",      BOND_AD_PRIO,      0},

	Maybe my memory is starting to go, but I thought in a prior
discussion we'd agreed to change this as well for consistency.

>So I think the prio here should also be OK.
>
>You can decide which one to use.

	I would prefer that the two options have discrete names, or,
really, that we not repeat "prio" as it's already used elsewhere.  Plus,
who knows, maybe in the future we'll have another priority option.

	-J

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

Re: [PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[Hangbin Liu]

On Thu, Aug 28, 2025 at 09:59:20AM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >On Wed, Aug 27, 2025 at 11:43:19AM -0700, Jay Vosburgh wrote:
> >> Hangbin Liu <liuhangbin@gmail.com> wrote:
> >> 
> >> >Add support for the actor_port_prio option for bond slaves.
> >> >This per-port priority can be used by the bonding driver in ad_select to
> >> >choose the higher-priority aggregator during failover.
> >> >
> >> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> >> >---
> >> >v4: no update
> >> >v3: rename ad_actor_port_prio to actor_port_prio
> >> >v2: no update
> >> >---
> >> > ip/iplink_bond.c       |  1 +
> >> > ip/iplink_bond_slave.c | 18 ++++++++++++++++--
> >> > man/man8/ip-link.8.in  |  6 ++++++
> >> > 3 files changed, 23 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
> >> >index d6960f6d9b03..1a2c1b3042a0 100644
> >> >--- a/ip/iplink_bond.c
> >> >+++ b/ip/iplink_bond.c
> >> >@@ -91,6 +91,7 @@ static const char *ad_select_tbl[] = {
> >> > 	"stable",
> >> > 	"bandwidth",
> >> > 	"count",
> >> >+	"prio",
> >> 
> >> 	Should this be actor_port_prio?
> >
> >hmm, actor_port_prio correspond to the ip link option name, which is also
> >acceptable.
> 
> 	Isn't this the text of the ip link option name right here (in
> the sense of what goes on the "ip link" command line)?

"stable", "bandwidth", "count" are not ip link parameters, and same with
kernel names, so I also used the kernel name "prio" here.
> 
> >While in kernel, we defined the select policy as
> >
> >        { "stable",    BOND_AD_STABLE,    BOND_VALFLAG_DEFAULT},
> >        { "bandwidth", BOND_AD_BANDWIDTH, 0},
> >        { "count",     BOND_AD_COUNT,     0},
> >+       { "prio",      BOND_AD_PRIO,      0},
> 
> 	Maybe my memory is starting to go, but I thought in a prior
> discussion we'd agreed to change this as well for consistency.

Maybe I didn't get your comment[1] correctly. I only changed
`ad_actor_port_prio` to `actor_port_prio` last time.

> 
> >So I think the prio here should also be OK.
> >
> >You can decide which one to use.
> 
> 	I would prefer that the two options have discrete names, or,
> really, that we not repeat "prio" as it's already used elsewhere.  Plus,
> who knows, maybe in the future we'll have another priority option.

OK, do not use same name for different usage. I will also change the "prio"
to "actor_port_prio" in next patch.

[1] https://lore.kernel.org/netdev/1109153.1755380673@famine/

Thanks
Hangbin

Re: [PATCHv4 iproute2-next] iplink: bond_slave: add support for actor_port_prio

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Thu, Aug 28, 2025 at 09:59:20AM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@gmail.com> wrote:
>> 
>> >On Wed, Aug 27, 2025 at 11:43:19AM -0700, Jay Vosburgh wrote:
>> >> Hangbin Liu <liuhangbin@gmail.com> wrote:
>> >> 
>> >> >Add support for the actor_port_prio option for bond slaves.
>> >> >This per-port priority can be used by the bonding driver in ad_select to
>> >> >choose the higher-priority aggregator during failover.
>> >> >
>> >> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> >> >---
>> >> >v4: no update
>> >> >v3: rename ad_actor_port_prio to actor_port_prio
>> >> >v2: no update
>> >> >---
>> >> > ip/iplink_bond.c       |  1 +
>> >> > ip/iplink_bond_slave.c | 18 ++++++++++++++++--
>> >> > man/man8/ip-link.8.in  |  6 ++++++
>> >> > 3 files changed, 23 insertions(+), 2 deletions(-)
>> >> >
>> >> >diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
>> >> >index d6960f6d9b03..1a2c1b3042a0 100644
>> >> >--- a/ip/iplink_bond.c
>> >> >+++ b/ip/iplink_bond.c
>> >> >@@ -91,6 +91,7 @@ static const char *ad_select_tbl[] = {
>> >> > 	"stable",
>> >> > 	"bandwidth",
>> >> > 	"count",
>> >> >+	"prio",
>> >> 
>> >> 	Should this be actor_port_prio?
>> >
>> >hmm, actor_port_prio correspond to the ip link option name, which is also
>> >acceptable.
>> 
>> 	Isn't this the text of the ip link option name right here (in
>> the sense of what goes on the "ip link" command line)?
>
>"stable", "bandwidth", "count" are not ip link parameters, and same with
>kernel names, so I also used the kernel name "prio" here.

	Perhaps I wasn't explaining sufficiently; I mean that the text
in ad_select_tbl[] is where the name of the option settings comes from
for ip link whatever commands.  E.g., If I do something like

ip link add dev bond0 type bond [...] ad_select rutabaga

	The "rutabaga" text is being compared to the elements of
ad_select_tbl[], correct?

	Anyway, what I meant is that I think setting this new priority
option should look something like:

ip link add dev bond0 type bond [...] ad_select actor_port_prio

	So that it's called the same thing everwhere, and it matches the
nomenclature used in the standard.

	-J

>> >While in kernel, we defined the select policy as
>> >
>> >        { "stable",    BOND_AD_STABLE,    BOND_VALFLAG_DEFAULT},
>> >        { "bandwidth", BOND_AD_BANDWIDTH, 0},
>> >        { "count",     BOND_AD_COUNT,     0},
>> >+       { "prio",      BOND_AD_PRIO,      0},
>> 
>> 	Maybe my memory is starting to go, but I thought in a prior
>> discussion we'd agreed to change this as well for consistency.
>
>Maybe I didn't get your comment[1] correctly. I only changed
>`ad_actor_port_prio` to `actor_port_prio` last time.
>
>> 
>> >So I think the prio here should also be OK.
>> >
>> >You can decide which one to use.
>> 
>> 	I would prefer that the two options have discrete names, or,
>> really, that we not repeat "prio" as it's already used elsewhere.  Plus,
>> who knows, maybe in the future we'll have another priority option.
>
>OK, do not use same name for different usage. I will also change the "prio"
>to "actor_port_prio" in next patch.
>
>[1] https://lore.kernel.org/netdev/1109153.1755380673@famine/
>
>Thanks
>Hangbin

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