netdev - Re: [PATCH net-next 1/4] net: bonding: add broadcast

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <1278464.1747041594@vermin>
Date: Mon, 12 May 2025 11:19:54 +0200
From: Jay Vosburgh <jv@...sburgh.net>
To: Tonghao Zhang <tonghao@...aicloud.com>
cc: Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
    "David S. Miller" <davem@...emloft.net>,
    Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
    Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
    Jonathan Corbet <corbet@....net>,
    Andrew Lunn <andrew+netdev@...n.ch>
Subject: Re: [PATCH net-next 1/4] net: bonding: add broadcast_neighbor option for 802.3ad

Tonghao Zhang <tonghao@...aicloud.com> wrote:

>> 2025年5月11日 下午11:53，Andrew Lunn <andrew@...n.ch> 写道：
>> 
>>> static inline bool bond_should_broadcast_neighbor(struct bonding *bond,
>>>                                                  struct sk_buff *skb)
>>> {
>>>        if (!bond->params.broadcast_neighbor ||
>>>            BOND_MODE(bond) != BOND_MODE_8023AD)
>>>                return false;
>> 
>> I think you missed the point. You have added these two tests to every
>> packet on the fast path. And it is very likely to return false. Is
>> bond.params.broadcast_neighbor likely to be in the cache? A cache miss
>> is expensive. Is bond.params.mode also likely to be in cache? You
>> placed broadcast_neighbor at the end of params, so it is unlikely to
>> be in the same cache line as bond.params.mode. So two cache misses.
>> 
>> What Jay would like is that the cost on the fast path is ~0 for when
>> this feature is not in use. Jump labels can achieve this. It inserts
>> either a NOP or a jump instruction, which costs nearly nothing, and
>> then uses self modifying code to swap between a NOP or a jump. You can
>> keep a global view of is any bond is using this new mode? If no, this
>No, no mode uses jump labels instead of bond.params checking.

	The suggestion here is to use a jump label (static branch) to
essentially eliminate the overhead of the options test for the common case
for most users, which is with broadcast_neighbor disabled.

	As described below, the static branch would be tracked
separately from the per-bond option.

>> test is eliminated. If yes, you do the test.
>I test the lacp mode with broadcast_neighbor enabled, there is no performance drop. This patch has been running in our production environment for a long time. We only use this option in lacp mode, for performance, the code can be modified as follows:

	How did you test this?  The performance under discussion here is
that branches in the packet transmit path can affect overall packet
transmission rates at very high rates (think in terms of small packet
rates at 40 Gb/sec and higher).  Bonding already has a significant
number of TX path branches, and we should be working to reduce that
number, not increase it.

>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index ce31445e85b6..8743bf007b7e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -5330,11 +5330,12 @@ static struct slave *bond_xdp_xmit_3ad_xor_slave_get(struct bonding *bond,
>        return slaves->arr[hash % count];
> }
>
>-static inline bool bond_should_broadcast_neighbor(struct bonding *bond,
>-                                                 struct sk_buff *skb)
>+static inline bool bond_should_broadcast_neighbor(struct sk_buff *skb,
>+                                                 struct net_device *dev)
> {
>-       if (!bond->params.broadcast_neighbor ||
>-           BOND_MODE(bond) != BOND_MODE_8023AD)
>+       struct bonding *bond = netdev_priv(dev);
>+
>+       if (!bond->params.broadcast_neighbor)
>                return false;

	Using a static branch, the above would be preceded by something
like:

	if (!static_branch_unlikely(&bond_bcast_neigh_enabled))
		return false;

	With additional logic in the options code that enables and
disables broadcast_neighbor that will increment or decrement (via
static_branch_inc / _dec) bond_bcast_neigh_enabled as the
broadcast_neighbor option is enabled or disabled.  The static branch
becomes a fast way to ask "is any bond in the system using
broadcast_neighbor" at very low cost.

	As Andrew helpfully pointed out, netfilter makes extensive use
of these; I'd suggest looking at the usage of something like
nft_trace_enabled as an example of what we're referring to.

	-J

>        if (skb->protocol == htons(ETH_P_ARP))
>@@ -5408,9 +5409,6 @@ static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
>        struct bond_up_slave *slaves;
>        struct slave *slave;
>
>-       if (bond_should_broadcast_neighbor(bond, skb))
>-               return bond_xmit_broadcast(skb, dev);
>-
>        slaves = rcu_dereference(bond->usable_slaves);
>        slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
>        if (likely(slave))
>@@ -5625,6 +5623,9 @@ static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev
>        case BOND_MODE_ACTIVEBACKUP:
>                return bond_xmit_activebackup(skb, dev);
>        case BOND_MODE_8023AD:
>+               if (bond_should_broadcast_neighbor(skb, dev))
>+                       return bond_xmit_broadcast(skb, dev);
>+               fallthrough;
>        case BOND_MODE_XOR:
>                return bond_3ad_xor_xmit(skb, dev);
>        case BOND_MODE_BROADCAST:
>> 
>> 	Andrew

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