* [PATCH net-next] neigh: Add missing rcu_assign_pointer
@ 2015-05-28 8:28 Ying Xue
2015-05-28 10:13 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Ying Xue @ 2015-05-28 8:28 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet
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.xue@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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-28 8:28 [PATCH net-next] neigh: Add missing rcu_assign_pointer Ying Xue
@ 2015-05-28 10:13 ` Eric Dumazet
2015-05-28 13:50 ` Herbert Xu
2015-05-29 1:21 ` Ying Xue
0 siblings, 2 replies; 8+ messages in thread
From: Eric Dumazet @ 2015-05-28 10:13 UTC (permalink / raw)
To: Ying Xue; +Cc: netdev
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.xue@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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-28 10:13 ` Eric Dumazet
@ 2015-05-28 13:50 ` Herbert Xu
2015-05-28 14:38 ` Eric Dumazet
2015-05-29 1:21 ` Ying Xue
1 sibling, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2015-05-28 13:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: ying.xue, netdev
Eric Dumazet <eric.dumazet@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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-28 13:50 ` Herbert Xu
@ 2015-05-28 14:38 ` Eric Dumazet
2015-05-29 0:24 ` Herbert Xu
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2015-05-28 14:38 UTC (permalink / raw)
To: Herbert Xu; +Cc: ying.xue, netdev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-28 14:38 ` Eric Dumazet
@ 2015-05-29 0:24 ` Herbert Xu
0 siblings, 0 replies; 8+ messages in thread
From: Herbert Xu @ 2015-05-29 0:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: ying.xue, netdev
On Thu, May 28, 2015 at 07:38:21AM -0700, Eric Dumazet wrote:
>
> 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
Indeed. Thanks for pointing this out.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-28 10:13 ` Eric Dumazet
2015-05-28 13:50 ` Herbert Xu
@ 2015-05-29 1:21 ` Ying Xue
2015-05-29 1:50 ` Eric Dumazet
1 sibling, 1 reply; 8+ messages in thread
From: Ying Xue @ 2015-05-29 1:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, herbert
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-29 1:21 ` Ying Xue
@ 2015-05-29 1:50 ` Eric Dumazet
2015-05-29 2:04 ` Ying Xue
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2015-05-29 1:50 UTC (permalink / raw)
To: Ying Xue; +Cc: netdev, herbert
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;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] neigh: Add missing rcu_assign_pointer
2015-05-29 1:50 ` Eric Dumazet
@ 2015-05-29 2:04 ` Ying Xue
0 siblings, 0 replies; 8+ messages in thread
From: Ying Xue @ 2015-05-29 2:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, herbert
On 05/29/2015 09:50 AM, Eric Dumazet wrote:
> I count 5 places of redundancy.
>
Another two places you found are necessary indeed!
Acked-by: Ying Xue <ying.xue@windriver.com>
> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-29 2:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-28 8:28 [PATCH net-next] neigh: Add missing rcu_assign_pointer Ying Xue
2015-05-28 10:13 ` Eric Dumazet
2015-05-28 13:50 ` Herbert Xu
2015-05-28 14:38 ` Eric Dumazet
2015-05-29 0:24 ` Herbert Xu
2015-05-29 1:21 ` Ying Xue
2015-05-29 1:50 ` Eric Dumazet
2015-05-29 2:04 ` Ying Xue
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.