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