Message-Id: <B3527729-560E-485C-8E1F-F335236699FD@bamaicloud.com>
Date: Wed, 9 Jul 2025 19:34:55 +0800
From: Tonghao Zhang <tonghao@...aicloud.com>
To: Jay Vosburgh <jv@...sburgh.net>
Cc: 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>,
 Nikolay Aleksandrov <razor@...ckwall.org>,
 Zengbing Tu <tuzengbing@...iglobal.com>
Subject: Re: [PATCH net-next RESEND] net: bonding: add bond_is_icmpv6_nd()
 helper



> 2025年7月9日 10:04，Jay Vosburgh <jv@...sburgh.net> 写道：
> 
> Tonghao Zhang <tonghao@...aicloud.com> 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>
>> Signed-off-by: Tonghao Zhang <tonghao@...aicloud.com>
>> Signed-off-by: Zengbing Tu <tuzengbing@...iglobal.com>
>> ---
>> drivers/net/bonding/bond_alb.c  | 32 +++++++-------------------------
>> drivers/net/bonding/bond_main.c | 17 ++---------------
>> include/net/bonding.h           | 19 +++++++++++++++++++
>> 3 files changed, 28 insertions(+), 40 deletions(-)
>> 
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index 2d37b07c8215..8e5b9ce52077 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):
>> @@ -1467,16 +1445,20 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>> break;
>> }
>> 
>> - if (alb_determine_nd(skb, bond)) {
>> + if (bond_is_icmpv6_nd(skb)) {
>> 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.
>>  */
>> + if (!pskb_network_may_pull(skb, sizeof(*ip6hdr))) {
>> + do_tx_balance = false;
>> + break;
>> + }
>> +
> 
> It's nice to consolidate some duplicate code and use the more
> efficient skb_header_pointer, but the above will do a pull anyway for
> nearly every packet that passed through the function.
> 
> Would it be better to not use the bond_is_icmpv6_nd helper here,
> but instead call skb_header_pointer directly, and then reuse its
> returned data for the hash computation that occurs just after this point
> in the code?
sounds good.
> 
>> ip6hdr = ipv6_hdr(skb);
>> if (ipv6_addr_any(&ip6hdr->saddr)) {
>> do_tx_balance = false;
>> 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)
>> +{
> 
> Minor nit: no need to specify inline, the compiler will decide.
Hi Jay
If we don’t use the inline in the header file, the linker will report an error. I think we should keep the ‘inline’.
ld: drivers/net/bonding/bond_procfs.o: in function `bond_is_icmpv6_nd':
/root/net-next/./include/net/bonding.h:819: multiple definition of `bond_is_icmpv6_nd'; drivers/net/bonding/bond_main.o:/root/net-next/./include/net/bonding.h:819: first defined here
> 
> -J
> 
>> + 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
>> 
> 
> ---
> -Jay Vosburgh, jv@...sburgh.net

Powered by blists - more mailing lists