> 2023年6月21日 06:33，Paul E. McKenney <paulmck@xxxxxxxxxx> 写道： > > On Sat, Jun 17, 2023 at 02:53:46PM +0000, Alan Huang wrote: >> 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@xxxxxxxxx> >> --- >> 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 e06ed40bb6..77244adbdf 100644 >> --- a/Documentation/RCU/rculist_nulls.rst >> +++ b/Documentation/RCU/rculist_nulls.rst >> @@ -98,7 +98,7 @@ Quoting Corey Minyard:: >> ---------------------- >> >> We need to make sure a reader cannot read the new 'obj->obj_node.next' value >> -and previous value of 'obj->key'. Otherwise, an item could be deleted >> +and previous value of 'obj->key' at the same time. Otherwise, an item could be deleted >> from a chain, and inserted into another chain. If new chain was empty >> before the move, 'next' pointer is NULL, and lockless reader can not >> detect the fact that it missed following items in original chain. >> @@ -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); > > You lost me on this one. > > From what I can see, in the original the atomic_set_release() provided > ordering between the ->key assignment and the ->refcnt assignment, > and then the rcu_assign_pointer() within hlist_add_head_rcu() provided > ordering between both the ->key and the ->refcnt assignments on the one > hand and the list insertion on the other. > > What am I missing here? 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. So, the line n->next = first is running concurrently with the line pos->next in lockless_lookup. As you can see, after pos->next in lockless_lookup is the smp_rmb(), we need to make sure obj->key is updated before obj->obj_node.next (which is n->next = first in hlist_add_head_rcu). So, the smp_wmb() is synchronized with the smp_rmb() in lockless_lookup() and the load acquire in try_get_ref(). > > Thanx, Paul > >> hlist_add_head_rcu(&obj->obj_node, list); >> unlock_chain(); // typically a spin_unlock() >> >> -- >> 2.34.1 >>
