Message-ID: <aHWzk3NwurSH-Jjt@fedora>
Date: Tue, 15 Jul 2025 01:49:07 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Tonghao Zhang <tonghao@...aicloud.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

On Mon, Jul 14, 2025 at 08:53:31PM +0800, Tonghao Zhang wrote:
> 
> 
> > 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.

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@...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