[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <125F3BD1-1DC7-42AF-AAB4-167AD665D687@bamaicloud.com>
Date: Mon, 14 Jul 2025 20:53:31 +0800
From: Tonghao Zhang <tonghao@...aicloud.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org,
Jay Vosburgh <jv@...sburgh.net>,
"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>,
Nikolay Aleksandrov <razor@...ckwall.org>,
Zengbing Tu <tuzengbing@...iglobal.com>
Subject: Re: [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
> 2025年7月14日 15:13,Hangbin Liu <liuhangbin@...il.com> 写道:
>
>
> Hmm, I don’t see much improvement with this patch compared to without it.
> So I don’t think this update is necessary.
This patch use the skb_header_pointer instead of pskb_network_may_pull. The skb_header_pointer is more efficient than pskb_network_may_pull.
And use the comm helper can consolidate some duplicate code.
Did you see the comments by Jay. BTW, I find you add the reviewed-by tag on v1 patch.
>
> On Thu, Jul 10, 2025 at 05:16:36PM +0800, Tonghao Zhang wrote:
>> Introduce ipv6 ns/nd checking helper, using skb_header_pointer()
>> instead of pskb_network_may_pull() on tx path.
>>
>> alb_determine_nd introduced from commit 0da8aa00bfcfe
>>
>> Cc: Jay Vosburgh <jv@...sburgh.net>
>> Cc: "David S. Miller" <davem@...emloft.net>
>> Cc: Eric Dumazet <edumazet@...gle.com>
>> Cc: Jakub Kicinski <kuba@...nel.org>
>> Cc: Paolo Abeni <pabeni@...hat.com>
>> Cc: Simon Horman <horms@...nel.org>
>> Cc: Jonathan Corbet <corbet@....net>
>> Cc: Andrew Lunn <andrew+netdev@...n.ch>
>> Cc: Nikolay Aleksandrov <razor@...ckwall.org>
>> Cc: Hangbin Liu <liuhangbin@...il.com>
>> Signed-off-by: Tonghao Zhang <tonghao@...aicloud.com>
>> Signed-off-by: Zengbing Tu <tuzengbing@...iglobal.com>
>> ---
>> v2:
>> - in alb mode, replace bond_is_icmpv6_nd with skb_header_pointer directly,
>> - and then reuse its returned data for the hash computation.
>>
>> v1:
>> - https://patchwork.kernel.org/project/netdevbpf/patch/20250708123251.2475-1-tonghao@bamaicloud.com/
>> ---
>> drivers/net/bonding/bond_alb.c | 47 +++++++++++----------------------
>> drivers/net/bonding/bond_main.c | 17 ++----------
>> include/net/bonding.h | 19 +++++++++++++
>> 3 files changed, 37 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index 2d37b07c8215..a37709fd7475 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -19,7 +19,6 @@
>> #include <linux/in.h>
>> #include <net/arp.h>
>> #include <net/ipv6.h>
>> -#include <net/ndisc.h>
>> #include <asm/byteorder.h>
>> #include <net/bonding.h>
>> #include <net/bond_alb.h>
>> @@ -1280,27 +1279,6 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
>> return res;
>> }
>>
>> -/* determine if the packet is NA or NS */
>> -static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
>> -{
>> - struct ipv6hdr *ip6hdr;
>> - struct icmp6hdr *hdr;
>> -
>> - if (!pskb_network_may_pull(skb, sizeof(*ip6hdr)))
>> - return true;
>> -
>> - ip6hdr = ipv6_hdr(skb);
>> - if (ip6hdr->nexthdr != IPPROTO_ICMPV6)
>> - return false;
>> -
>> - if (!pskb_network_may_pull(skb, sizeof(*ip6hdr) + sizeof(*hdr)))
>> - return true;
>> -
>> - hdr = icmp6_hdr(skb);
>> - return hdr->icmp6_type == NDISC_NEIGHBOUR_ADVERTISEMENT ||
>> - hdr->icmp6_type == NDISC_NEIGHBOUR_SOLICITATION;
>> -}
>> -
>> /************************ exported alb functions ************************/
>>
>> int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>> @@ -1381,7 +1359,7 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>> if (!is_multicast_ether_addr(eth_data->h_dest)) {
>> switch (skb->protocol) {
>> case htons(ETH_P_IPV6):
>> - if (alb_determine_nd(skb, bond))
>> + if (bond_is_icmpv6_nd(skb))
>> break;
>> fallthrough;
>> case htons(ETH_P_IP):
>> @@ -1426,6 +1404,10 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>> struct ethhdr *eth_data;
>> u32 hash_index = 0;
>> int hash_size = 0;
>> + struct {
>> + struct ipv6hdr ip6;
>> + struct icmp6hdr icmp6;
>> + } *combined, _combined;
>>
>> skb_reset_mac_header(skb);
>> eth_data = eth_hdr(skb);
>> @@ -1449,8 +1431,6 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>> break;
>> }
>> case ETH_P_IPV6: {
>> - const struct ipv6hdr *ip6hdr;
>> -
>> /* IPv6 doesn't really use broadcast mac address, but leave
>> * that here just in case.
>> */
>> @@ -1467,24 +1447,29 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>> break;
>> }
>>
>> - if (alb_determine_nd(skb, bond)) {
>> + /* Do not tx balance any IPv6 NS/NA packets. */
>> + combined = skb_header_pointer(skb, skb_mac_header_len(skb),
>> + sizeof(_combined), &_combined);
>> + if (!combined || (combined->ip6.nexthdr == NEXTHDR_ICMP &&
>> + (combined->icmp6.icmp6_type ==
>> + NDISC_NEIGHBOUR_SOLICITATION ||
>> + combined->icmp6.icmp6_type ==
>> + NDISC_NEIGHBOUR_ADVERTISEMENT))) {
>> do_tx_balance = false;
>> break;
>> }
>>
>> - /* The IPv6 header is pulled by alb_determine_nd */
>> /* Additionally, DAD probes should not be tx-balanced as that
>> * will lead to false positives for duplicate addresses and
>> * prevent address configuration from working.
>> */
>> - ip6hdr = ipv6_hdr(skb);
>> - if (ipv6_addr_any(&ip6hdr->saddr)) {
>> + if (ipv6_addr_any(&combined->ip6.saddr)) {
>> do_tx_balance = false;
>> break;
>> }
>>
>> - hash_start = (char *)&ip6hdr->daddr;
>> - hash_size = sizeof(ip6hdr->daddr);
>> + hash_start = (char *)&combined->ip6.daddr;
>> + hash_size = sizeof(combined->ip6.daddr);
>> break;
>> }
>> case ETH_P_ARP:
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 17c7542be6a5..a8034a561011 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5338,10 +5338,6 @@ static bool bond_should_broadcast_neighbor(struct sk_buff *skb,
>> struct net_device *dev)
>> {
>> struct bonding *bond = netdev_priv(dev);
>> - struct {
>> - struct ipv6hdr ip6;
>> - struct icmp6hdr icmp6;
>> - } *combined, _combined;
>>
>> if (!static_branch_unlikely(&bond_bcast_neigh_enabled))
>> return false;
>> @@ -5349,19 +5345,10 @@ static bool bond_should_broadcast_neighbor(struct sk_buff *skb,
>> if (!bond->params.broadcast_neighbor)
>> return false;
>>
>> - if (skb->protocol == htons(ETH_P_ARP))
>> + if (skb->protocol == htons(ETH_P_ARP) ||
>> + (skb->protocol == htons(ETH_P_IPV6) && bond_is_icmpv6_nd(skb)))
>> return true;
>>
>> - if (skb->protocol == htons(ETH_P_IPV6)) {
>> - combined = skb_header_pointer(skb, skb_mac_header_len(skb),
>> - sizeof(_combined),
>> - &_combined);
>> - if (combined && combined->ip6.nexthdr == NEXTHDR_ICMP &&
>> - (combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_SOLICITATION ||
>> - combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_ADVERTISEMENT))
>> - return true;
>> - }
>> -
>> return false;
>> }
>>
>> diff --git a/include/net/bonding.h b/include/net/bonding.h
>> index e06f0d63b2c1..32d9fcca858c 100644
>> --- a/include/net/bonding.h
>> +++ b/include/net/bonding.h
>> @@ -29,6 +29,7 @@
>> #include <net/bond_options.h>
>> #include <net/ipv6.h>
>> #include <net/addrconf.h>
>> +#include <net/ndisc.h>
>>
>> #define BOND_MAX_ARP_TARGETS 16
>> #define BOND_MAX_NS_TARGETS BOND_MAX_ARP_TARGETS
>> @@ -814,4 +815,22 @@ static inline netdev_tx_t bond_tx_drop(struct net_device *dev, struct sk_buff *s
>> return NET_XMIT_DROP;
>> }
>>
>> +static inline bool bond_is_icmpv6_nd(struct sk_buff *skb)
>> +{
>> + struct {
>> + struct ipv6hdr ip6;
>> + struct icmp6hdr icmp6;
>> + } *combined, _combined;
>> +
>> + combined = skb_header_pointer(skb, skb_mac_header_len(skb),
>> + sizeof(_combined),
>> + &_combined);
>> + if (combined && combined->ip6.nexthdr == NEXTHDR_ICMP &&
>> + (combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_SOLICITATION ||
>> + combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_ADVERTISEMENT))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> #endif /* _NET_BONDING_H */
>> --
>> 2.34.1
>>
>
>
Powered by blists - more mailing lists