Kent Overstreet <kent.overstr...@gmail.com> 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....@yhuang6-desk1.ccr.corp.intel.com/
https://lore.kernel.org/lkml/yhholuiar3qj1...@google.com/
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
