From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 736FDC433B4 for ; Fri, 16 Apr 2021 04:43:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 49522611AB for ; Fri, 16 Apr 2021 04:43:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231408AbhDPEnX (ORCPT ); Fri, 16 Apr 2021 00:43:23 -0400 Received: from mail.kernel.org ([198.145.29.99]:32848 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229555AbhDPEnV (ORCPT ); Fri, 16 Apr 2021 00:43:21 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 1FA7861152; Fri, 16 Apr 2021 04:42:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1618548177; bh=4hdUt+EDEa/QUCWdkHyC/ffQXQkj9XGd5bdPb0nZqGI=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=RNNe76AAteWei9QEDSL1+ENgDEJidm7Scx0Nb9Fw6YLe6E3uthLjhZOlN8AkQawCo ZA6hax7FBKFVQOgw4NMNqmUEnQZFMiKWRK00jPFL+7Ma3F7QFwwGTU3dnMwIGLMAPM kCe/spG4cTm8k7j6pd3G68MQHQm3B3T3G9izCZAAZLz0btelgxzH1O7l8sYLkMWNT/ Z8pfsZprZJLYT8dIZDL19ohTcVmRZxjaV6ye5G7J4pisc210LpvtjFQav7dYWGXGjN LzAlloF6mXhz+8IKecok97GDGZmsC6OLJdnkP8okaH7Ocf4iL9ToGbVFujqYCdVZOS U1gVdCgxBxtdQ== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id D68475C013E; Thu, 15 Apr 2021 21:42:56 -0700 (PDT) Date: Thu, 15 Apr 2021 21:42:56 -0700 From: "Paul E. McKenney" To: Huang Ying Cc: linux-kernel@vger.kernel.org, Tejun Heo , Kent Overstreet , Roman Gushchin , Ming Lei , Al Viro , Miaohe Lin Subject: Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations Message-ID: <20210416044256.GE4212@paulmck-ThinkPad-P17-Gen-1> Reply-To: paulmck@kernel.org References: <20210413024703.2745636-1-ying.huang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210413024703.2745636-1-ying.huang@intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? Thanx, Paul > This function implements that via using smp_load_acquire() in > __ref_is_percpu() to read the percpu pointer. > > Signed-off-by: "Huang, Ying" > Cc: Tejun Heo > Cc: Kent Overstreet > Cc: "Paul E. McKenney" > Cc: Roman Gushchin > Cc: Ming Lei > Cc: Al Viro > Cc: Miaohe Lin > --- > include/linux/percpu-refcount.h | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h > index 16c35a728b4c..9838f7ea4bf1 100644 > --- a/include/linux/percpu-refcount.h > +++ b/include/linux/percpu-refcount.h > @@ -165,13 +165,13 @@ static inline bool __ref_is_percpu(struct percpu_ref *ref, > * !__PERCPU_REF_ATOMIC, which may be set asynchronously, and then > * used as a pointer. If the compiler generates a separate fetch > * when using it as a pointer, __PERCPU_REF_ATOMIC may be set in > - * between contaminating the pointer value, meaning that > - * READ_ONCE() is required when fetching it. > + * between contaminating the pointer value, smp_load_acquire() > + * will prevent this. > * > - * The dependency ordering from the READ_ONCE() pairs > + * The dependency ordering from the smp_load_acquire() pairs > * with smp_store_release() in __percpu_ref_switch_to_percpu(). > */ > - percpu_ptr = READ_ONCE(ref->percpu_count_ptr); > + percpu_ptr = smp_load_acquire(&ref->percpu_count_ptr); > > /* > * Theoretically, the following could test just ATOMIC; however, > @@ -231,6 +231,9 @@ static inline void percpu_ref_get(struct percpu_ref *ref) > * Returns %true on success; %false on failure. > * > * This function is safe to call as long as @ref is between init and exit. > + * > + * This function is an ACQUIRE operation, that is, all memory operations > + * after will appear to happen after checking the refcount. > */ > static inline bool percpu_ref_tryget_many(struct percpu_ref *ref, > unsigned long nr) > @@ -260,6 +263,9 @@ static inline bool percpu_ref_tryget_many(struct percpu_ref *ref, > * Returns %true on success; %false on failure. > * > * This function is safe to call as long as @ref is between init and exit. > + * > + * This function is an ACQUIRE operation, that is, all memory operations > + * after will appear to happen after checking the refcount. > */ > static inline bool percpu_ref_tryget(struct percpu_ref *ref) > { > @@ -280,6 +286,9 @@ static inline bool percpu_ref_tryget(struct percpu_ref *ref) > * percpu_ref_tryget_live(). > * > * This function is safe to call as long as @ref is between init and exit. > + * > + * This function is an ACQUIRE operation, that is, all memory operations > + * after will appear to happen after checking the refcount. > */ > static inline bool percpu_ref_tryget_live(struct percpu_ref *ref) > { > -- > 2.30.2 >