* [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
@ 2025-07-10 9:16 Tonghao Zhang
2025-07-14 7:13 ` Hangbin Liu
0 siblings, 1 reply; 6+ messages in thread
From: Tonghao Zhang @ 2025-07-10 9:16 UTC (permalink / raw)
To: netdev
Cc: Tonghao Zhang, Jay Vosburgh, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Andrew Lunn, Nikolay Aleksandrov, Hangbin Liu, Zengbing Tu
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@jvosburgh.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Nikolay Aleksandrov <razor@blackwall.org>
Cc: Hangbin Liu <liuhangbin@gmail.com>
Signed-off-by: Tonghao Zhang <tonghao@bamaicloud.com>
Signed-off-by: Zengbing Tu <tuzengbing@didiglobal.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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
2025-07-10 9:16 [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper Tonghao Zhang
@ 2025-07-14 7:13 ` Hangbin Liu
2025-07-14 12:53 ` Tonghao Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Hangbin Liu @ 2025-07-14 7:13 UTC (permalink / raw)
To: Tonghao Zhang
Cc: netdev, Jay Vosburgh, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Andrew Lunn, Nikolay Aleksandrov, Zengbing Tu
Hmm, I don’t see much improvement with this patch compared to without it.
So I don’t think this update is necessary.
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@jvosburgh.net>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: Nikolay Aleksandrov <razor@blackwall.org>
> Cc: Hangbin Liu <liuhangbin@gmail.com>
> Signed-off-by: Tonghao Zhang <tonghao@bamaicloud.com>
> Signed-off-by: Zengbing Tu <tuzengbing@didiglobal.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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
2025-07-14 7:13 ` Hangbin Liu
@ 2025-07-14 12:53 ` Tonghao Zhang
2025-07-15 1:49 ` Hangbin Liu
2025-07-15 10:33 ` Paolo Abeni
0 siblings, 2 replies; 6+ messages in thread
From: Tonghao Zhang @ 2025-07-14 12:53 UTC (permalink / raw)
To: Hangbin Liu
Cc: netdev, Jay Vosburgh, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Andrew Lunn, Nikolay Aleksandrov, Zengbing Tu
> 2025年7月14日 15:13，Hangbin Liu <liuhangbin@gmail.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@jvosburgh.net>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: Simon Horman <horms@kernel.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
>> Cc: Nikolay Aleksandrov <razor@blackwall.org>
>> Cc: Hangbin Liu <liuhangbin@gmail.com>
>> Signed-off-by: Tonghao Zhang <tonghao@bamaicloud.com>
>> Signed-off-by: Zengbing Tu <tuzengbing@didiglobal.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
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
2025-07-14 12:53 ` Tonghao Zhang
@ 2025-07-15 1:49 ` Hangbin Liu
2025-07-15 10:33 ` Paolo Abeni
1 sibling, 0 replies; 6+ messages in thread
From: Hangbin Liu @ 2025-07-15 1:49 UTC (permalink / raw)
To: Tonghao Zhang
Cc: netdev, Jay Vosburgh, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Andrew Lunn, Nikolay Aleksandrov, Zengbing Tu
On Mon, Jul 14, 2025 at 08:53:31PM +0800, Tonghao Zhang wrote:
>
>
> > 2025年7月14日 15:13，Hangbin Liu <liuhangbin@gmail.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.
I don't think skb_header_pointer is more efficient.
> 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.
Yes. I add review tag for v1. But for v2 I feel this doesn't reduce much
duplicate code, while it make the code more complex. Just my feeling, no offense.
I will left the patch review to Jay.
Thanks
Hangbin
>
> >
> > 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@jvosburgh.net>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> Cc: Eric Dumazet <edumazet@google.com>
> >> Cc: Jakub Kicinski <kuba@kernel.org>
> >> Cc: Paolo Abeni <pabeni@redhat.com>
> >> Cc: Simon Horman <horms@kernel.org>
> >> Cc: Jonathan Corbet <corbet@lwn.net>
> >> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> >> Cc: Nikolay Aleksandrov <razor@blackwall.org>
> >> Cc: Hangbin Liu <liuhangbin@gmail.com>
> >> Signed-off-by: Tonghao Zhang <tonghao@bamaicloud.com>
> >> Signed-off-by: Zengbing Tu <tuzengbing@didiglobal.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
> >>
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
2025-07-14 12:53 ` Tonghao Zhang
2025-07-15 1:49 ` Hangbin Liu
@ 2025-07-15 10:33 ` Paolo Abeni
2025-07-16 1:44 ` Jay Vosburgh
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2025-07-15 10:33 UTC (permalink / raw)
To: Tonghao Zhang, Hangbin Liu
Cc: netdev, Jay Vosburgh, David S. Miller, Eric Dumazet,
Jakub Kicinski, Simon Horman, Jonathan Corbet, Andrew Lunn,
Nikolay Aleksandrov, Zengbing Tu
On 7/14/25 2:53 PM, Tonghao Zhang wrote:
>> 2025年7月14日 15:13，Hangbin Liu <liuhangbin@gmail.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.
I think the eventual cleanup here is very subjective, especially
compared to the diffstat. Any eventual performance improvement should be
supported by some figures, in relevant tests.
In this specific case I don't think you will be able to measure any
relevant gain; pskb_network_may_pull() could be slower than
skb_header_pointer() only when the headers are not in the liner part,
and that in turns could happen only if we are already on some kind of
slow path.
/P
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper
2025-07-15 10:33 ` Paolo Abeni
@ 2025-07-16 1:44 ` Jay Vosburgh
0 siblings, 0 replies; 6+ messages in thread
From: Jay Vosburgh @ 2025-07-16 1:44 UTC (permalink / raw)
To: Paolo Abeni
Cc: Tonghao Zhang, Hangbin Liu, netdev, David S. Miller, Eric Dumazet,
Jakub Kicinski, Simon Horman, Jonathan Corbet, Andrew Lunn,
Nikolay Aleksandrov, Zengbing Tu
Paolo Abeni <pabeni@redhat.com> wrote:
>On 7/14/25 2:53 PM, Tonghao Zhang wrote:
>>> 2025年7月14日 15:13，Hangbin Liu <liuhangbin@gmail.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.
>
>I think the eventual cleanup here is very subjective, especially
>compared to the diffstat. Any eventual performance improvement should be
>supported by some figures, in relevant tests.
>
>In this specific case I don't think you will be able to measure any
>relevant gain; pskb_network_may_pull() could be slower than
>skb_header_pointer() only when the headers are not in the liner part,
>and that in turns could happen only if we are already on some kind of
>slow path.
Does the ICMP6 header of a received packet end up in the linear
part, or in a frag? That's what's being inspected by this code, and I
presumed that the linear area would be just the protocol headers
although thinking about it now, I suppose ICMPv6 is a protocol, but I
don't know how often the pull of the ICMPv6 message data would have to
copy to the linear area.
-J
---
-Jay Vosburgh, jv@jvosburgh.net
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-16 1:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-10 9:16 [PATCH net-next v2] net: bonding: add bond_is_icmpv6_nd() helper Tonghao Zhang
2025-07-14 7:13 ` Hangbin Liu
2025-07-14 12:53 ` Tonghao Zhang
2025-07-15 1:49 ` Hangbin Liu
2025-07-15 10:33 ` Paolo Abeni
2025-07-16 1:44 ` Jay Vosburgh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).