diff mbox series

[net-next,v3,1/2] Convert neighbour-table to use hlist

Message ID 20241010120139.2856603-2-gnaaman@drivenets.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Improve neigh_flush_dev performance | 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/apply fail Patch patch 2 does not apply to net-next-0
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 19 this patch: 19
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 39 this patch: 39
netdev/verify_signedoff success Signed-off-by tag matches author and committer

Commit Message

Gilad Naaman Oct. 10, 2024, 12:01 p.m. UTC 
Use doubly-linked instead of singly-linked list when linking neighbours,
so that it is possible to remove neighbours without traversing the
entire table.

Signed-off-by: Gilad Naaman <gnaaman@drivenets.com>
---
 include/net/neighbour.h |   8 +-
 net/core/neighbour.c    | 162 ++++++++++++++++------------------------
 2 files changed, 68 insertions(+), 102 deletions(-)

Comments

Kuniyuki Iwashima Oct. 11, 2024, 1:16 a.m. UTC | #1 
From: Gilad Naaman <gnaaman@drivenets.com>
Date: Thu, 10 Oct 2024 12:01:24 +0000
> @@ -304,9 +304,7 @@ static inline struct neighbour *___neigh_lookup_noref(
>  	u32 hash_val;
>  
>  	hash_val = hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
> -	for (n = rcu_dereference(nht->hash_buckets[hash_val]);
> -	     n != NULL;
> -	     n = rcu_dereference(n->next)) {
> +	hlist_for_each_entry_rcu(n, &nht->hash_buckets[hash_val], list) {

Let's move macros in .h and use it here.


>  		if (n->dev == dev && key_eq(n, pkey))
>  			return n;
>  	}
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 77b819cd995b..bf7f69b585d6 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -37,6 +37,7 @@
>  #include <linux/string.h>
>  #include <linux/log2.h>
>  #include <linux/inetdevice.h>
> +#include <linux/rculist.h>
>  #include <net/addrconf.h>
>  
>  #include <trace/events/neigh.h>
> @@ -57,6 +58,26 @@ static void neigh_update_notify(struct neighbour *neigh, u32 nlmsg_pid);
>  static int pneigh_ifdown_and_unlock(struct neigh_table *tbl,
>  				    struct net_device *dev);
>  
> +#define neigh_hlist_entry(n) hlist_entry_safe(n, struct neighbour, list)
> +
> +#define neigh_for_each_rcu(pos, head, cond...) \

No cond here, and use this only under RCU.

For places under neigh table lock, let's define neigh_for_each()
with hlist_for_each_entry().

The current neigh_for_each() is only used in spectrum_router.c
and can be moved to mlxsw_sp_neigh_rif_made_sync_each() with
the new macro used.

Let's split this patch for ease of review.

  1. Add hlist_node and link/unlink by hlist_add() / hlist_del()
  2. Define neigh_for_each() macro and move the current
     neigh_for_each() to mlxsw_sp_neigh_rif_made_sync_each()
  3. Rewrite the seq_file part with macro
  4. Convert the rest of while()/for() with macro
  5. Remove ->next


> +	hlist_for_each_entry_rcu(pos, head, list, ##cond)
> +
> +#define neigh_for_each_safe_rcu_protected(pos, n, head, c)		\
> +	for (pos = neigh_first_rcu_protected(head, c);			\
> +	     pos && ({ n = neigh_next_rcu_protected(pos, c); 1; });	\
> +	     pos = n)

This should be hlist_for_each_entry_safe().

There is a reason why rculist.h does not have this version.
_safe means you are on the write side, which does not need RCU.


> +
> +#define neigh_first_rcu(bucket) \
> +	neigh_hlist_entry(rcu_dereference(hlist_first_rcu(bucket)))
> +#define neigh_next_rcu(n) \
> +	neigh_hlist_entry(rcu_dereference(hlist_next_rcu(&(n)->list)))
> +
> +#define neigh_first_rcu_protected(head, c) \
> +	neigh_hlist_entry(rcu_dereference_protected(hlist_first_rcu(head), c))
> +#define neigh_next_rcu_protected(n, c) \
> +	neigh_hlist_entry(rcu_dereference_protected(hlist_next_rcu(&(n)->list), c))
> +
>  #ifdef CONFIG_PROC_FS
>  static const struct seq_operations neigh_stat_seq_ops;
>  #endif
> @@ -205,18 +226,13 @@ static void neigh_update_flags(struct neighbour *neigh, u32 flags, int *notify,
>  	}
>  }
>  
> -static bool neigh_del(struct neighbour *n, struct neighbour __rcu **np,
> -		      struct neigh_table *tbl)
> +static bool neigh_del(struct neighbour *n, struct neigh_table *tbl)

Now this can be renamed to neigh_remove_one() without static.


>  {
>  	bool retval = false;
>  
>  	write_lock(&n->lock);
>  	if (refcount_read(&n->refcnt) == 1) {
> -		struct neighbour *neigh;
> -
> -		neigh = rcu_dereference_protected(n->next,
> -						  lockdep_is_held(&tbl->lock));
> -		rcu_assign_pointer(*np, neigh);
> +		hlist_del_rcu(&n->list);
>  		neigh_mark_dead(n);
>  		retval = true;
>  	}
> @@ -228,25 +244,7 @@ static bool neigh_del(struct neighbour *n, struct neighbour __rcu **np,
>  
>  bool neigh_remove_one(struct neighbour *ndel, struct neigh_table *tbl)
>  {
> -	struct neigh_hash_table *nht;
> -	void *pkey = ndel->primary_key;
> -	u32 hash_val;
> -	struct neighbour *n;
> -	struct neighbour __rcu **np;
> -
> -	nht = rcu_dereference_protected(tbl->nht,
> -					lockdep_is_held(&tbl->lock));
> -	hash_val = tbl->hash(pkey, ndel->dev, nht->hash_rnd);
> -	hash_val = hash_val >> (32 - nht->hash_shift);
> -
> -	np = &nht->hash_buckets[hash_val];
> -	while ((n = rcu_dereference_protected(*np,
> -					      lockdep_is_held(&tbl->lock)))) {
> -		if (n == ndel)
> -			return neigh_del(n, np, tbl);
> -		np = &n->next;
> -	}
> -	return false;
> +	return neigh_del(ndel, tbl);
>  }
>  
>  static int neigh_forced_gc(struct neigh_table *tbl)
> @@ -387,22 +385,18 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
>  					lockdep_is_held(&tbl->lock));
>  
>  	for (i = 0; i < (1 << nht->hash_shift); i++) {
> -		struct neighbour *n;
> -		struct neighbour __rcu **np = &nht->hash_buckets[i];
> +		struct neighbour *n, *next;
>  
> -		while ((n = rcu_dereference_protected(*np,
> -					lockdep_is_held(&tbl->lock))) != NULL) {
> +		neigh_for_each_safe_rcu_protected(n, next,
> +						  &nht->hash_buckets[i],
> +						  lockdep_is_held(&tbl->lock)) {

tbl->nht is already fetched with lockdep_is_held(),
soneigh_for_each_safe() is enough.


>  			if (dev && n->dev != dev) {
> -				np = &n->next;
>  				continue;
>  			}

{} is no longer needed.


>  			if (skip_perm && n->nud_state & NUD_PERMANENT) {
> -				np = &n->next;
>  				continue;
>  			}

Same here.


[...]
> @@ -591,7 +585,7 @@ static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
>  	for (i = 0; i < (1 << old_nht->hash_shift); i++) {
>  		struct neighbour *n, *next;
>  
> -		for (n = rcu_dereference_protected(old_nht->hash_buckets[i],
> +		for (n = neigh_first_rcu_protected(&old_nht->hash_buckets[i],
>  						   lockdep_is_held(&tbl->lock));

_safe version can be used,


>  		     n != NULL;
>  		     n = next) {
> @@ -599,14 +593,9 @@ static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
>  					 new_nht->hash_rnd);
>  
>  			hash >>= (32 - new_nht->hash_shift);
> -			next = rcu_dereference_protected(n->next,
> -						lockdep_is_held(&tbl->lock));
> -
> -			rcu_assign_pointer(n->next,
> -					   rcu_dereference_protected(
> -						new_nht->hash_buckets[hash],
> -						lockdep_is_held(&tbl->lock)));
> -			rcu_assign_pointer(new_nht->hash_buckets[hash], n);
> +			next = neigh_next_rcu_protected(n, lockdep_is_held(&tbl->lock));

then, this will be unnecessary.


> +			hlist_del_rcu(&n->list);
> +			hlist_add_head_rcu(&n->list, &new_nht->hash_buckets[hash]);
>  		}
>  	}
>  
> @@ -693,11 +682,9 @@ ___neigh_create(struct neigh_table *tbl, const void *pkey,
>  		goto out_tbl_unlock;
>  	}
>  
> -	for (n1 = rcu_dereference_protected(nht->hash_buckets[hash_val],
> -					    lockdep_is_held(&tbl->lock));
> -	     n1 != NULL;
> -	     n1 = rcu_dereference_protected(n1->next,
> -			lockdep_is_held(&tbl->lock))) {
> +	neigh_for_each_rcu(n1,
> +			   &nht->hash_buckets[hash_val],
> +			   lockdep_is_held(&tbl->lock)) {

lockdep_is_held() is used above.

Let's use neigh_for_each().


[...]
> @@ -3242,7 +3208,8 @@ static struct neighbour *neigh_get_next(struct seq_file *seq,
>  		if (v)
>  			return n;
>  	}
> -	n = rcu_dereference(n->next);
> +
> +	n = neigh_next_rcu(n);

I guess neigh_get_next() can be simplified with
hlist_for_each_entry_continue_rcu() ?

Then, neigh_first_rcu() and neigh_next_rcu() would be unnecessary ?

>  
>  	while (1) {
>  		while (n) {
> @@ -3260,7 +3227,8 @@ static struct neighbour *neigh_get_next(struct seq_file *seq,
>  			if (READ_ONCE(n->nud_state) & ~NUD_NOARP)
>  				break;
>  next:
> -			n = rcu_dereference(n->next);
> +
> +			n = neigh_next_rcu(n);
>  		}
>  
>  		if (n)
> @@ -3269,7 +3237,7 @@ static struct neighbour *neigh_get_next(struct seq_file *seq,
>  		if (++state->bucket >= (1 << nht->hash_shift))
>  			break;
>  
> -		n = rcu_dereference(nht->hash_buckets[state->bucket]);
> +		n = neigh_first_rcu(&nht->hash_buckets[state->bucket]);
>  	}
>  
>  	if (n && pos)
> -- 
> 2.46.0
>
Gilad Naaman Oct. 13, 2024, 10:45 a.m. UTC | #2 
> The current neigh_for_each() is only used in spectrum_router.c
> and can be moved to mlxsw_sp_neigh_rif_made_sync_each() with
> the new macro used.

Oh, I completely missed that it exists, sorry.

> Let's split this patch for ease of review.
> 
>   1. Add hlist_node and link/unlink by hlist_add() / hlist_del()
>   2. Define neigh_for_each() macro and move the current
>      neigh_for_each() to mlxsw_sp_neigh_rif_made_sync_each()
>   3. Rewrite the seq_file part with macro
>   4. Convert the rest of while()/for() with macro
>   5. Remove ->next

Just making sure, you mean that in (1.) I should add hlist_node *alongside*
the current `struct neighbour __rcu *next`, and only remove this duplication in
(5.)?
Kuniyuki Iwashima Oct. 14, 2024, 10:43 p.m. UTC | #3 
From: Gilad Naaman <gnaaman@drivenets.com>
Date: Sun, 13 Oct 2024 10:45:21 +0000
> > The current neigh_for_each() is only used in spectrum_router.c
> > and can be moved to mlxsw_sp_neigh_rif_made_sync_each() with
> > the new macro used.
> 
> Oh, I completely missed that it exists, sorry.
> 
> > Let's split this patch for ease of review.
> > 
> >   1. Add hlist_node and link/unlink by hlist_add() / hlist_del()
> >   2. Define neigh_for_each() macro and move the current
> >      neigh_for_each() to mlxsw_sp_neigh_rif_made_sync_each()
> >   3. Rewrite the seq_file part with macro
> >   4. Convert the rest of while()/for() with macro
> >   5. Remove ->next
> 
> Just making sure, you mean that in (1.) I should add hlist_node *alongside*
> the current `struct neighbour __rcu *next`, and only remove this duplication in
> (5.)?

Right, it will allow part-by-part changes that are easier to review.
diff mbox series

Patch

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index a44f262a7384..93903f9854f9 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -135,7 +135,7 @@  struct neigh_statistics {
 #define NEIGH_CACHE_STAT_INC(tbl, field) this_cpu_inc((tbl)->stats->field)
 
 struct neighbour {
-	struct neighbour __rcu	*next;
+	struct hlist_node	list;
 	struct neigh_table	*tbl;
 	struct neigh_parms	*parms;
 	unsigned long		confirmed;
@@ -190,7 +190,7 @@  struct pneigh_entry {
 #define NEIGH_NUM_HASH_RND	4
 
 struct neigh_hash_table {
-	struct neighbour __rcu	**hash_buckets;
+	struct hlist_head	*hash_buckets;
 	unsigned int		hash_shift;
 	__u32			hash_rnd[NEIGH_NUM_HASH_RND];
 	struct rcu_head		rcu;
@@ -304,9 +304,7 @@  static inline struct neighbour *___neigh_lookup_noref(
 	u32 hash_val;
 
 	hash_val = hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
-	for (n = rcu_dereference(nht->hash_buckets[hash_val]);
-	     n != NULL;
-	     n = rcu_dereference(n->next)) {
+	hlist_for_each_entry_rcu(n, &nht->hash_buckets[hash_val], list) {
 		if (n->dev == dev && key_eq(n, pkey))
 			return n;
 	}
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 77b819cd995b..bf7f69b585d6 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -37,6 +37,7 @@ 
 #include <linux/string.h>
 #include <linux/log2.h>
 #include <linux/inetdevice.h>
+#include <linux/rculist.h>
 #include <net/addrconf.h>
 
 #include <trace/events/neigh.h>
@@ -57,6 +58,26 @@  static void neigh_update_notify(struct neighbour *neigh, u32 nlmsg_pid);
 static int pneigh_ifdown_and_unlock(struct neigh_table *tbl,
 				    struct net_device *dev);
 
+#define neigh_hlist_entry(n) hlist_entry_safe(n, struct neighbour, list)
+
+#define neigh_for_each_rcu(pos, head, cond...) \
+	hlist_for_each_entry_rcu(pos, head, list, ##cond)
+
+#define neigh_for_each_safe_rcu_protected(pos, n, head, c)		\
+	for (pos = neigh_first_rcu_protected(head, c);			\
+	     pos && ({ n = neigh_next_rcu_protected(pos, c); 1; });	\
+	     pos = n)
+
+#define neigh_first_rcu(bucket) \
+	neigh_hlist_entry(rcu_dereference(hlist_first_rcu(bucket)))
+#define neigh_next_rcu(n) \
+	neigh_hlist_entry(rcu_dereference(hlist_next_rcu(&(n)->list)))
+
+#define neigh_first_rcu_protected(head, c) \
+	neigh_hlist_entry(rcu_dereference_protected(hlist_first_rcu(head), c))
+#define neigh_next_rcu_protected(n, c) \
+	neigh_hlist_entry(rcu_dereference_protected(hlist_next_rcu(&(n)->list), c))
+
 #ifdef CONFIG_PROC_FS
 static const struct seq_operations neigh_stat_seq_ops;
 #endif
@@ -205,18 +226,13 @@  static void neigh_update_flags(struct neighbour *neigh, u32 flags, int *notify,
 	}
 }
 
-static bool neigh_del(struct neighbour *n, struct neighbour __rcu **np,
-		      struct neigh_table *tbl)
+static bool neigh_del(struct neighbour *n, struct neigh_table *tbl)
 {
 	bool retval = false;
 
 	write_lock(&n->lock);
 	if (refcount_read(&n->refcnt) == 1) {
-		struct neighbour *neigh;
-
-		neigh = rcu_dereference_protected(n->next,
-						  lockdep_is_held(&tbl->lock));
-		rcu_assign_pointer(*np, neigh);
+		hlist_del_rcu(&n->list);
 		neigh_mark_dead(n);
 		retval = true;
 	}
@@ -228,25 +244,7 @@  static bool neigh_del(struct neighbour *n, struct neighbour __rcu **np,
 
 bool neigh_remove_one(struct neighbour *ndel, struct neigh_table *tbl)
 {
-	struct neigh_hash_table *nht;
-	void *pkey = ndel->primary_key;
-	u32 hash_val;
-	struct neighbour *n;
-	struct neighbour __rcu **np;
-
-	nht = rcu_dereference_protected(tbl->nht,
-					lockdep_is_held(&tbl->lock));
-	hash_val = tbl->hash(pkey, ndel->dev, nht->hash_rnd);
-	hash_val = hash_val >> (32 - nht->hash_shift);
-
-	np = &nht->hash_buckets[hash_val];
-	while ((n = rcu_dereference_protected(*np,
-					      lockdep_is_held(&tbl->lock)))) {
-		if (n == ndel)
-			return neigh_del(n, np, tbl);
-		np = &n->next;
-	}
-	return false;
+	return neigh_del(ndel, tbl);
 }
 
 static int neigh_forced_gc(struct neigh_table *tbl)
@@ -387,22 +385,18 @@  static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
 					lockdep_is_held(&tbl->lock));
 
 	for (i = 0; i < (1 << nht->hash_shift); i++) {
-		struct neighbour *n;
-		struct neighbour __rcu **np = &nht->hash_buckets[i];
+		struct neighbour *n, *next;
 
-		while ((n = rcu_dereference_protected(*np,
-					lockdep_is_held(&tbl->lock))) != NULL) {
+		neigh_for_each_safe_rcu_protected(n, next,
+						  &nht->hash_buckets[i],
+						  lockdep_is_held(&tbl->lock)) {
 			if (dev && n->dev != dev) {
-				np = &n->next;
 				continue;
 			}
 			if (skip_perm && n->nud_state & NUD_PERMANENT) {
-				np = &n->next;
 				continue;
 			}
-			rcu_assign_pointer(*np,
-				   rcu_dereference_protected(n->next,
-						lockdep_is_held(&tbl->lock)));
+			hlist_del_rcu(&n->list);
 			write_lock(&n->lock);
 			neigh_del_timer(n);
 			neigh_mark_dead(n);
@@ -530,9 +524,9 @@  static void neigh_get_hash_rnd(u32 *x)
 
 static struct neigh_hash_table *neigh_hash_alloc(unsigned int shift)
 {
-	size_t size = (1 << shift) * sizeof(struct neighbour *);
+	size_t size = (1 << shift) * sizeof(struct hlist_head);
 	struct neigh_hash_table *ret;
-	struct neighbour __rcu **buckets;
+	struct hlist_head *buckets;
 	int i;
 
 	ret = kmalloc(sizeof(*ret), GFP_ATOMIC);
@@ -541,7 +535,7 @@  static struct neigh_hash_table *neigh_hash_alloc(unsigned int shift)
 	if (size <= PAGE_SIZE) {
 		buckets = kzalloc(size, GFP_ATOMIC);
 	} else {
-		buckets = (struct neighbour __rcu **)
+		buckets = (struct hlist_head *)
 			  __get_free_pages(GFP_ATOMIC | __GFP_ZERO,
 					   get_order(size));
 		kmemleak_alloc(buckets, size, 1, GFP_ATOMIC);
@@ -562,8 +556,8 @@  static void neigh_hash_free_rcu(struct rcu_head *head)
 	struct neigh_hash_table *nht = container_of(head,
 						    struct neigh_hash_table,
 						    rcu);
-	size_t size = (1 << nht->hash_shift) * sizeof(struct neighbour *);
-	struct neighbour __rcu **buckets = nht->hash_buckets;
+	size_t size = (1 << nht->hash_shift) * sizeof(struct hlist_head);
+	struct hlist_head *buckets = nht->hash_buckets;
 
 	if (size <= PAGE_SIZE) {
 		kfree(buckets);
@@ -591,7 +585,7 @@  static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
 	for (i = 0; i < (1 << old_nht->hash_shift); i++) {
 		struct neighbour *n, *next;
 
-		for (n = rcu_dereference_protected(old_nht->hash_buckets[i],
+		for (n = neigh_first_rcu_protected(&old_nht->hash_buckets[i],
 						   lockdep_is_held(&tbl->lock));
 		     n != NULL;
 		     n = next) {
@@ -599,14 +593,9 @@  static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
 					 new_nht->hash_rnd);
 
 			hash >>= (32 - new_nht->hash_shift);
-			next = rcu_dereference_protected(n->next,
-						lockdep_is_held(&tbl->lock));
-
-			rcu_assign_pointer(n->next,
-					   rcu_dereference_protected(
-						new_nht->hash_buckets[hash],
-						lockdep_is_held(&tbl->lock)));
-			rcu_assign_pointer(new_nht->hash_buckets[hash], n);
+			next = neigh_next_rcu_protected(n, lockdep_is_held(&tbl->lock));
+			hlist_del_rcu(&n->list);
+			hlist_add_head_rcu(&n->list, &new_nht->hash_buckets[hash]);
 		}
 	}
 
@@ -693,11 +682,9 @@  ___neigh_create(struct neigh_table *tbl, const void *pkey,
 		goto out_tbl_unlock;
 	}
 
-	for (n1 = rcu_dereference_protected(nht->hash_buckets[hash_val],
-					    lockdep_is_held(&tbl->lock));
-	     n1 != NULL;
-	     n1 = rcu_dereference_protected(n1->next,
-			lockdep_is_held(&tbl->lock))) {
+	neigh_for_each_rcu(n1,
+			   &nht->hash_buckets[hash_val],
+			   lockdep_is_held(&tbl->lock)) {
 		if (dev == n1->dev && !memcmp(n1->primary_key, n->primary_key, key_len)) {
 			if (want_ref)
 				neigh_hold(n1);
@@ -713,10 +700,7 @@  ___neigh_create(struct neigh_table *tbl, const void *pkey,
 		list_add_tail(&n->managed_list, &n->tbl->managed_list);
 	if (want_ref)
 		neigh_hold(n);
-	rcu_assign_pointer(n->next,
-			   rcu_dereference_protected(nht->hash_buckets[hash_val],
-						     lockdep_is_held(&tbl->lock)));
-	rcu_assign_pointer(nht->hash_buckets[hash_val], n);
+	hlist_add_head_rcu(&n->list, &nht->hash_buckets[hash_val]);
 	write_unlock_bh(&tbl->lock);
 	neigh_dbg(2, "neigh %p is created\n", n);
 	rc = n;
@@ -948,8 +932,7 @@  static void neigh_connect(struct neighbour *neigh)
 static void neigh_periodic_work(struct work_struct *work)
 {
 	struct neigh_table *tbl = container_of(work, struct neigh_table, gc_work.work);
-	struct neighbour *n;
-	struct neighbour __rcu **np;
+	struct neighbour *n, *next;
 	unsigned int i;
 	struct neigh_hash_table *nht;
 
@@ -976,10 +959,9 @@  static void neigh_periodic_work(struct work_struct *work)
 		goto out;
 
 	for (i = 0 ; i < (1 << nht->hash_shift); i++) {
-		np = &nht->hash_buckets[i];
-
-		while ((n = rcu_dereference_protected(*np,
-				lockdep_is_held(&tbl->lock))) != NULL) {
+		neigh_for_each_safe_rcu_protected(n, next,
+						  &nht->hash_buckets[i],
+						  lockdep_is_held(&tbl->lock)) {
 			unsigned int state;
 
 			write_lock(&n->lock);
@@ -988,7 +970,7 @@  static void neigh_periodic_work(struct work_struct *work)
 			if ((state & (NUD_PERMANENT | NUD_IN_TIMER)) ||
 			    (n->flags & NTF_EXT_LEARNED)) {
 				write_unlock(&n->lock);
-				goto next_elt;
+				continue;
 			}
 
 			if (time_before(n->used, n->confirmed) &&
@@ -999,18 +981,13 @@  static void neigh_periodic_work(struct work_struct *work)
 			    (state == NUD_FAILED ||
 			     !time_in_range_open(jiffies, n->used,
 						 n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {
-				rcu_assign_pointer(*np,
-					rcu_dereference_protected(n->next,
-						lockdep_is_held(&tbl->lock)));
+				hlist_del_rcu(&n->list);
 				neigh_mark_dead(n);
 				write_unlock(&n->lock);
 				neigh_cleanup_and_release(n);
 				continue;
 			}
 			write_unlock(&n->lock);
-
-next_elt:
-			np = &n->next;
 		}
 		/*
 		 * It's fine to release lock here, even if hash table
@@ -2728,9 +2705,8 @@  static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
 	for (h = s_h; h < (1 << nht->hash_shift); h++) {
 		if (h > s_h)
 			s_idx = 0;
-		for (n = rcu_dereference(nht->hash_buckets[h]), idx = 0;
-		     n != NULL;
-		     n = rcu_dereference(n->next)) {
+		idx = 0;
+		neigh_for_each_rcu(n, &nht->hash_buckets[h]) {
 			if (idx < s_idx || !net_eq(dev_net(n->dev), net))
 				goto next;
 			if (neigh_ifindex_filtered(n->dev, filter->dev_idx) ||
@@ -3097,9 +3073,7 @@  void neigh_for_each(struct neigh_table *tbl, void (*cb)(struct neighbour *, void
 	for (chain = 0; chain < (1 << nht->hash_shift); chain++) {
 		struct neighbour *n;
 
-		for (n = rcu_dereference(nht->hash_buckets[chain]);
-		     n != NULL;
-		     n = rcu_dereference(n->next))
+		neigh_for_each_rcu(n, &nht->hash_buckets[chain])
 			cb(n, cookie);
 	}
 	read_unlock_bh(&tbl->lock);
@@ -3117,23 +3091,19 @@  void __neigh_for_each_release(struct neigh_table *tbl,
 	nht = rcu_dereference_protected(tbl->nht,
 					lockdep_is_held(&tbl->lock));
 	for (chain = 0; chain < (1 << nht->hash_shift); chain++) {
-		struct neighbour *n;
-		struct neighbour __rcu **np;
+		struct neighbour *n, *next;
 
-		np = &nht->hash_buckets[chain];
-		while ((n = rcu_dereference_protected(*np,
-					lockdep_is_held(&tbl->lock))) != NULL) {
+		neigh_for_each_safe_rcu_protected(n, next,
+						  &nht->hash_buckets[chain],
+						  lockdep_is_held(&tbl->lock)) {
 			int release;
 
 			write_lock(&n->lock);
 			release = cb(n);
 			if (release) {
-				rcu_assign_pointer(*np,
-					rcu_dereference_protected(n->next,
-						lockdep_is_held(&tbl->lock)));
+				hlist_del_rcu(&n->list);
 				neigh_mark_dead(n);
-			} else
-				np = &n->next;
+			}
 			write_unlock(&n->lock);
 			if (release)
 				neigh_cleanup_and_release(n);
@@ -3200,25 +3170,21 @@  static struct neighbour *neigh_get_first(struct seq_file *seq)
 
 	state->flags &= ~NEIGH_SEQ_IS_PNEIGH;
 	for (bucket = 0; bucket < (1 << nht->hash_shift); bucket++) {
-		n = rcu_dereference(nht->hash_buckets[bucket]);
-
-		while (n) {
+		neigh_for_each_rcu(n, &nht->hash_buckets[bucket]) {
 			if (!net_eq(dev_net(n->dev), net))
-				goto next;
+				continue;
 			if (state->neigh_sub_iter) {
 				loff_t fakep = 0;
 				void *v;
 
 				v = state->neigh_sub_iter(state, n, &fakep);
 				if (!v)
-					goto next;
+					continue;
 			}
 			if (!(state->flags & NEIGH_SEQ_SKIP_NOARP))
 				break;
 			if (READ_ONCE(n->nud_state) & ~NUD_NOARP)
 				break;
-next:
-			n = rcu_dereference(n->next);
 		}
 
 		if (n)
@@ -3242,7 +3208,8 @@  static struct neighbour *neigh_get_next(struct seq_file *seq,
 		if (v)
 			return n;
 	}
-	n = rcu_dereference(n->next);
+
+	n = neigh_next_rcu(n);
 
 	while (1) {
 		while (n) {
@@ -3260,7 +3227,8 @@  static struct neighbour *neigh_get_next(struct seq_file *seq,
 			if (READ_ONCE(n->nud_state) & ~NUD_NOARP)
 				break;
 next:
-			n = rcu_dereference(n->next);
+
+			n = neigh_next_rcu(n);
 		}
 
 		if (n)
@@ -3269,7 +3237,7 @@  static struct neighbour *neigh_get_next(struct seq_file *seq,
 		if (++state->bucket >= (1 << nht->hash_shift))
 			break;
 
-		n = rcu_dereference(nht->hash_buckets[state->bucket]);
+		n = neigh_first_rcu(&nht->hash_buckets[state->bucket]);
 	}
 
 	if (n && pos)