Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
On 05/28/2015 06:13 PM, Eric Dumazet wrote: 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? Regards, Ying -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
On Fri, 2015-05-29 at 09:21 +0800, Ying Xue wrote:
On 05/28/2015 06:13 PM, Eric Dumazet wrote:
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;
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] neigh: Add missing rcu_assign_pointer
Commit e4c4e448cf55 (neigh: Convert garbage collection from softirq
to workqueue) misses to use rcu_assign_pointer() macro to assign a
RCU-protected pointer.
Signed-off-by: Ying Xue ying@windriver.com
---
net/core/neighbour.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3a74df7..aaad3a5 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -783,7 +783,8 @@ static void neigh_periodic_work(struct work_struct *work)
if (atomic_read(n-refcnt) == 1
(state == NUD_FAILED ||
time_after(jiffies, n-used + NEIGH_VAR(n-parms,
GC_STALETIME {
- *np = n-next;
+ rcu_assign_pointer(*np,
rcu_dereference_protected(n-next,
+
lockdep_is_held(tbl-lock)));
n-dead = 1;
write_unlock(n-lock);
neigh_cleanup_and_release(n);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
On Thu, 2015-05-28 at 16:28 +0800, Ying Xue wrote:
Commit e4c4e448cf55 (neigh: Convert garbage collection from softirq
to workqueue) misses to use rcu_assign_pointer() macro to assign a
RCU-protected pointer.
Signed-off-by: Ying Xue ying@windriver.com
---
net/core/neighbour.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3a74df7..aaad3a5 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -783,7 +783,8 @@ static void neigh_periodic_work(struct work_struct *work)
if (atomic_read(n-refcnt) == 1
(state == NUD_FAILED ||
time_after(jiffies, n-used + NEIGH_VAR(n-parms,
GC_STALETIME {
- *np = n-next;
+ rcu_assign_pointer(*np,
rcu_dereference_protected(n-next,
+
lockdep_is_held(tbl-lock)));
n-dead = 1;
write_unlock(n-lock);
neigh_cleanup_and_release(n);
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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
On Thu, 2015-05-28 at 21:50 +0800, Herbert Xu wrote: This patch is indeed bogus but accessing an RCU-protected like this will trigger sparse warnings. So better make it an RCU_INIT_POINTER. A = B; is perfectly fine since both A and B have the same __rcu attribute. Sparse has no warning and should not. root@edumazet-glaptop2:/usr/src/net# grep CONFIG_SPARSE_RCU_POINTER .config CONFIG_SPARSE_RCU_POINTER=y root@edumazet-glaptop2:/usr/src/net# make C=2 CF=-D__CHECK_ENDIAN__ net/core/neighbour.o ... CHECK net/core/neighbour.c -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
Eric Dumazet eric.duma...@gmail.com wrote: 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. This patch is indeed bogus but accessing an RCU-protected like this will trigger sparse warnings. So better make it an RCU_INIT_POINTER. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
