percpu: implement percpu_ref_reinit()

ioctx_alloc() reaches inside percpu_ref and directly frees ->pcpu_count in its failure path, which is quite gross. percpu_ref has been providing a proper interface to do this, percpu_ref_cancel_init(), for quite some time now. Let's use that instead.

This patch doesn't introduce any behavior changes.

Signed-off-by Tejun Heo <[email protected]>

Cc Benjamin LaHaise <[email protected]>

Cc Kent Overstreet <[email protected]>

---
 fs/aio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/aio.c +2 -2

index 4f078c0..5e0d7f9 100644
@@ -715,8 +715,8 @@ err_ctx:
 err:
 	mutex_unlock(&ctx->ring_lock);
 	free_percpu(ctx->cpu);
-	free_percpu(ctx->reqs.pcpu_count);
-	free_percpu(ctx->users.pcpu_count);
+	percpu_ref_cancel_init(&ctx->reqs);
+	percpu_ref_cancel_init(&ctx->users);
 	kmem_cache_free(kioctx_cachep, ctx);
 	pr_debug("error allocating ioctx %d\n", err);
 	return ERR_PTR(err);

Signature · 1 line

1.9.3

Quoted 5 lines · Tejun Heo

ioctx_alloc() reaches inside percpu_ref and directly frees ->pcpu_count in its failure path, which is quite gross. percpu_ref has been providing a proper interface to do this, percpu_ref_cancel_init(), for quite some time now. Let's use that instead.

I applied this to my aio-next tree at git://git.kvack.org/~bcrl/aio-next.git .

Hello, Benjamin.

It'd probably easier if I route this through percpu tree with the rest of changes as there are further changes on the aio side (trivial but still), so we'd end up having to do pulling dance back and forth. The changes on aio side are mostly trivial. Can I route them through percpu tree with your acks?

Thanks.

Signature · 1 line

tejun

Hi Tejun,

Quoted 1 line · Tejun Heo

Hello, Benjamin.

...

Quoted 5 lines

It'd probably easier if I route this through percpu tree with the rest of changes as there are further changes on the aio side (trivial but still), so we'd end up having to do pulling dance back and forth. The changes on aio side are mostly trivial. Can I route them through percpu tree with your acks?

Sure, no problem -- add my acks and proceed. I don't think there should be any conflicts. Cheers,

percpu-refcount currently reserves two lowest bits of its percpu pointer to indicate its state; however, only one bit is used for PCPU_REF_DEAD.

Simplify it by removing PCPU_STATUS_BITS/MASK and testing PCPU_REF_DEAD directly. This also allows the compiler to choose a more efficient instruction depending on the architecture.

Signed-off-by Tejun Heo <[email protected]>

Cc Kent Overstreet <[email protected]>

Cc Christoph Lameter <[email protected]>

---
 include/linux/percpu-refcount.h | 4 +---
 lib/percpu-refcount.c           | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

include/linux/percpu-refcount.h +0 -0

index 5d8920e..bfdeb0d4 100644

include/linux/percpu-refcount.h +1 -2

-#define PCPU_STATUS_MASK	((1 << PCPU_STATUS_BITS) - 1)
 #define PCPU_REF_PTR		0
 #define PCPU_REF_DEAD		1
 
-#define REF_STATUS(count)	(((unsigned long) count) & PCPU_STATUS_MASK)
+#define REF_STATUS(count)	(((unsigned long) count) & PCPU_REF_DEAD)
 
 /**
  * percpu_ref_get - increment a percpu refcount

lib/percpu-refcount.c +1 -1

index 963b703..17bce2b 100644
@@ -96,7 +96,7 @@ static void percpu_ref_kill_rcu(struct rcu_head *rcu)
 
 	/* Mask out PCPU_REF_DEAD */
 	pcpu_count = (unsigned __percpu *)
-		(((unsigned long) pcpu_count) & ~PCPU_STATUS_MASK);
+		(((unsigned long) pcpu_count) & ~PCPU_REF_DEAD);
 
 	for_each_possible_cpu(cpu)
 		count += *per_cpu_ptr(pcpu_count, cpu);

