* [PATCH 0/2] rcu: Fix rculist_nulls and doc
@ 2023-07-06 10:28 Alan Huang
2023-07-06 10:28 ` [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls Alan Huang
2023-07-06 10:28 ` [PATCH 2/2] docs/RCU: Bring back smp_wmb() Alan Huang
0 siblings, 2 replies; 11+ messages in thread
From: Alan Huang @ 2023-07-06 10:28 UTC (permalink / raw)
To: paulmck, frederic, quic_neeraju, joel, josh, boqun.feng, corbet
Cc: rcu, linux-doc, Alan Huang
Fix cases where using rculist_nulls combined with SLAB_TYPESAFE_BY_RCU.
Alan Huang (2):
rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
docs/RCU: Bring back smp_wmb()
Documentation/RCU/rculist_nulls.rst | 9 +++++++--
include/linux/rculist_nulls.h | 4 ++--
2 files changed, 9 insertions(+), 4 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-06 10:28 [PATCH 0/2] rcu: Fix rculist_nulls and doc Alan Huang
@ 2023-07-06 10:28 ` Alan Huang
2023-07-10 19:08 ` Paul E. McKenney
2023-07-10 20:01 ` Joel Fernandes
2023-07-06 10:28 ` [PATCH 2/2] docs/RCU: Bring back smp_wmb() Alan Huang
1 sibling, 2 replies; 11+ messages in thread
From: Alan Huang @ 2023-07-06 10:28 UTC (permalink / raw)
To: paulmck, frederic, quic_neeraju, joel, josh, boqun.feng, corbet
Cc: rcu, linux-doc, Alan Huang
When the objects managed by rculist_nulls are allocated with
SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
object that is being added, which means the modification of ->next
is visible to readers. So, this patch uses WRITE_ONCE() for assignments
to ->next.
Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
include/linux/rculist_nulls.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index ba4c00dd8005..89186c499dd4 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
{
struct hlist_nulls_node *first = h->first;
- n->next = first;
+ WRITE_ONCE(n->next, first);
WRITE_ONCE(n->pprev, &h->first);
rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
if (!is_a_nulls(first))
@@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
last = i;
if (last) {
- n->next = last->next;
+ WRITE_ONCE(n->next, last->next);
n->pprev = &last->next;
rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] docs/RCU: Bring back smp_wmb()
2023-07-06 10:28 [PATCH 0/2] rcu: Fix rculist_nulls and doc Alan Huang
2023-07-06 10:28 ` [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls Alan Huang
@ 2023-07-06 10:28 ` Alan Huang
2023-07-10 19:11 ` Paul E. McKenney
1 sibling, 1 reply; 11+ messages in thread
From: Alan Huang @ 2023-07-06 10:28 UTC (permalink / raw)
To: paulmck, frederic, quic_neeraju, joel, josh, boqun.feng, corbet
Cc: rcu, linux-doc, Alan Huang
The objects are allocated with SLAB_TYPESAFE_BY_RCU, and there is
n->next = first within hlist_add_head_rcu() before rcu_assign_pointer(),
which modifies obj->obj_node.next. There may be readers holding the
reference of obj in lockless_lookup, and when updater modifies ->next,
readers can see the change immediately because of SLAB_TYPESAFE_BY_RCU.
There are two memory ordering required in the insertion algorithm,
we need to make sure obj->key is updated before obj->obj_node.next
and obj->refcnt, atomic_set_release is not enough to provide the
required memory barrier.
Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
Documentation/RCU/rculist_nulls.rst | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/RCU/rculist_nulls.rst b/Documentation/RCU/rculist_nulls.rst
index 21e40fcc08de..fa3729dc7e74 100644
--- a/Documentation/RCU/rculist_nulls.rst
+++ b/Documentation/RCU/rculist_nulls.rst
@@ -64,7 +64,7 @@ but a version with an additional memory barrier (smp_rmb())
{
struct hlist_node *node, *next;
for (pos = rcu_dereference((head)->first);
- pos && ({ next = pos->next; smp_rmb(); prefetch(next); 1; }) &&
+ pos && ({ next = READ_ONCE(pos->next); smp_rmb(); prefetch(next); 1; }) &&
({ obj = hlist_entry(pos, typeof(*obj), obj_node); 1; });
pos = rcu_dereference(next))
if (obj->key == key)
@@ -112,7 +112,12 @@ detect the fact that it missed following items in original chain.
obj = kmem_cache_alloc(...);
lock_chain(); // typically a spin_lock()
obj->key = key;
- atomic_set_release(&obj->refcnt, 1); // key before refcnt
+ /*
+ * We need to make sure obj->key is updated before obj->obj_node.next
+ * and obj->refcnt.
+ */
+ smp_wmb();
+ atomic_set(&obj->refcnt, 1);
hlist_add_head_rcu(&obj->obj_node, list);
unlock_chain(); // typically a spin_unlock()
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-06 10:28 ` [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls Alan Huang
@ 2023-07-10 19:08 ` Paul E. McKenney
2023-07-10 20:01 ` Joel Fernandes
1 sibling, 0 replies; 11+ messages in thread
From: Paul E. McKenney @ 2023-07-10 19:08 UTC (permalink / raw)
To: Alan Huang
Cc: frederic, quic_neeraju, joel, josh, boqun.feng, corbet, rcu,
linux-doc
On Thu, Jul 06, 2023 at 10:28:48AM +0000, Alan Huang wrote:
> When the objects managed by rculist_nulls are allocated with
> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
> object that is being added, which means the modification of ->next
> is visible to readers. So, this patch uses WRITE_ONCE() for assignments
> to ->next.
>
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
Very good, queued for the v6.6 merge window, thank you!
Thanx, Paul
> ---
> include/linux/rculist_nulls.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> index ba4c00dd8005..89186c499dd4 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> {
> struct hlist_nulls_node *first = h->first;
>
> - n->next = first;
> + WRITE_ONCE(n->next, first);
> WRITE_ONCE(n->pprev, &h->first);
> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> if (!is_a_nulls(first))
> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> last = i;
>
> if (last) {
> - n->next = last->next;
> + WRITE_ONCE(n->next, last->next);
> n->pprev = &last->next;
> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> } else {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] docs/RCU: Bring back smp_wmb()
2023-07-06 10:28 ` [PATCH 2/2] docs/RCU: Bring back smp_wmb() Alan Huang
@ 2023-07-10 19:11 ` Paul E. McKenney
2023-07-11 14:50 ` Alan Huang
0 siblings, 1 reply; 11+ messages in thread
From: Paul E. McKenney @ 2023-07-10 19:11 UTC (permalink / raw)
To: Alan Huang
Cc: frederic, quic_neeraju, joel, josh, boqun.feng, corbet, rcu,
linux-doc
On Thu, Jul 06, 2023 at 10:28:49AM +0000, Alan Huang wrote:
> The objects are allocated with SLAB_TYPESAFE_BY_RCU, and there is
> n->next = first within hlist_add_head_rcu() before rcu_assign_pointer(),
> which modifies obj->obj_node.next. There may be readers holding the
> reference of obj in lockless_lookup, and when updater modifies ->next,
> readers can see the change immediately because of SLAB_TYPESAFE_BY_RCU.
>
> There are two memory ordering required in the insertion algorithm,
> we need to make sure obj->key is updated before obj->obj_node.next
> and obj->refcnt, atomic_set_release is not enough to provide the
> required memory barrier.
>
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> ---
> Documentation/RCU/rculist_nulls.rst | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/RCU/rculist_nulls.rst b/Documentation/RCU/rculist_nulls.rst
> index 21e40fcc08de..fa3729dc7e74 100644
> --- a/Documentation/RCU/rculist_nulls.rst
> +++ b/Documentation/RCU/rculist_nulls.rst
> @@ -64,7 +64,7 @@ but a version with an additional memory barrier (smp_rmb())
> {
> struct hlist_node *node, *next;
> for (pos = rcu_dereference((head)->first);
> - pos && ({ next = pos->next; smp_rmb(); prefetch(next); 1; }) &&
> + pos && ({ next = READ_ONCE(pos->next); smp_rmb(); prefetch(next); 1; }) &&
This one looks good, though the READ_ONCE() becoming rcu_dereference()
would allow the smp_rmb() to be dropped, correct?
> ({ obj = hlist_entry(pos, typeof(*obj), obj_node); 1; });
> pos = rcu_dereference(next))
> if (obj->key == key)
> @@ -112,7 +112,12 @@ detect the fact that it missed following items in original chain.
> obj = kmem_cache_alloc(...);
> lock_chain(); // typically a spin_lock()
> obj->key = key;
> - atomic_set_release(&obj->refcnt, 1); // key before refcnt
> + /*
> + * We need to make sure obj->key is updated before obj->obj_node.next
> + * and obj->refcnt.
> + */
> + smp_wmb();
> + atomic_set(&obj->refcnt, 1);
...but what is smp_wmb() doing that the combination of atomic_set_release()
and hlist_add_head_rcu() was not already doing? What am I missing?
Thanx, Paul
> hlist_add_head_rcu(&obj->obj_node, list);
> unlock_chain(); // typically a spin_unlock()
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-06 10:28 ` [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls Alan Huang
2023-07-10 19:08 ` Paul E. McKenney
@ 2023-07-10 20:01 ` Joel Fernandes
2023-07-10 20:30 ` Paul E. McKenney
2023-07-11 14:51 ` Alan Huang
1 sibling, 2 replies; 11+ messages in thread
From: Joel Fernandes @ 2023-07-10 20:01 UTC (permalink / raw)
To: Alan Huang
Cc: paulmck, frederic, quic_neeraju, josh, boqun.feng, corbet, rcu,
linux-doc
On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote:
>
> When the objects managed by rculist_nulls are allocated with
> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
> object that is being added, which means the modification of ->next
> is visible to readers. So, this patch uses WRITE_ONCE() for assignments
> to ->next.
>
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> ---
> include/linux/rculist_nulls.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> index ba4c00dd8005..89186c499dd4 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> {
> struct hlist_nulls_node *first = h->first;
>
> - n->next = first;
> + WRITE_ONCE(n->next, first);
> WRITE_ONCE(n->pprev, &h->first);
> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> if (!is_a_nulls(first))
> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> last = i;
>
> if (last) {
> - n->next = last->next;
> + WRITE_ONCE(n->next, last->next);
> n->pprev = &last->next;
> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> } else {
Don't you need READ_ONCE() for the read-side accesses as well?
thanks,
- Joel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-10 20:01 ` Joel Fernandes
@ 2023-07-10 20:30 ` Paul E. McKenney
2023-07-11 14:56 ` Alan Huang
2023-07-11 14:51 ` Alan Huang
1 sibling, 1 reply; 11+ messages in thread
From: Paul E. McKenney @ 2023-07-10 20:30 UTC (permalink / raw)
To: Joel Fernandes
Cc: Alan Huang, frederic, quic_neeraju, josh, boqun.feng, corbet, rcu,
linux-doc
On Mon, Jul 10, 2023 at 04:01:07PM -0400, Joel Fernandes wrote:
> On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote:
> >
> > When the objects managed by rculist_nulls are allocated with
> > SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
> > object that is being added, which means the modification of ->next
> > is visible to readers. So, this patch uses WRITE_ONCE() for assignments
> > to ->next.
> >
> > Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> > ---
> > include/linux/rculist_nulls.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> > index ba4c00dd8005..89186c499dd4 100644
> > --- a/include/linux/rculist_nulls.h
> > +++ b/include/linux/rculist_nulls.h
> > @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> > {
> > struct hlist_nulls_node *first = h->first;
> >
> > - n->next = first;
> > + WRITE_ONCE(n->next, first);
> > WRITE_ONCE(n->pprev, &h->first);
> > rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> > if (!is_a_nulls(first))
> > @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> > last = i;
> >
> > if (last) {
> > - n->next = last->next;
> > + WRITE_ONCE(n->next, last->next);
> > n->pprev = &last->next;
> > rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> > } else {
>
> Don't you need READ_ONCE() for the read-side accesses as well?
Both hlist_nulls_for_each_entry_rcu() and hlist_nulls_for_each_entry_safe()
use rcu_dereference_raw(). To your point, both hlist_nulls_first_rcu()
and hlist_nulls_next_rcu() look rather strange. I would have expected
them to also use rcu_dereference_raw().
Thanx, Paul
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] docs/RCU: Bring back smp_wmb()
2023-07-10 19:11 ` Paul E. McKenney
@ 2023-07-11 14:50 ` Alan Huang
0 siblings, 0 replies; 11+ messages in thread
From: Alan Huang @ 2023-07-11 14:50 UTC (permalink / raw)
To: paulmck
Cc: frederic, Neeraj Upadhyay, Joel Fernandes, josh, boqun.feng,
corbet, rcu, linux-doc
> 2023年7月11日 03:11,Paul E. McKenney <paulmck@kernel.org> 写道:
>
> On Thu, Jul 06, 2023 at 10:28:49AM +0000, Alan Huang wrote:
>> The objects are allocated with SLAB_TYPESAFE_BY_RCU, and there is
>> n->next = first within hlist_add_head_rcu() before rcu_assign_pointer(),
>> which modifies obj->obj_node.next. There may be readers holding the
>> reference of obj in lockless_lookup, and when updater modifies ->next,
>> readers can see the change immediately because of SLAB_TYPESAFE_BY_RCU.
>>
>> There are two memory ordering required in the insertion algorithm,
>> we need to make sure obj->key is updated before obj->obj_node.next
>> and obj->refcnt, atomic_set_release is not enough to provide the
>> required memory barrier.
>>
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>> ---
>> Documentation/RCU/rculist_nulls.rst | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/RCU/rculist_nulls.rst b/Documentation/RCU/rculist_nulls.rst
>> index 21e40fcc08de..fa3729dc7e74 100644
>> --- a/Documentation/RCU/rculist_nulls.rst
>> +++ b/Documentation/RCU/rculist_nulls.rst
>> @@ -64,7 +64,7 @@ but a version with an additional memory barrier (smp_rmb())
>> {
>> struct hlist_node *node, *next;
>> for (pos = rcu_dereference((head)->first);
>> - pos && ({ next = pos->next; smp_rmb(); prefetch(next); 1; }) &&
>> + pos && ({ next = READ_ONCE(pos->next); smp_rmb(); prefetch(next); 1; }) &&
>
> This one looks good, though the READ_ONCE() becoming rcu_dereference()
> would allow the smp_rmb() to be dropped, correct?
The pattern here is:
reader updater
// pos->next is also obj->obj_node.next
READ_ONCE(pos->next); WRITE_ONCE(obj->key, key);
smp_rmb(); smp_wmb();
// this is n->next = first; within hlist_add_head_rcu
READ_ONCE(obj->key); WRITE_ONCE(obj->obj_node.next, h->first);
The point here is that the objects are allocated with SLAB_TYPESAFE_BY_RCU, and there is
n->next = first;
within hlist_add_head_rcu, the modification is visible to readers immediately (before the invocation of rcu_assign_pointer)
Therefore, the smp_rmb() is necessary to ensure that we won’t get the new value of ->next and the old ->key.
(If we get the new ->next and old ->key, we can not detect movement of the object.)
But in this patch, I forgot to add READ_ONCE to obj->key, will send v2.
>
>> ({ obj = hlist_entry(pos, typeof(*obj), obj_node); 1; });
>> pos = rcu_dereference(next))
>> if (obj->key == key)
>> @@ -112,7 +112,12 @@ detect the fact that it missed following items in original chain.
>> obj = kmem_cache_alloc(...);
>> lock_chain(); // typically a spin_lock()
>> obj->key = key;
>> - atomic_set_release(&obj->refcnt, 1); // key before refcnt
>> + /*
>> + * We need to make sure obj->key is updated before obj->obj_node.next
>> + * and obj->refcnt.
>> + */
>> + smp_wmb();
>> + atomic_set(&obj->refcnt, 1);
>
> ...but what is smp_wmb() doing that the combination of atomic_set_release()
> and hlist_add_head_rcu() was not already doing? What am I missing?
Like above.
>
> Thanx, Paul
>
>> hlist_add_head_rcu(&obj->obj_node, list);
>> unlock_chain(); // typically a spin_unlock()
>>
>> --
>> 2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-10 20:01 ` Joel Fernandes
2023-07-10 20:30 ` Paul E. McKenney
@ 2023-07-11 14:51 ` Alan Huang
1 sibling, 0 replies; 11+ messages in thread
From: Alan Huang @ 2023-07-11 14:51 UTC (permalink / raw)
To: Joel Fernandes
Cc: paulmck, frederic, Neeraj Upadhyay, josh, boqun.feng, corbet, rcu,
linux-doc
> 2023年7月11日 04:01,Joel Fernandes <joel@joelfernandes.org> 写道:
>
> On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote:
>>
>> When the objects managed by rculist_nulls are allocated with
>> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
>> object that is being added, which means the modification of ->next
>> is visible to readers. So, this patch uses WRITE_ONCE() for assignments
>> to ->next.
>>
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>> ---
>> include/linux/rculist_nulls.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
>> index ba4c00dd8005..89186c499dd4 100644
>> --- a/include/linux/rculist_nulls.h
>> +++ b/include/linux/rculist_nulls.h
>> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
>> {
>> struct hlist_nulls_node *first = h->first;
>>
>> - n->next = first;
>> + WRITE_ONCE(n->next, first);
>> WRITE_ONCE(n->pprev, &h->first);
>> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
>> if (!is_a_nulls(first))
>> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
>> last = i;
>>
>> if (last) {
>> - n->next = last->next;
>> + WRITE_ONCE(n->next, last->next);
>> n->pprev = &last->next;
>> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
>> } else {
>
> Don't you need READ_ONCE() for the read-side accesses as well?
Like Paul said, read-side uses rcu_dereference_raw()
>
> thanks,
>
> - Joel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-10 20:30 ` Paul E. McKenney
@ 2023-07-11 14:56 ` Alan Huang
2023-07-11 16:45 ` Paul E. McKenney
0 siblings, 1 reply; 11+ messages in thread
From: Alan Huang @ 2023-07-11 14:56 UTC (permalink / raw)
To: paulmck
Cc: Joel Fernandes, frederic, Neeraj Upadhyay, josh, boqun.feng,
corbet, rcu, linux-doc
> 2023年7月11日 04:30,Paul E. McKenney <paulmck@kernel.org> 写道:
>
> On Mon, Jul 10, 2023 at 04:01:07PM -0400, Joel Fernandes wrote:
>> On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote:
>>>
>>> When the objects managed by rculist_nulls are allocated with
>>> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
>>> object that is being added, which means the modification of ->next
>>> is visible to readers. So, this patch uses WRITE_ONCE() for assignments
>>> to ->next.
>>>
>>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>>> ---
>>> include/linux/rculist_nulls.h | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
>>> index ba4c00dd8005..89186c499dd4 100644
>>> --- a/include/linux/rculist_nulls.h
>>> +++ b/include/linux/rculist_nulls.h
>>> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
>>> {
>>> struct hlist_nulls_node *first = h->first;
>>>
>>> - n->next = first;
>>> + WRITE_ONCE(n->next, first);
>>> WRITE_ONCE(n->pprev, &h->first);
>>> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
>>> if (!is_a_nulls(first))
>>> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
>>> last = i;
>>>
>>> if (last) {
>>> - n->next = last->next;
>>> + WRITE_ONCE(n->next, last->next);
>>> n->pprev = &last->next;
>>> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
>>> } else {
>>
>> Don't you need READ_ONCE() for the read-side accesses as well?
>
> Both hlist_nulls_for_each_entry_rcu() and hlist_nulls_for_each_entry_safe()
> use rcu_dereference_raw(). To your point, both hlist_nulls_first_rcu()
> and hlist_nulls_next_rcu() look rather strange. I would have expected
> them to also use rcu_dereference_raw().
hlist_nulls_first_rcu() and hlist_nulls_next_rcu() were added in commit:
67bdbffd696f(“rculist: avoid __rcu annotations”)
According to the commit message, this avoids warnings from missing __rcu annotations.
>
> Thanx, Paul
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls
2023-07-11 14:56 ` Alan Huang
@ 2023-07-11 16:45 ` Paul E. McKenney
0 siblings, 0 replies; 11+ messages in thread
From: Paul E. McKenney @ 2023-07-11 16:45 UTC (permalink / raw)
To: Alan Huang
Cc: Joel Fernandes, frederic, Neeraj Upadhyay, josh, boqun.feng,
corbet, rcu, linux-doc
On Tue, Jul 11, 2023 at 10:56:02PM +0800, Alan Huang wrote:
>
> > 2023年7月11日 04:30,Paul E. McKenney <paulmck@kernel.org> 写道:
> >
> > On Mon, Jul 10, 2023 at 04:01:07PM -0400, Joel Fernandes wrote:
> >> On Thu, Jul 6, 2023 at 6:29 AM Alan Huang <mmpgouride@gmail.com> wrote:
> >>>
> >>> When the objects managed by rculist_nulls are allocated with
> >>> SLAB_TYPESAFE_BY_RCU, readers may still hold references to this
> >>> object that is being added, which means the modification of ->next
> >>> is visible to readers. So, this patch uses WRITE_ONCE() for assignments
> >>> to ->next.
> >>>
> >>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> >>> ---
> >>> include/linux/rculist_nulls.h | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> >>> index ba4c00dd8005..89186c499dd4 100644
> >>> --- a/include/linux/rculist_nulls.h
> >>> +++ b/include/linux/rculist_nulls.h
> >>> @@ -101,7 +101,7 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
> >>> {
> >>> struct hlist_nulls_node *first = h->first;
> >>>
> >>> - n->next = first;
> >>> + WRITE_ONCE(n->next, first);
> >>> WRITE_ONCE(n->pprev, &h->first);
> >>> rcu_assign_pointer(hlist_nulls_first_rcu(h), n);
> >>> if (!is_a_nulls(first))
> >>> @@ -137,7 +137,7 @@ static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
> >>> last = i;
> >>>
> >>> if (last) {
> >>> - n->next = last->next;
> >>> + WRITE_ONCE(n->next, last->next);
> >>> n->pprev = &last->next;
> >>> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> >>> } else {
> >>
> >> Don't you need READ_ONCE() for the read-side accesses as well?
> >
> > Both hlist_nulls_for_each_entry_rcu() and hlist_nulls_for_each_entry_safe()
> > use rcu_dereference_raw(). To your point, both hlist_nulls_first_rcu()
> > and hlist_nulls_next_rcu() look rather strange. I would have expected
> > them to also use rcu_dereference_raw().
>
> hlist_nulls_first_rcu() and hlist_nulls_next_rcu() were added in commit:
>
> 67bdbffd696f(“rculist: avoid __rcu annotations”)
>
> According to the commit message, this avoids warnings from missing __rcu annotations.
Indeed it does, apologies for the noise!
Thanx, Paul
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-07-11 16:46 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-06 10:28 [PATCH 0/2] rcu: Fix rculist_nulls and doc Alan Huang
2023-07-06 10:28 ` [PATCH 1/2] rcu: Use WRITE_ONCE() for assignments to ->next for rculist_nulls Alan Huang
2023-07-10 19:08 ` Paul E. McKenney
2023-07-10 20:01 ` Joel Fernandes
2023-07-10 20:30 ` Paul E. McKenney
2023-07-11 14:56 ` Alan Huang
2023-07-11 16:45 ` Paul E. McKenney
2023-07-11 14:51 ` Alan Huang
2023-07-06 10:28 ` [PATCH 2/2] docs/RCU: Bring back smp_wmb() Alan Huang
2023-07-10 19:11 ` Paul E. McKenney
2023-07-11 14:50 ` Alan Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).