[RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations

[RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations

[Huang Ying]

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.

This function implements that via using smp_load_acquire() in
__ref_is_percpu() to read the percpu pointer.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kent Overstreet <kent.overstreet@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Roman Gushchin <guro@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Miaohe Lin <linmiaohe@huawei.com>
---
 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

Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations

[Paul E. McKenney]

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" <ying.huang@intel.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Kent Overstreet <kent.overstreet@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Roman Gushchin <guro@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  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
> 

Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations

[Kent Overstreet]

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?

Re: [RFC PATCH] percpu_ref: Make percpu_ref_tryget*() ACQUIRE operations

[Huang, Ying]

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