Re: [PATCH v2] docs/RCU: Bring smp_wmb() back

[Alan Huang <mmpgouride@gmail.com>]

> 2023年6月21日 06:33，Paul E. McKenney <paulmck@kernel.org> 写道：
> 
> 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@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 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
>>