Message-Id: <638B6CB1-BCC6-4887-96F2-013196DA5138@bamaicloud.com>
Date: Mon, 12 May 2025 22:55:02 +0800
From: Tonghao Zhang <tonghao@...aicloud.com>
To: Jay Vosburgh <jv@...sburgh.net>
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



> 2025年5月12日 17:19，Jay Vosburgh <jv@...sburgh.net> 写道：
> 
> 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.
I got it, thanks Jay, and Andrew.
> 
> -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

Powered by blists - more mailing lists