Re: [PATCH 23/33] generic dynamic per cpu refcounting

[Kent Overstreet <koverstreet@google.com>]

On Tue, Apr 02, 2013 at 12:27:38PM -0400, Theodore Ts'o wrote:
> Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> > +		v = atomic64_add_return(1 + (1ULL << PCPU_COUNT_BITS),
> > +					&ref->count);
> > +
> > +		if (!(v >> PCPU_COUNT_BITS) &&
> > +		    REF_STATUS(pcpu_count) == PCPU_REF_NONE && alloc)
> > +			percpu_ref_alloc(ref, pcpu_count);
> 
> This assumes that the kernel is compiled with -fno-strict-overflow.
> Which we do, and this is not the only place int the kernel where we
> depend on this, so while I was nervous before, I'm okay with it now.
> Could we at least have a comment saying that we're depending on
> -fno-strict-overflow, though?

Well, I don't think it is true that we are depending on
-fno-strict-overflow since the overflow happens in atomic_add() which is
a black box to the compiler.

It would be nice if we had unsigned atomic types... but given that we
don't and I'm pretty sure overflow in atomic types happens all over the
place that part honestly seems fine to me...

That said, I suppose a comment indicating that it is intentionally
overflowing is probably merited. Ted, Andrew, is this acceptable to you?

---
 lib/percpu-refcount.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
index 79c6158..200088f 100644
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -124,6 +124,13 @@ void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
 		v = atomic64_add_return(1 + (1ULL << PCPU_COUNT_BITS),
 					&ref->count);
 
+		/*
+		 * The high bits of the counter count the number of gets() that
+		 * have occured; we check for overflow to call
+		 * percpu_ref_alloc() every (1 << (64 - PCPU_COUNT_BITS))
+		 * iterations.
+		 */
+
 		if (!(v >> PCPU_COUNT_BITS) &&
 		    REF_STATUS(pcpu_count) == PCPU_REF_NONE && alloc)
 			percpu_ref_alloc(ref, pcpu_count);
-- 
1.7.12.146.g16d26b1