Now that explicit invocation of percpu_ref_exit() is necessary to free the percpu counter, we can implement percpu_ref_reinit() which reinitializes a released percpu_ref. This can be used implement scalable gating switch which can be drained and then re-opened without worrying about memory allocation failures.

percpu_ref_is_zero() is added to be used in a sanity check in percpu_ref_exit(). As this function will be useful for other purposes too, make it a public interface.

Signed-off-by Tejun Heo <[email protected]>

Cc Kent Overstreet <[email protected]>

Cc Christoph Lameter <[email protected]>

---
 include/linux/percpu-refcount.h | 21 ++++++++++++++++++++-
 lib/percpu-refcount.c           | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

include/linux/percpu-refcount.h +1 -0

index 0ddd283..80f21ea 100644
@@ -67,6 +67,7 @@ struct percpu_ref {
 
 int __must_check percpu_ref_init(struct percpu_ref *ref,
 				 percpu_ref_func_t *release);
+void percpu_ref_reinit(struct percpu_ref *ref);
 void percpu_ref_exit(struct percpu_ref *ref);
 void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 				 percpu_ref_func_t *confirm_kill);

include/linux/percpu-refcount.h +15 -0

 	if (unlikely(pcpu_ptr & PCPU_REF_DEAD))
 		return false;

@@ -206,4 +210,19 @@ static inline void percpu_ref_put(struct percpu_ref *ref)
 	rcu_read_unlock_sched();
 }
 
+/**
+ * percpu_ref_is_zero - test whether a percpu refcount reached zero
+ * @ref: percpu_ref to test
+ *
+ * Returns %true if @ref reached zero.
+ */
+static inline bool percpu_ref_is_zero(struct percpu_ref *ref)
+{
+	unsigned __percpu *pcpu_count;
+
+	if (__pcpu_ref_alive(ref, &pcpu_count))
+		return false;
+	return !atomic_read(&ref->count);
+}
+
 #endif

lib/percpu-refcount.c +35 -0

index ac42991..b348a2f 100644
@@ -61,6 +61,41 @@ int percpu_ref_init(struct percpu_ref *ref, percpu_ref_func_t *release)
 EXPORT_SYMBOL_GPL(percpu_ref_init);
 
 /**
+ * percpu_ref_reinit - re-initialize a percpu refcount
+ * @ref: perpcu_ref to re-initialize
+ *
+ * Re-initialize @ref so that it's in the same state as when it finished
+ * percpu_ref_init().  @ref must have been initialized successfully, killed
+ * and reached 0 but not exited.
+ *
+ * Note that percpu_ref_tryget[_live]() are safe to perform on @ref while
+ * this function is in progress.
+ */
+void percpu_ref_reinit(struct percpu_ref *ref)
+{
+	unsigned __percpu *pcpu_count = pcpu_count_ptr(ref);
+	int cpu;
+
+	BUG_ON(!pcpu_count);
+	WARN_ON(!percpu_ref_is_zero(ref));
+
+	atomic_set(&ref->count, 1 + PCPU_COUNT_BIAS);
+
+	/*
+	 * Restore per-cpu operation.  smp_store_release() is paired with
+	 * smp_load_acquire() in __pcpu_ref_alive() and guarantees that the
+	 * zeroing is visible to all percpu accesses which can see the
+	 * following PCPU_REF_DEAD clearing.
+	 */
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(pcpu_count, cpu) = 0;
+
+	smp_store_release(&ref->pcpu_count_ptr,
+			  ref->pcpu_count_ptr & ~PCPU_REF_DEAD);
+}
+EXPORT_SYMBOL_GPL(percpu_ref_reinit);
+
+/**
  * percpu_ref_exit - undo percpu_ref_init()
  * @ref: percpu_ref to exit
  *

Quoted 5 lines

include/linux/percpu-refcount.h +0 -1

@@ -97,7 +98,10 @@ static inline void percpu_ref_kill(struct percpu_ref *ref)
 static inline bool __pcpu_ref_alive(struct percpu_ref *ref,
 				    unsigned __percpu **pcpu_countp)
 {
-	unsigned long pcpu_ptr = ACCESS_ONCE(ref->pcpu_count_ptr);

include/linux/percpu-refcount.h static inline void percpu_ref_kill(struct percpu_ref *ref) · line 104

Quoted 4 lines

include/linux/percpu-refcount.h +4 -0

+	unsigned long pcpu_ptr;
+
+	/* paired with smp_store_release() in percpu_ref_reinit() */
+	pcpu_ptr = smp_load_acquire(&ref->pcpu_count_ptr);

