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

From: Huang, Ying
Date: Fri Apr 16 2021 - 02:17:03 EST

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,

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