Re: [PATCHv2 net-next 1/3] bonding: add support for per-port LACP actor priority

[Jonas Gorski <jonas.gorski@gmail.com>]

Hi,

On 14.08.25 12:42, Hangbin Liu wrote:
> Introduce a new netlink attribute 'ad_actor_port_prio' to allow setting
> the LACP actor port priority on a per-slave basis. This extends the
> existing bonding infrastructure to support more granular control over
> LACP negotiations.
> 
> The priority value is embedded in LACPDU packets and will be used by
> subsequent patches to influence aggregator selection policies.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  Documentation/networking/bonding.rst |  9 +++++++
>  drivers/net/bonding/bond_3ad.c       |  2 ++
>  drivers/net/bonding/bond_netlink.c   | 16 +++++++++++++
>  drivers/net/bonding/bond_options.c   | 36 ++++++++++++++++++++++++++++
>  include/net/bond_3ad.h               |  1 +
>  include/net/bond_options.h           |  1 +
>  include/uapi/linux/if_link.h         |  1 +
>  7 files changed, 66 insertions(+)
> 
> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index f8f5766703d4..874d8a4681ec 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -193,6 +193,15 @@ ad_actor_sys_prio
>  	This parameter has effect only in 802.3ad mode and is available through
>  	SysFs interface.
>  
> +ad_actor_port_prio
> +
> +	In an AD system, this specifies the port priority. The allowed range
> +	is 1 - 65535. If the value is not specified, it takes 255 as the
> +	default value.
> +
> +	This parameter has effect only in 802.3ad mode and is available through
> +	netlink interface.
> +
>  ad_actor_system
>  
>  	In an AD system, this specifies the mac-address for the actor in
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 2fca8e84ab10..19b389b81600 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -436,6 +436,7 @@ static void __ad_actor_update_port(struct port *port)
>  
>  	port->actor_system = BOND_AD_INFO(bond).system.sys_mac_addr;
>  	port->actor_system_priority = BOND_AD_INFO(bond).system.sys_priority;
> +	port->actor_port_priority = SLAVE_AD_INFO(port->slave)->port_priority;
>  }
>  
>  /* Conversions */
> @@ -2211,6 +2212,7 @@ void bond_3ad_bind_slave(struct slave *slave)
>  		port->actor_admin_port_key = bond->params.ad_user_port_key << 6;
>  		ad_update_actor_keys(port, false);
>  		/* actor system is the bond's system */
> +		SLAVE_AD_INFO(slave)->port_priority = port->actor_port_priority;

I don't know the code flow well, but the assignment direction here is the
opposite of other values, e.g.

                port->actor_port_number = SLAVE_AD_INFO(slave)->id;

so I would have expected this to be

		port->actor_port_priority = SLAVE_AD_INFO(slave)->port_priority;

Is this intentional?

>  		__ad_actor_update_port(port);
>  		/* tx timer(to verify that no more than MAX_TX_IN_SECOND
>  		 * lacpdu's are sent in one second)
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 57fff2421f1b..3a37298583ed 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -28,6 +28,7 @@ static size_t bond_get_slave_size(const struct net_device *bond_dev,
>  		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 */
> +		nla_total_size(sizeof(u16)) +	/* IFLA_BOND_SLAVE_AD_ACTOR_PORT_PRIO */
>  		0;
>  }
>  
> @@ -77,6 +78,10 @@ static int bond_fill_slave_info(struct sk_buff *skb,
>  					ad_port->partner_oper.port_state))
>  				goto nla_put_failure;
>  		}
> +
> +		if (nla_put_u16(skb, IFLA_BOND_SLAVE_AD_ACTOR_PORT_PRIO,
> +				SLAVE_AD_INFO(slave)->port_priority))
> +			goto nla_put_failure;


This is an 802.3ad (exclusive) setting, shouldn't this be in the

  if (BOND_MODE(slave->bond) == BOND_MODE_8023AD) { }

block above this?

Regards,
Jonas