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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2030137.1756513356@famine>
Date: Fri, 29 Aug 2025 17:22:36 -0700
From: Jay Vosburgh <jv@...sburgh.net>
To: Hangbin Liu <liuhangbin@...il.com>
cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
    "David S. Miller" <davem@...emloft.net>,
    Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
    Paolo Abeni <pabeni@...hat.com>,
    Nikolay Aleksandrov <razor@...ckwall.org>,
    Simon Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>,
    Jonathan Corbet <corbet@....net>, Petr Machata <petrm@...dia.com>,
    Amit Cohen <amcohen@...dia.com>,
    Vladimir Oltean <vladimir.oltean@....com>,
    Stephen Hemminger <stephen@...workplumber.org>,
    David Ahern <dsahern@...il.com>,
    Jonas Gorski <jonas.gorski@...il.com>, linux-doc@...r.kernel.org,
    linux-kselftest@...r.kernel.org
Subject: Re: [PATCHv4 iproute2-next] iplink: bond_slave: add support for
 actor_port_prio

Hangbin Liu <liuhangbin@...il.com> wrote:

>On Thu, Aug 28, 2025 at 09:59:20AM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@...il.com> wrote:
>> 
>> >On Wed, Aug 27, 2025 at 11:43:19AM -0700, Jay Vosburgh wrote:
>> >> Hangbin Liu <liuhangbin@...il.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@...il.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@...sburgh.net