Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
From: Eric Dumazet <hidden>
Date: 2015-05-29 01:50:37
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Fri, 2015-05-29 at 09:21 +0800, Ying Xue wrote:
On 05/28/2015 06:13 PM, Eric Dumazet wrote:quoted
This patch is not needed. You really should read Documentation/RCU , because it looks like you are quite confused. When we remove an element from a RCU protected list, all the objects in the chain are already ready to be caught by rcu readers. Therefore, no additional memory barrier is needed before doing *np = n->next; Please do not add spurious memory barriers. Like atomic operations, we want all of them being required and possibly documented.Yes, you are right, thanks for your clear explanation :) However, there are still three places where we use rcu_assign_pointer() to remove a neigh entry from a RCU-protected list, and the three places are neigh_forced_gc(), neigh_flush_dev(), and __neigh_for_each_release() respectively. This means it's redundant for us to use rcu_assign_pointer() in the three places, right?
I count 5 places of redundancy.
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3a74df750af4044eba0e7d88ae01ca9b4dac0e72..ac3b69183cc982e722d9683d6de7a39f66b50b64 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c@@ -141,9 +141,7 @@ static int neigh_forced_gc(struct neigh_table *tbl) write_lock(&n->lock); if (atomic_read(&n->refcnt) == 1 && !(n->nud_state & NUD_PERMANENT)) { - rcu_assign_pointer(*np, - rcu_dereference_protected(n->next, - lockdep_is_held(&tbl->lock))); + *np = n->next; n->dead = 1; shrunk = 1; write_unlock(&n->lock);
@@ -210,9 +208,7 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev) np = &n->next; continue; } - rcu_assign_pointer(*np, - rcu_dereference_protected(n->next, - lockdep_is_held(&tbl->lock))); + *np = n->next; write_lock(&n->lock); neigh_del_timer(n); n->dead = 1;
@@ -380,10 +376,8 @@ static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl, 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))); + n->next = new_nht->hash_buckets[hash]; + rcu_assign_pointer(new_nht->hash_buckets[hash], n); } }
@@ -515,9 +509,7 @@ struct neighbour *__neigh_create(struct neigh_table *tbl, const void *pkey, n->dead = 0; if (want_ref) neigh_hold(n); - rcu_assign_pointer(n->next, - rcu_dereference_protected(nht->hash_buckets[hash_val], - lockdep_is_held(&tbl->lock))); + n->next = nht->hash_buckets[hash_val]; rcu_assign_pointer(nht->hash_buckets[hash_val], n); write_unlock_bh(&tbl->lock); neigh_dbg(2, "neigh %p is created\n", n);
@@ -2381,9 +2373,7 @@ void __neigh_for_each_release(struct neigh_table *tbl, write_lock(&n->lock); release = cb(n); if (release) { - rcu_assign_pointer(*np, - rcu_dereference_protected(n->next, - lockdep_is_held(&tbl->lock))); + *np = n->next; n->dead = 1; } else np = &n->next;