Does "smp_load_acquire()" hurts the performance of percpu_ref_get/put() in non-x86 system?

Quoted 5 lines · Lai Jiangshan

Quoted 5 lines

include/linux/percpu-refcount.h +0 -1

@@ -97,7 +98,10 @@ static inline void percpu_ref_kill(struct percpu_ref *ref)
 static inline bool __pcpu_ref_alive(struct percpu_ref *ref,
 				    unsigned __percpu **pcpu_countp)
 {
-	unsigned long pcpu_ptr = ACCESS_ONCE(ref->pcpu_count_ptr);

Quoted 8 lines

include/linux/percpu-refcount.h static inline void percpu_ref_kill(struct percpu_ref *ref) · line 104

Quoted 4 lines

include/linux/percpu-refcount.h +4 -0

+	unsigned long pcpu_ptr;
+
+	/* paired with smp_store_release() in percpu_ref_reinit() */
+	pcpu_ptr = smp_load_acquire(&ref->pcpu_count_ptr);

Does "smp_load_acquire()" hurts the performance of percpu_ref_get/put() in non-x86 system?

It's equivalent to data dependency barrier. The only arch which needs something more than barrier() is alpha. It isn't an issue.

Quoted 15 lines · Tejun Heo

Quoted 13 lines · Lai Jiangshan

include/linux/percpu-refcount.h static inline void percpu_ref_kill(struct percpu_ref *ref) · line 104

Quoted 9 lines

include/linux/percpu-refcount.h +4 -1

@@ -97,7 +98,10 @@ static inline void percpu_ref_kill(struct percpu_ref *ref)
 static inline bool __pcpu_ref_alive(struct percpu_ref *ref,
 				    unsigned __percpu **pcpu_countp)
 {
-	unsigned long pcpu_ptr = ACCESS_ONCE(ref->pcpu_count_ptr);
+	unsigned long pcpu_ptr;
+
+	/* paired with smp_store_release() in percpu_ref_reinit() */
+	pcpu_ptr = smp_load_acquire(&ref->pcpu_count_ptr);

Does "smp_load_acquire()" hurts the performance of percpu_ref_get/put() in non-x86 system?

Quoted 3 lines

It's equivalent to data dependency barrier. The only arch which needs something more than barrier() is alpha. It isn't an issue.

But I searched from the source, smp_load_acquire() is just barrier() in x86, arm64, ia64, s390, sparc, but it includes memory barrier instruction in other archs.

Quoted 19 lines · Lai Jiangshan

Quoted 17 lines · Tejun Heo

Quoted 5 lines · Lai Jiangshan

Quoted 5 lines

include/linux/percpu-refcount.h +0 -1

@@ -97,7 +98,10 @@ static inline void percpu_ref_kill(struct percpu_ref *ref)
 static inline bool __pcpu_ref_alive(struct percpu_ref *ref,
 				    unsigned __percpu **pcpu_countp)
 {
-	unsigned long pcpu_ptr = ACCESS_ONCE(ref->pcpu_count_ptr);

Quoted 8 lines

include/linux/percpu-refcount.h static inline void percpu_ref_kill(struct percpu_ref *ref) · line 104

Quoted 4 lines

include/linux/percpu-refcount.h +4 -0

+	unsigned long pcpu_ptr;
+
+	/* paired with smp_store_release() in percpu_ref_reinit() */
+	pcpu_ptr = smp_load_acquire(&ref->pcpu_count_ptr);

Does "smp_load_acquire()" hurts the performance of percpu_ref_get/put() in non-x86 system?

It's equivalent to data dependency barrier. The only arch which needs something more than barrier() is alpha. It isn't an issue.

Quoted 3 lines

But I searched from the source, smp_load_acquire() is just barrier() in x86, arm64, ia64, s390, sparc, but it includes memory barrier instruction in other archs.

Hmmm, right, it's a stronger guarantee than the data dependency barrier. This should probably use smp_wmb() and smp_read_barrier_depends(). That's all it needs anyway.

Thanks.

Signature · 1 line

tejun

Yep, smp_load_acquire() orders its load against later loads and stores, so it really does need a memory barrier on weakly ordered systems.

This is the "publish" operation for dynamically allocated per-CPU references? If so, agreed, you should be able to rely on dependency ordering. Make sure to comment the smp_read_barrier_depends(). ;-)

Hey, Paul.

Quoted 2 lines · Paul E. McKenney

Yep, smp_load_acquire() orders its load against later loads and stores, so it really does need a memory barrier on weakly ordered systems.

Yeap.

Quoted 3 lines

This is the "publish" operation for dynamically allocated per-CPU references? If so, agreed, you should be able to rely on dependency ordering. Make sure to comment the smp_read_barrier_depends(). ;-)

Definitely, there aren't many things which are more frustrating than barriers w/o comments explaining their pairing. I'm pairing store_release with read_barrier_depends as that's what RCU is doing. Is this the preferred way now? I like the new store_release and load_acquire as they document what's being barriered better but as Lai suggested in another reply it does seem a bit unbalanced. I wonder whether load_acquire_depends would make sense.

If you mean what I think you mean by load_acquire_depends(), it is spelled "rcu_dereference()" or, in this case, where you are never removing anything that has been added, "rcu_dereference_raw()". Because you are never removing anything, you don't need rcu_read_lock() or rcu_read_unlock(), thus you don't want lockdep yelling at you about not having RCU read-side critical sections, thus rcu_dereference_raw().

Hello,

Quoted 6 lines · Paul E. McKenney

If you mean what I think you mean by load_acquire_depends(), it is spelled "rcu_dereference()" or, in this case, where you are never removing anything that has been added, "rcu_dereference_raw()". Because you are never removing anything, you don't need rcu_read_lock() or rcu_read_unlock(), thus you don't want lockdep yelling at you about not having RCU read-side critical sections, thus rcu_dereference_raw().

Yeah, along that line but it's kinda weird to use rcu_dereference() when RCU isn't involved. It'd be clearer to have something like load_acquire_depends() and then define RCU deref in terms of it.

This is purely notational and clarifiying in the documentation is probably enough.

Now that explicit invocation of percpu_ref_exit() is necessary to free the percpu counter, we can implement percpu_ref_reinit() which reinitializes a released percpu_ref. This can be used implement scalable gating switch which can be drained and then re-opened without worrying about memory allocation failures.

percpu_ref_is_zero() is added to be used in a sanity check in percpu_ref_exit(). As this function will be useful for other purposes too, make it a public interface.

v2: Use smp_read_barrier_depends() instead of smp_load_acquire(). We
only need data dep barrier and smp_load_acquire() is stronger and heavier on some archs. Spotted by Lai Jiangshan.

Signed-off-by Tejun Heo <[email protected]>

Cc Kent Overstreet <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lai Jiangshan <[email protected]>

---
 include/linux/percpu-refcount.h |   19 +++++++++++++++++++
 lib/percpu-refcount.c           |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

include/linux/percpu-refcount.h +19 -0

@@ -67,6 +67,7 @@ struct percpu_ref {
 
 int __must_check percpu_ref_init(struct percpu_ref *ref,
 				 percpu_ref_func_t *release);
+void percpu_ref_reinit(struct percpu_ref *ref);
 void percpu_ref_exit(struct percpu_ref *ref);
 void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
 				 percpu_ref_func_t *confirm_kill);

@@ -99,6 +100,9 @@ static inline bool __pcpu_ref_alive(stru
 {
 	unsigned long pcpu_ptr = ACCESS_ONCE(ref->pcpu_count_ptr);
 
+	/* paired with smp_store_release() in percpu_ref_reinit() */
+	smp_read_barrier_depends();
+
 	if (unlikely(pcpu_ptr & PCPU_REF_DEAD))
 		return false;
 

@@ -206,4 +210,19 @@ static inline void percpu_ref_put(struct
 	rcu_read_unlock_sched();
 }
 
+/**
+ * percpu_ref_is_zero - test whether a percpu refcount reached zero
+ * @ref: percpu_ref to test
+ *
+ * Returns %true if @ref reached zero.
+ */
+static inline bool percpu_ref_is_zero(struct percpu_ref *ref)
+{
+	unsigned __percpu *pcpu_count;
+
+	if (__pcpu_ref_alive(ref, &pcpu_count))
+		return false;
+	return !atomic_read(&ref->count);
+}
+
 #endif

lib/percpu-refcount.c +13 -0

@@ -61,6 +61,41 @@ int percpu_ref_init(struct percpu_ref *r
 EXPORT_SYMBOL_GPL(percpu_ref_init);
 
 /**
+ * percpu_ref_reinit - re-initialize a percpu refcount
+ * @ref: perpcu_ref to re-initialize
+ *
+ * Re-initialize @ref so that it's in the same state as when it finished
+ * percpu_ref_init().  @ref must have been initialized successfully, killed
+ * and reached 0 but not exited.
+ *
+ * Note that percpu_ref_tryget[_live]() are safe to perform on @ref while
+ * this function is in progress.
+ */
+void percpu_ref_reinit(struct percpu_ref *ref)
+{
+	unsigned __percpu *pcpu_count = pcpu_count_ptr(ref);

lib/percpu-refcount.c +4 -1

+}
+EXPORT_SYMBOL_GPL(percpu_ref_reinit);
+
+/**
  * percpu_ref_exit - undo percpu_ref_init()
  * @ref: percpu_ref to exit
  *
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
.

lib/percpu-refcount.c int percpu_ref_init(struct percpu_ref *r · line 86

Quoted 3 lines

lib/percpu-refcount.c +3 -0

+	/*
+	 * Restore per-cpu operation.  smp_store_release() is paired with
+	 * smp_load_acquire() in __pcpu_ref_alive() and guarantees that the

s/smp_load_acquire()/smp_read_barrier_depends()/

s/smp_store_release()/smp_mb()/ if you accept my next comment.

lib/percpu-refcount.c int percpu_ref_init(struct percpu_ref *r · line 94

Quoted 8 lines

lib/percpu-refcount.c +8 -0

+	 * zeroing is visible to all percpu accesses which can see the
+	 * following PCPU_REF_DEAD clearing.
+	 */
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(pcpu_count, cpu) = 0;
+
+	smp_store_release(&ref->pcpu_count_ptr,
+			  ref->pcpu_count_ptr & ~PCPU_REF_DEAD);

I think it would be better if smp_mb() is used.

it is documented that smp_read_barrier_depends() and smp_mb() are paired. Not smp_read_barrier_depends() and smp_store_release().

Quoted 5 lines · Lai Jiangshan

lib/percpu-refcount.c int percpu_ref_init(struct percpu_ref *r · line 86

Quoted 3 lines

lib/percpu-refcount.c +3 -0

+	/*
+	 * Restore per-cpu operation.  smp_store_release() is paired with
+	 * smp_load_acquire() in __pcpu_ref_alive() and guarantees that the

s/smp_load_acquire()/smp_read_barrier_depends()/

Will update.

Quoted 12 lines

s/smp_store_release()/smp_mb()/ if you accept my next comment.

lib/percpu-refcount.c int percpu_ref_init(struct percpu_ref *r · line 94

Quoted 8 lines

lib/percpu-refcount.c +8 -0

+	 * zeroing is visible to all percpu accesses which can see the
+	 * following PCPU_REF_DEAD clearing.
+	 */
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(pcpu_count, cpu) = 0;
+
+	smp_store_release(&ref->pcpu_count_ptr,
+			  ref->pcpu_count_ptr & ~PCPU_REF_DEAD);

I think it would be better if smp_mb() is used.

smp_wmb() would be better here. We don't need the reader side.

Quoted 2 lines

it is documented that smp_read_barrier_depends() and smp_mb() are paired. Not smp_read_barrier_depends() and smp_store_release().

I don't know. I thought about doing that but the RCU accessors are pairing store_release with read_barrier_depends, so I don't think the particular paring is problematic and store_release is better at documenting what's being barriered.

Quoted 22 lines · Tejun Heo

Quoted 5 lines · Lai Jiangshan

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/*
+	 * Restore per-cpu operation.  smp_store_release() is paired with
+	 * smp_load_acquire() in __pcpu_ref_alive() and guarantees that the

s/smp_load_acquire()/smp_read_barrier_depends()/

Will update.

Quoted 12 lines

s/smp_store_release()/smp_mb()/ if you accept my next comment.

Review

Quoted 8 lines

(quoted diff) +8 -0

+	 * zeroing is visible to all percpu accesses which can see the
+	 * following PCPU_REF_DEAD clearing.
+	 */
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(pcpu_count, cpu) = 0;
+
+	smp_store_release(&ref->pcpu_count_ptr,
+			  ref->pcpu_count_ptr & ~PCPU_REF_DEAD);

I think it would be better if smp_mb() is used.

Quoted 4 lines

smp_wmb() would be better here. We don't need the reader side.

Quoted 2 lines

it is documented that smp_read_barrier_depends() and smp_mb() are paired. Not smp_read_barrier_depends() and smp_store_release().

Well, sounds like the documentation needs an update, then. ;-)

For example, current rcu_assign_pointer() is a wrapper around smp_store_release().

Quoted 4 lines

I don't know. I thought about doing that but the RCU accessors are pairing store_release with read_barrier_depends, so I don't think the particular paring is problematic and store_release is better at documenting what's being barriered.

Which Tejun noted as well.

documentation: Add acquire/release barriers to pairing rules

It is possible to pair acquire and release barriers with other barriers, so this commit adds them to the list in the SMP barrier pairing section.

Reported-by Lai Jiangshan <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Documentation/memory-barriers.txt +0 -0

index a6ca533a73fc..2a7c3c4fb53f 100644
@@ -757,10 +757,12 @@ SMP BARRIER PAIRING
 When dealing with CPU-CPU interactions, certain types of memory barrier should
 always be paired.  A lack of appropriate pairing is almost certainly an error.
 

Hello,

Currently, percpu_ref destruction - freeing the percpu counter - happens when the percpu_ref is released. This while a bit more convenient restricts how percpu_ref can be used. It can't be initialized with static percpu area and it can't be reinitialized without going through percpu allocation.

There are use cases which can take advantage of cycling percpu_ref through init/release multiple times. This patchset separates out percpu_ref destruction into percpu_exit() which should be invoked explicitly and introduces percpu_ref_reinit() which can be used to recycle a released percpu_ref.

This patchset doesn't add any users. Patchset to use this will soon be posted.

This patchset contains the following six patches.

0001-percpu-refcount-aio-use-percpu_ref_cancel_init-in-io.patch 0002-percpu-refcount-one-bit-is-enough-for-REF_STATUS.patch 0003-percpu-refcount-add-helpers-for-percpu_count-accesse.patch 0004-percpu-refcount-use-unsigned-long-for-pcpu_count-poi.patch 0005-percpu-refcount-require-percpu_ref-to-be-exited-expl.patch 0006-percpu-refcount-implement-percpu_ref_reinit-and-perc.patch