diff mbox series

[net-next,4/4] ipv4: remove fib_info_devhash[]

Message ID 20241004134720.579244-5-edumazet@google.com (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series ipv4: preliminary work for per-netns RTNL | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 43 this patch: 43
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: corbet@lwn.net linux-doc@vger.kernel.org
netdev/build_clang success Errors and warnings before: 83 this patch: 83
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4144 this patch: 4144
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 110 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 22 this patch: 23
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet Oct. 4, 2024, 1:47 p.m. UTC 
Upcoming per-netns RTNL conversion needs to get rid
of shared hash tables.

fib_info_devhash[] is one of them.

It is unclear why we used a hash table, because
a single hlist_head per net device was cheaper and scalable.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 .../networking/net_cachelines/net_device.rst  |  1 +
 include/linux/netdevice.h                     |  2 ++
 net/ipv4/fib_semantics.c                      | 35 ++++++++-----------
 3 files changed, 18 insertions(+), 20 deletions(-)

Comments

Kuniyuki Iwashima Oct. 4, 2024, 10:45 p.m. UTC | #1 
From: Eric Dumazet <edumazet@google.com>
Date: Fri,  4 Oct 2024 13:47:20 +0000
> Upcoming per-netns RTNL conversion needs to get rid
> of shared hash tables.
> 
> fib_info_devhash[] is one of them.
> 
> It is unclear why we used a hash table, because
> a single hlist_head per net device was cheaper and scalable.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks!
Simon Horman Oct. 7, 2024, 2:08 p.m. UTC | #2 
On Fri, Oct 04, 2024 at 01:47:20PM +0000, Eric Dumazet wrote:
> Upcoming per-netns RTNL conversion needs to get rid
> of shared hash tables.
> 
> fib_info_devhash[] is one of them.
> 
> It is unclear why we used a hash table, because
> a single hlist_head per net device was cheaper and scalable.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  .../networking/net_cachelines/net_device.rst  |  1 +
>  include/linux/netdevice.h                     |  2 ++
>  net/ipv4/fib_semantics.c                      | 35 ++++++++-----------
>  3 files changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
> index 22b07c814f4a4575d255fdf472d07c549536e543..a8e2a7ce0383343464800be8db31aeddd791f086 100644
> --- a/Documentation/networking/net_cachelines/net_device.rst
> +++ b/Documentation/networking/net_cachelines/net_device.rst
> @@ -83,6 +83,7 @@ unsigned_int                        allmulti
>  bool                                uc_promisc                                                      
>  unsigned_char                       nested_level                                                    
>  struct_in_device*                   ip_ptr                  read_mostly         read_mostly         __in_dev_get
> +struct hlist_head                   fib_nh_head
>  struct_inet6_dev*                   ip6_ptr                 read_mostly         read_mostly         __in6_dev_get
>  struct_vlan_info*                   vlan_info                                                       
>  struct_dsa_port*                    dsa_ptr                                                         

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 4d20c776a4ff3d0e881b8d9b99901edb35f66da2..cda20a3fe1adf54c1e6df5b5a8882ef7830e1b46 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2209,6 +2209,8 @@ struct net_device {
>  
>  	/* Protocol-specific pointers */
>  	struct in_device __rcu	*ip_ptr;
> +	struct hlist_head	fib_nh_head;
> +
>  #if IS_ENABLED(CONFIG_VLAN_8021Q)
>  	struct vlan_info __rcu	*vlan_info;
>  #endif

Hi Eric,

A minor nit from my side: Kernel-doc should be updated for this new field.

...
Eric Dumazet Oct. 7, 2024, 2:13 p.m. UTC | #3 
On Mon, Oct 7, 2024 at 4:08 PM Simon Horman <horms@kernel.org> wrote:

> Hi Eric,
>
> A minor nit from my side: Kernel-doc should be updated for this new field.

Oh well, do we still carry kdoc for this gigantic structure ?

So many minor details adding a lot of noise for no benefit :/
diff mbox series

Patch

diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
index 22b07c814f4a4575d255fdf472d07c549536e543..a8e2a7ce0383343464800be8db31aeddd791f086 100644
--- a/Documentation/networking/net_cachelines/net_device.rst
+++ b/Documentation/networking/net_cachelines/net_device.rst
@@ -83,6 +83,7 @@  unsigned_int                        allmulti
 bool                                uc_promisc                                                      
 unsigned_char                       nested_level                                                    
 struct_in_device*                   ip_ptr                  read_mostly         read_mostly         __in_dev_get
+struct hlist_head                   fib_nh_head
 struct_inet6_dev*                   ip6_ptr                 read_mostly         read_mostly         __in6_dev_get
 struct_vlan_info*                   vlan_info                                                       
 struct_dsa_port*                    dsa_ptr                                                         
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4d20c776a4ff3d0e881b8d9b99901edb35f66da2..cda20a3fe1adf54c1e6df5b5a8882ef7830e1b46 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2209,6 +2209,8 @@  struct net_device {
 
 	/* Protocol-specific pointers */
 	struct in_device __rcu	*ip_ptr;
+	struct hlist_head	fib_nh_head;
+
 #if IS_ENABLED(CONFIG_VLAN_8021Q)
 	struct vlan_info __rcu	*vlan_info;
 #endif
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index ece779bfb8f6bec67eb7751761df9a4f158020a8..d2cee5c314f5e76530ac564f49b433822bb0a272 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -56,10 +56,6 @@  static unsigned int fib_info_hash_size;
 static unsigned int fib_info_hash_bits;
 static unsigned int fib_info_cnt;
 
-#define DEVINDEX_HASHBITS 8
-#define DEVINDEX_HASHSIZE (1U << DEVINDEX_HASHBITS)
-static struct hlist_head fib_info_devhash[DEVINDEX_HASHSIZE];
-
 /* for_nexthops and change_nexthops only used when nexthop object
  * is not set in a fib_info. The logic within can reference fib_nh.
  */
@@ -319,12 +315,9 @@  static inline int nh_comp(struct fib_info *fi, struct fib_info *ofi)
 	return 0;
 }
 
-static struct hlist_head *
-fib_info_devhash_bucket(const struct net_device *dev)
+static struct hlist_head *fib_nh_head(struct net_device *dev)
 {
-	u32 val = net_hash_mix(dev_net(dev)) ^ dev->ifindex;
-
-	return &fib_info_devhash[hash_32(val, DEVINDEX_HASHBITS)];
+	return &dev->fib_nh_head;
 }
 
 static unsigned int fib_info_hashfn_1(int init_val, u8 protocol, u8 scope,
@@ -435,11 +428,11 @@  int ip_fib_check_default(__be32 gw, struct net_device *dev)
 	struct hlist_head *head;
 	struct fib_nh *nh;
 
-	head = fib_info_devhash_bucket(dev);
+	head = fib_nh_head(dev);
 
 	hlist_for_each_entry_rcu(nh, head, nh_hash) {
-		if (nh->fib_nh_dev == dev &&
-		    nh->fib_nh_gw4 == gw &&
+		DEBUG_NET_WARN_ON_ONCE(nh->fib_nh_dev != dev);
+		if (nh->fib_nh_gw4 == gw &&
 		    !(nh->fib_nh_flags & RTNH_F_DEAD)) {
 			return 0;
 		}
@@ -1595,7 +1588,7 @@  struct fib_info *fib_create_info(struct fib_config *cfg,
 
 			if (!nexthop_nh->fib_nh_dev)
 				continue;
-			head = fib_info_devhash_bucket(nexthop_nh->fib_nh_dev);
+			head = fib_nh_head(nexthop_nh->fib_nh_dev);
 			hlist_add_head_rcu(&nexthop_nh->nh_hash, head);
 		} endfor_nexthops(fi)
 	}
@@ -1948,12 +1941,12 @@  void fib_nhc_update_mtu(struct fib_nh_common *nhc, u32 new, u32 orig)
 
 void fib_sync_mtu(struct net_device *dev, u32 orig_mtu)
 {
-	struct hlist_head *head = fib_info_devhash_bucket(dev);
+	struct hlist_head *head = fib_nh_head(dev);
 	struct fib_nh *nh;
 
 	hlist_for_each_entry(nh, head, nh_hash) {
-		if (nh->fib_nh_dev == dev)
-			fib_nhc_update_mtu(&nh->nh_common, dev->mtu, orig_mtu);
+		DEBUG_NET_WARN_ON_ONCE(nh->fib_nh_dev != dev);
+		fib_nhc_update_mtu(&nh->nh_common, dev->mtu, orig_mtu);
 	}
 }
 
@@ -1967,7 +1960,7 @@  void fib_sync_mtu(struct net_device *dev, u32 orig_mtu)
  */
 int fib_sync_down_dev(struct net_device *dev, unsigned long event, bool force)
 {
-	struct hlist_head *head = fib_info_devhash_bucket(dev);
+	struct hlist_head *head = fib_nh_head(dev);
 	struct fib_info *prev_fi = NULL;
 	int scope = RT_SCOPE_NOWHERE;
 	struct fib_nh *nh;
@@ -1981,7 +1974,8 @@  int fib_sync_down_dev(struct net_device *dev, unsigned long event, bool force)
 		int dead;
 
 		BUG_ON(!fi->fib_nhs);
-		if (nh->fib_nh_dev != dev || fi == prev_fi)
+		DEBUG_NET_WARN_ON_ONCE(nh->fib_nh_dev != dev);
+		if (fi == prev_fi)
 			continue;
 		prev_fi = fi;
 		dead = 0;
@@ -2131,7 +2125,7 @@  int fib_sync_up(struct net_device *dev, unsigned char nh_flags)
 	}
 
 	prev_fi = NULL;
-	head = fib_info_devhash_bucket(dev);
+	head = fib_nh_head(dev);
 	ret = 0;
 
 	hlist_for_each_entry(nh, head, nh_hash) {
@@ -2139,7 +2133,8 @@  int fib_sync_up(struct net_device *dev, unsigned char nh_flags)
 		int alive;
 
 		BUG_ON(!fi->fib_nhs);
-		if (nh->fib_nh_dev != dev || fi == prev_fi)
+		DEBUG_NET_WARN_ON_ONCE(nh->fib_nh_dev != dev);
+		if (fi == prev_fi)
 			continue;
 
 		prev_fi = fi;