Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 16 Apr 2021 01:02:04 -0400
From:   Kent Overstreet <kent.overstreet@gmail.com>
To:     Huang Ying <ying.huang@intel.com>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        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>
Subject: Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE
 operations
Message-ID: <20210416050054.ws2nl6ds7kd6i4so@zaphod.evilpiepirate.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210416044256.GE4212@paulmck-ThinkPad-P17-Gen-1>
Precedence: bulk

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.
> 
> 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? 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?