From: "Huang, Ying" <ying.huang@intel.com>
To: Kent Overstreet <kent.overstreet@gmail.com>,
"Paul E. McKenney" <paulmck@kernel.org>
Cc: linux-kernel@vger.kernel.org, Tejun Heo <tj@kernel.org>,
Roman Gushchin <guro@fb.com>, Ming Lei <ming.lei@redhat.com>,
Al Viro <viro@zeniv.linux.org.uk>,
Miaohe Lin <linmiaohe@huawei.com>,
Dennis Zhou <dennis@kernel.org>
Subject: Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations
Date: Fri, 16 Apr 2021 14:16:56 +0800 [thread overview]
Message-ID: <87fszqoisn.fsf@yhuang6-desk1.ccr.corp.intel.com> (raw)
In-Reply-To: <20210416050054.ws2nl6ds7kd6i4so@zaphod.evilpiepirate.org> (Kent Overstreet's message of "Fri, 16 Apr 2021 01:02:04 -0400")
Kent Overstreet <kent.overstreet@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.fsf@yhuang6-desk1.ccr.corp.intel.com/
https://lore.kernel.org/lkml/YHhOLuIAR3QJ1jx4@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
prev parent reply other threads:[~2021-04-16 6:17 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-13 2:47 [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations Huang Ying
2021-04-16 4:42 ` Paul E. McKenney
2021-04-16 5:02 ` Kent Overstreet
2021-04-16 6:16 ` Huang, Ying [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87fszqoisn.fsf@yhuang6-desk1.ccr.corp.intel.com \
--to=ying.huang@intel.com \
--cc=dennis@kernel.org \
--cc=guro@fb.com \
--cc=kent.overstreet@gmail.com \
--cc=linmiaohe@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.lei@redhat.com \
--cc=paulmck@kernel.org \
--cc=tj@kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).