Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations

[Huang, Ying]

Kent Overstreet <kent.overstreet@xxxxxxxxx> writes:

> On Thu, Apr 15, 2021 at 09:42:56PM -0700, Paul E. McKenney wrote:
>> On Tue, Apr 13, 2021 at 10:47:03AM +0800, Huang Ying wrote:
>> > One typical use case of percpu_ref_tryget() family functions is as
>> > follows,
>> > 
>> >   if (percpu_ref_tryget(&p->ref)) {
>> > 	  /* Operate on the other fields of *p */
>> >   }
>> > 
>> > The refcount needs to be checked before operating on the other fields
>> > of the data structure (*p), otherwise, the values gotten from the
>> > other fields may be invalid or inconsistent.  To guarantee the correct
>> > memory ordering, percpu_ref_tryget*() needs to be the ACQUIRE
>> > operations.
>> 
>> I am not seeing the need for this.
>> 
>> If __ref_is_percpu() returns true, then the overall count must be non-zero
>> and there will be an RCU grace period between now and the time that this
>> count becomes zero.  For the calls to __ref_is_percpu() enclosed within
>> rcu_read_lock() and rcu_read_unlock(), the grace period will provide
>> the needed ordering.  (See the comment header for the synchronize_rcu()
>> function.)
>> 
>> Otherwise, when __ref_is_percpu() returns false, its caller does a
>> value-returning atomic read-modify-write operation, which provides
>> full ordering.

Hi, Paul,

Yes, for the cases you described (from non-zero to 0), current code
works well, no changes are needed.

>> Either way, the required acquire semantics (and more) are already
>> provided, and in particular, this analysis covers the percpu_ref_tryget()
>> you call out above.
>> 
>> Or am I missing something subtle here?
>
> I think you're right, but some details about the race we're concerned about
> would be helpful. Are we concerned about seeing values from after the ref has
> hit 0? In that case I agree with Paul. Or is the concern about seeing values
> from before a transition from 0 to nonzero?

Hi, Kent,

Yes, that's exactly what I concern about.  In swap code, we may get a
pointer to a data structure (swap_info_struct) when its refcount is 0
(not fully initialized), and we cannot access the other fields of the
data structure until its refcount becomes non-zero (fully initialized).
So the order must be guaranteed between checking refcount and accessing
the other fields of the data structure.

I have discussed with Dennis Zhou about this in another thread too,

https://lore.kernel.org/lkml/87o8egp1bk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
https://lore.kernel.org/lkml/YHhOLuIAR3QJ1jx4@xxxxxxxxxx/

He think the use case of swap code isn't typical.  So he prefers to deal
with that in swap code, such as adding a smp_rmb() after
percpu_ref_tryget_live(), etc.

So, if the transition from 0 to non-zero isn't concerned in most other
use cases, I am fine to deal with that in the swap code.

> That wasn't a concern when I wrote
> the code for the patterns of use I had in mind, but Tejun's done some stuff with
> the code since.
>
> Huang, can you elaborate?

Best Regards,
Huang, Ying