generic dynamic per cpu refcounting

This has already been on lkml and is in Andrew's tree, Tejun just asked me to send it out again:

+ * percpu_ref_get - increment a dynamic percpu refcount
+ *
+ * Increments @ref and possibly converts it to percpu counters. Must be called
+ * with rcu_read_lock() held, and may potentially drop/reacquire rcu_read_lock()
+ * to allocate percpu counters - if sleeping/allocation isn't safe for some
+ * other reason (e.g. a spinlock), see percpu_ref_get_noalloc().

(quoted diff) +8 -0

+int percpu_ref_kill(struct percpu_ref *ref)
+{
...
+	if (status == PCPU_REF_PTR) {
+		unsigned count = 0, cpu;
+
+		synchronize_rcu();
+
+		for_each_possible_cpu(cpu)

Review

Quoted 12 lines

(quoted diff) +11 -0

+void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
+{
+	unsigned long pcpu_count;
+	uint64_t v;
+
+	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
+
+	if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
+		/* for rcu - we're not using rcu_dereference() */
+		smp_read_barrier_depends();
+		__this_cpu_inc(*((unsigned __percpu *) pcpu_count));

The comment looks confusing a bit... smp_read_barrier_depends() is not for rcu, we obviously need it to access (unsigned __percpu *) pcpu_count. But yes, since we didn't use rcu_dereference() we have to add it by hand.

Quoted 10 lines

(quoted diff) +8 -0

+int percpu_ref_kill(struct percpu_ref *ref)
+{
...
+	if (status == PCPU_REF_PTR) {
+		unsigned count = 0, cpu;
+
+		synchronize_rcu();
+
+		for_each_possible_cpu(cpu)

Review

Quoted 10 lines

(quoted diff) +10 -0

+			count += *per_cpu_ptr((unsigned __percpu *) pcpu_count, cpu);
+
+		pr_debug("global %lli pcpu %i",
+			 atomic64_read(&ref->count) & PCPU_COUNT_MASK,
+			 (int) count);
+
+		atomic64_add((int) count, &ref->count);
+		smp_wmb();
+		/* Between setting global count and setting PCPU_REF_DEAD */
+		ref->pcpu_count = PCPU_REF_DEAD;

The coment explains what the code does, but not why ;)

I guess this is for percpu_ref_put(), and this wmb() pairs with implicit mb() implied by atomic64_dec_return().

(add lkml)

Quoted 3 lines · Kent Overstreet

This has already been on lkml and is in Andrew's tree, Tejun just asked me to send it out again:

I'll try to read this code later, just a couple of questions after a quick glance. Sorry this was already discussed...

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct percpu_ref {
+	atomic64_t		count;
+	unsigned long		pcpu_count;
+};

The code looks a bit tricky mostly because you pack state/pointer/jiffies into ->pcpu_count. The same for ->count.

I assume that you have a good reason to shrink the sizeof(percpu_ref), but I am curious: who is the user of this thing?

Quoted 6 lines

+ * percpu_ref_get - increment a dynamic percpu refcount
+ *
+ * Increments @ref and possibly converts it to percpu counters. Must be called
+ * with rcu_read_lock() held, and may potentially drop/reacquire rcu_read_lock()
+ * to allocate percpu counters - if sleeping/allocation isn't safe for some
+ * other reason (e.g. a spinlock), see percpu_ref_get_noalloc().

And this looks strange. It must be called under rcu_read_lock(), but ->rcu_read_lock_nesting must be == 1. Otherwise rcu_read_unlock() in percpu_ref_alloc() won't work.

Again, I think you have a reason, but could you explain? IOW, why we can't make it might_sleep() instead? The fast path can do rcu_read_lock() itself.

Review

Quoted 4 lines

(quoted diff) +4 -0

+static inline void percpu_ref_get_noalloc(struct percpu_ref *ref)
+{
+	__percpu_ref_get(ref, false);
+}

and this could be percpu_ref_get_atomic().

Once again, I am not arguing, just can't understand.

Quoted 7 lines

(quoted diff) +7 -0

+void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
+{
+	unsigned long pcpu_count;
+	uint64_t v;
+
+	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
+

Review

Quoted 4 lines

(quoted diff) +4 -0

+	if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
+		/* for rcu - we're not using rcu_dereference() */
+		smp_read_barrier_depends();
+		__this_cpu_inc(*((unsigned __percpu *) pcpu_count));

The comment looks confusing a bit... smp_read_barrier_depends() is not for rcu, we obviously need it to access (unsigned __percpu *) pcpu_count. But yes, since we didn't use rcu_dereference() we have to add it by hand.

Quoted 14 lines

(quoted diff) +12 -0

+int percpu_ref_kill(struct percpu_ref *ref)
+{
...
+	if (status == PCPU_REF_PTR) {
+		unsigned count = 0, cpu;
+
+		synchronize_rcu();
+
+		for_each_possible_cpu(cpu)
+			count += *per_cpu_ptr((unsigned __percpu *) pcpu_count, cpu);
+
+		pr_debug("global %lli pcpu %i",
+			 atomic64_read(&ref->count) & PCPU_COUNT_MASK,

Review

Quoted 6 lines

(quoted diff) +6 -0

+			 (int) count);
+
+		atomic64_add((int) count, &ref->count);
+		smp_wmb();
+		/* Between setting global count and setting PCPU_REF_DEAD */
+		ref->pcpu_count = PCPU_REF_DEAD;

The coment explains what the code does, but not why ;)

I guess this is for percpu_ref_put(), and this wmb() pairs with implicit mb() implied by atomic64_dec_return().

Review

Quoted 2 lines

(quoted diff) +1 -0

+		free_percpu((unsigned __percpu *) pcpu_count);

I guess it could be freed right after for_each_possible_cpu() above, but this doesn't matter.

Quoted 16 lines · Oleg Nesterov

Review

Quoted 12 lines

(quoted diff) +11 -0

+void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
+{
+	unsigned long pcpu_count;
+	uint64_t v;
+
+	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
+
+	if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
+		/* for rcu - we're not using rcu_dereference() */
+		smp_read_barrier_depends();
+		__this_cpu_inc(*((unsigned __percpu *) pcpu_count));

The comment looks confusing a bit... smp_read_barrier_depends() is not for rcu, we obviously need it to access (unsigned __percpu *) pcpu_count. But yes, since we didn't use rcu_dereference() we have to add it by hand.

Hmm. Otoh, arch/alpha uses asm-generic/percpu.h so in theory we need smp_read_barrier_depends() after __this_cpu_generic_to_op() calculates the "real" pointer, __this_cpu_ptr() ?

Just curious...

Quoted 23 lines · Oleg Nesterov

Quoted 10 lines

(quoted diff) +8 -0

+int percpu_ref_kill(struct percpu_ref *ref)
+{
...
+	if (status == PCPU_REF_PTR) {
+		unsigned count = 0, cpu;
+
+		synchronize_rcu();
+
+		for_each_possible_cpu(cpu)

Review

Quoted 10 lines

(quoted diff) +10 -0

+			count += *per_cpu_ptr((unsigned __percpu *) pcpu_count, cpu);
+
+		pr_debug("global %lli pcpu %i",
+			 atomic64_read(&ref->count) & PCPU_COUNT_MASK,
+			 (int) count);
+
+		atomic64_add((int) count, &ref->count);
+		smp_wmb();
+		/* Between setting global count and setting PCPU_REF_DEAD */
+		ref->pcpu_count = PCPU_REF_DEAD;

The coment explains what the code does, but not why ;)

Quoted 2 lines

I guess this is for percpu_ref_put(), and this wmb() pairs with implicit mb() implied by atomic64_dec_return().

Hmm. Most probably I missed something, but it seems we need another synchronize_rcu() _after_ we set PCPU_REF_DEAD.

To simplify, suppose that percpu_ref_put() is never called directly but we have

	void put_and_dsetroy(...)
	{
		if (percpu_ref_put(...))
			destroy(...);
	}

Suppose that ref->count == 2 after atomic64_add() above. IOW, we have a "master" reference for _kill() and someone else did _get.

So the caller does

	percpu_ref_kill();
	put_and_dsetroy();

And this can race with another holder which drops the last reference, its put_and_dsetroy() can see PCPU_REF_DYING and return false.

Or I misunderstood the code/interface?

Quoted 7 lines

	percpu_ref_kill();
	put_and_dsetroy();

And this can race with another holder which drops the last reference, its put_and_dsetroy() can see PCPU_REF_DYING and return false.

Or I misunderstood the code/interface?

Nope, nailed it :) That should _definitely_ be in the documentation.

Actually - I think it'd be better to have the default percpu_ref_kill() do the second synchronize_rcu(), and have an unsafe version that skips it.

Hello, guys.

Quoted 8 lines · Kent Overstreet

Quoted 7 lines

	percpu_ref_kill();
	put_and_dsetroy();

And this can race with another holder which drops the last reference, its put_and_dsetroy() can see PCPU_REF_DYING and return false.

Or I misunderstood the code/interface?

Quoted 1 line

Nope, nailed it :) That should _definitely_ be in the documentation.

Can we just combine kill initiation and base ref put and make that the responsibility of the owner? Extra features on basic constructs may seem good for certain use cases but tend to bring more confusion than good in the long run. If a user needs to synchronize among multiple killers, let the user deal with the issue.

Quoted 4 lines

Actually - I think it'd be better to have the default percpu_ref_kill() do the second synchronize_rcu(), and have an unsafe version that skips it.

Note that synchronize_rcu/sched() can be very slow and cause problems in paths which are frequently traveled and visible to userland. It's fine for things like module destruction but can be a problem even during device destruction - blkcg had synchronize_rcu() in request_queue destruction which led to huge latencies during boot because SCSI wants to create and then destroy request_queues for all possible LUNs on certain configurations. So, if you put synchronize_rcu/sched() in percpu_ref_kill(), that better not be used from e.g. close(2).

Quoted 22 lines · Oleg Nesterov

Quoted 16 lines · Oleg Nesterov

Review

Quoted 12 lines

(quoted diff) +11 -0

+void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
+{
+	unsigned long pcpu_count;
+	uint64_t v;
+
+	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
+
+	if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
+		/* for rcu - we're not using rcu_dereference() */
+		smp_read_barrier_depends();
+		__this_cpu_inc(*((unsigned __percpu *) pcpu_count));

The comment looks confusing a bit... smp_read_barrier_depends() is not for rcu, we obviously need it to access (unsigned __percpu *) pcpu_count. But yes, since we didn't use rcu_dereference() we have to add it by hand.

Hmm. Otoh, arch/alpha uses asm-generic/percpu.h so in theory we need smp_read_barrier_depends() after __this_cpu_generic_to_op() calculates the "real" pointer, __this_cpu_ptr() ?

Quoted 1 line

Just curious...

Don't think I follow, but I don't see how... that barriers just needs to be between the ACCESS_ONCE and touching the memory pointed to, the fact that there's a calculation in there to get the real pointer is irrelevant...

Yeah. It'd be really nice if it was doable without synchronize_rcu(), but it'd definitely make get/put heavier.

Though, re. close() - considering we only need a synchronize_rcu() if the ref was in percpu mode, I wonder if that would be a dealbreaker. I have no clue myself.

Getting rid of synchronize_rcu would basically require turning get and put into cmpxchg() loops - even in the percpu fastpath. However, percpu mode would still be getting rid of the shared cacheline contention, we'd just be adding another branch that can be safely marked unlikely() - and my current version has one of those already, so two branches instead of one in the fast path.

Hey, Kent.

Quoted 6 lines · Kent Overstreet

Yeah. It'd be really nice if it was doable without synchronize_rcu(), but it'd definitely make get/put heavier.

Though, re. close() - considering we only need a synchronize_rcu() if the ref was in percpu mode, I wonder if that would be a dealbreaker. I have no clue myself.

The problem is that the performance drop (or latency increase) in patheological cases would be catastrophic. We're talking about possibly quite a few millisecs of delay between each close(). When done sequentially for large number of files, it gets ugly. It becomes a dangerous optimization to make.

Quoted 6 lines

Getting rid of synchronize_rcu would basically require turning get and put into cmpxchg() loops - even in the percpu fastpath. However, percpu mode would still be getting rid of the shared cacheline contention, we'd just be adding another branch that can be safely marked unlikely() - and my current version has one of those already, so two branches instead of one in the fast path.

Or offer an asynchrnous interface so that high-frequency users don't end up inserting synchronize_sched() between each call. It makes the interface more complex and further away from simple atomic_t replacement tho.

Could do that too, but then teardown gets really messy for the user - we need two synchronize_rcu()s:

state := dying

synchronize_rcu()

/* Now nothing's changing the per cpu counters */

Add per cpu counters to atomic counter counter

/* Atomic counter is now consistent */

state := dead

synchronize_rcu()

/* Now percpu_ref_put will check for ref == 0 */

/* Drop initial ref */

percpu_ref_put()

And note that the first synchronize_rcu() is only needed when we had allocated per cpu counters, my current code skips it otherwise.

Hey, Kent.

Quoted 13 lines · Kent Overstreet

Could do that too, but then teardown gets really messy for the user - we need two synchronize_rcu()s:

state := dying

synchronize_rcu()

/* Now nothing's changing the per cpu counters */

Add per cpu counters to atomic counter counter

/* Atomic counter is now consistent */

Quoted 3 lines

state := dead

synchronize_rcu()

I don't understand why we need two stages. What prevents the killing thread from fetching percpu counters after dying passes one synchronize_sched()?

Quoted 5 lines

/* Now percpu_ref_put will check for ref == 0 */

/* Drop initial ref */

Quoted 4 lines

percpu_ref_put()

And note that the first synchronize_rcu() is only needed when we had allocated per cpu counters, my current code skips it otherwise.

And regardless, at the interface level, can't it just provide percpu_ref_put_base_ref(release_fn)?

Quoted 3 lines

I don't understand why we need two stages. What prevents the killing thread from fetching percpu counters after dying passes one synchronize_sched()?

It does. The second synchronize_sched() is needed after we set state := dead, and before we drop the initial ref. Otherwise the ref could hit 0 before percpu_ref_put knows to check for it.

Hello, Kent.

Quoted 7 lines · Kent Overstreet

Quoted 3 lines

I don't understand why we need two stages. What prevents the killing thread from fetching percpu counters after dying passes one synchronize_sched()?

It does. The second synchronize_sched() is needed after we set state := dead, and before we drop the initial ref. Otherwise the ref could hit 0 before percpu_ref_put knows to check for it.

Still a bit confused. Why do we need to make the two steps separate? What prevents us from doing the following?

	set dying;
	synchronize_sched();
	collect percpu refs into global atomic_t;
	put the base ref;

Quoted 4 lines

	set dying;
	synchronize_sched();
	collect percpu refs into global atomic_t;
	put the base ref;

After you set state := dying, percpu_ref_put() decrements the atomic_t, but it can't check if it's 0 yet because the thread that's collecting the percpu refs might not be done yet.

So percpu_ref_put can't check for ref == 0 until after state == dead. But the put in your example might have made ref 0. When did you set state to dead?

But at that point, the operation is already global, so there gotta be a lighter way to synchronize stuff than going through full grace period. ie. You can add a bias value before marking dead so that the counter never reaches zero before all percpu counters are collected and then unbias it right before putting the base ref, that way the only way you can hit zero ref is all refs are actually zero.

Quoted 5 lines · Kent Overstreet

Quoted 4 lines

	set dying;
	synchronize_sched();
	collect percpu refs into global atomic_t;
	put the base ref;

Quoted 7 lines

After you set state := dying, percpu_ref_put() decrements the atomic_t, but it can't check if it's 0 yet because the thread that's collecting the percpu refs might not be done yet.

So percpu_ref_put can't check for ref == 0 until after state == dead. But the put in your example might have made ref 0. When did you set state to dead?

But at that point, the operation is already global, so there gotta be a lighter way to synchronize stuff than going through full grace period. ie. You can add a bias value before marking dead so that the counter never reaches zero before all percpu counters are collected and then unbias it right before putting the base ref, that way the only way you can hit zero ref is all refs are actually zero.

Quoted 6 lines · Tejun Heo

But at that point, the operation is already global, so there gotta be a lighter way to synchronize stuff than going through full grace period. ie. You can add a bias value before marking dead so that the counter never reaches zero before all percpu counters are collected and then unbias it right before putting the base ref, that way the only way you can hit zero ref is all refs are actually zero.

Note that I'm saying that there's no need to distinguish between dying and dead.

The only thing percpu part should care about it whether percpu is on or off. We need a full grace period to turn off percpu operations of any type but that should be the only case where full grace period is necessary. The rest should be synchronizable via the usual global synchronization. We probably can just test percpu variable against NULL and forget about the encoded state numbers.

Ahh. Bias value sounds... hacky (i.e. harder to convince myself it's correct) but I see what you're getting at.

Something to consider is wrapping; after we set state to dying but before we've collected the percpu counters, the atomic counter may be negative.

Hello, Kent.

Quoted 2 lines · Kent Overstreet

Ahh. Bias value sounds... hacky (i.e. harder to convince myself it's correct) but I see what you're getting at.

I don't think it's that hacky. Just push the base point to the opposite of zero - LLONG_MAX. The global counter can start biased so that it gets unbiased only once dying is confirmed and everything can blindly do atomic64_dec_and_test() and trust the result.

Quoted 3 lines

Something to consider is wrapping; after we set state to dying but before we've collected the percpu counters, the atomic counter may be negative.

There's a reason why we use 64bit vars for this type of global counting. They virtually don't wrap. Here, you'll need 1<<63 for the counter to unintentionally reach 0, which we consider practically impossible.

Oh, if this is going to be widely used I should probably have a different implementation for archs that don't have atomic64_t in hardware. Don't suppose you know the CONFIG_ macro to test against? I couldn't find it when I was looking recently.

include/linux/percpu-refcount.h new file +6 -0

+/**
+ * percpu_ref_get - increment a dynamic percpu refcount
+ *
+ * Analagous to atomic_inc().
+  */
+static inline void percpu_ref_get(struct percpu_ref *ref)

Hey, Kent.

Quoted 4 lines · Kent Overstreet

Oh, if this is going to be widely used I should probably have a different implementation for archs that don't have atomic64_t in hardware. Don't suppose you know the CONFIG_ macro to test against? I couldn't find it when I was looking recently.

!CONFIG_GENERIC_ATOMIC64, I think. I don't think it's worthwhile to worry about tho. Reverting to simple atomic_t ops on !CONFIG_SMP should cover most relevant cases.

Very tentative review follows.

include/linux/percpu-refcount.h line 92

Quoted 3 lines

include/linux/percpu-refcount.h new file +3 -0

+#define PCPU_REF_PTR		0
+#define PCPU_REF_NONE		1
+#define PCPU_REF_DEAD		2

Do we still need to distinguish between NONE and DEAD? It would be nice if we can just test against NULL.

Quoted 10 lines

include/linux/percpu-refcount.h new file +10 -0

+/**
+ * percpu_ref_get - increment a dynamic percpu refcount
+ *
+ * Analagous to atomic_inc().
+  */
+static inline void percpu_ref_get(struct percpu_ref *ref)
+{
+	unsigned long pcpu_count;
+
+	preempt_disable();

include/linux/percpu-refcount.h line 111

Quoted 6 lines

include/linux/percpu-refcount.h new file +6 -0

+
+	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
+
+	if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
+		/* for rcu - we're not using rcu_dereference() */
+		smp_read_barrier_depends();

Let's explain more. :)

include/linux/percpu-refcount.h line 112

Quoted 1 line

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

+		__this_cpu_inc(*((unsigned __percpu *) pcpu_count));

What about overflow? Note that we can have systemetic cases where ref is gotten on one cpu and put on another transferring counts in a specific direction.

lib/percpu-refcount.c line 54

Quoted 4 lines

lib/percpu-refcount.c new file +4 -0

+#define PCPU_COUNT_BITS		50
+#define PCPU_COUNT_MASK		((1LL << PCPU_COUNT_BITS) - 1)
+
+#define PCPU_COUNT_BIAS		(1ULL << 32)

I really don't get this. Why use 1<<32 for bias which isn't too difficult to overflow especially if you have many cpu threads and some systemetic drift in percpu refs? What's the point of not using the higher bits? Is it to encode count usage statistics into the same counter? If so, just add another field. 24bytes is fine.

I'd really like to see just basic percpu refcnt with async and sync interfaces w/o dynamic allocation. At this point, we aren't really sure if there are gonna be enough benefits or use cases from dynamic allocation, so I don't really see the point in the added complexity. Let's start with basic stuff and add on dynamic alloc if it actually is necessary.

Quoted 3 lines

What about overflow? Note that we can have systemetic cases where ref is gotten on one cpu and put on another transferring counts in a specific direction.

Heh, this keeps coming up.

It works because modular arithmatic is still associative for addition and subtraction. It doesn't matter which cpu the gets and puts happen on, the counters will still sum to the same value - even if individual counters overflow/underflow.

(It's the same as doing a bunch of increments and decrements to a single integer, but in random order. 0 - 1 - 1 + 1 + 1 + 1 will always equal 1, no matter where you stick parentheses).

That's also why I use an unsigned integer for the per cpu counters... since they're going to wrap and signed integer overflow is undefined. Not that I really expect gcc to figure out a way to screw me for that, but I tend to be anal about such things.

Quoted 5 lines

I really don't get this. Why use 1<<32 for bias which isn't too difficult to overflow especially if you have many cpu threads and some systemetic drift in percpu refs? What's the point of not using the higher bits? Is it to encode count usage statistics into the same counter? If so, just add another field. 24bytes is fine.

The systematic drift doesn't affect overflow here - the bias is only applied to the atomic counter, which can only be off by at most whatever the per cpu counters happen to sum to (and remember it's modular arithmatic, so that's INT_MIN/INT_MAX).

Since I'm using 32 bit ints for the per cpu counters, 1 << 32 works just fine. If we want to expand the per cpu counters to longs or 64 bit ints, then yeah I'd use a larger bias.

It's nitpicky but using a larger bias feels like overly defensive programming to me (if you know how big the bias has to be, great, use that - if you aren't sure, wtf are you doing :P)

Quoted 6 lines

I'd really like to see just basic percpu refcnt with async and sync interfaces w/o dynamic allocation. At this point, we aren't really sure if there are gonna be enough benefits or use cases from dynamic allocation, so I don't really see the point in the added complexity. Let's start with basic stuff and add on dynamic alloc if it actually is necessary.

Well, with the alloc rework the dynamic allocation is completely hidden from the users - it's just get and put. Killing dynamic allocation would just mean that percpu_ref_init() could fail, as far as the api is concerned.

Hello, Kent.

Quoted 15 lines · Kent Overstreet

Quoted 3 lines

What about overflow? Note that we can have systemetic cases where ref is gotten on one cpu and put on another transferring counts in a specific direction.

Heh, this keeps coming up.

It works because modular arithmatic is still associative for addition and subtraction. It doesn't matter which cpu the gets and puts happen on, the counters will still sum to the same value - even if individual counters overflow/underflow.

(It's the same as doing a bunch of increments and decrements to a single integer, but in random order. 0 - 1 - 1 + 1 + 1 + 1 will always equal 1, no matter where you stick parentheses).

Quoted 4 lines

That's also why I use an unsigned integer for the per cpu counters... since they're going to wrap and signed integer overflow is undefined. Not that I really expect gcc to figure out a way to screw me for that, but I tend to be anal about such things.

Hmmm... okay, but that only works if the summing up is also done in 32bits, right? Is that why the global counter is kept at 32bits?

Quoted 6 lines

Quoted 5 lines

I really don't get this. Why use 1<<32 for bias which isn't too difficult to overflow especially if you have many cpu threads and some systemetic drift in percpu refs? What's the point of not using the higher bits? Is it to encode count usage statistics into the same counter? If so, just add another field. 24bytes is fine.

Quoted 4 lines

The systematic drift doesn't affect overflow here - the bias is only applied to the atomic counter, which can only be off by at most whatever the per cpu counters happen to sum to (and remember it's modular arithmatic, so that's INT_MIN/INT_MAX).

Yeah if you don't have to worry about flushing percpu counters to prevent over/underflows, this shouldn't matter. It would be nice to document it tho.

Quoted 7 lines

Since I'm using 32 bit ints for the per cpu counters, 1 << 32 works just fine. If we want to expand the per cpu counters to longs or 64 bit ints, then yeah I'd use a larger bias.

It's nitpicky but using a larger bias feels like overly defensive programming to me (if you know how big the bias has to be, great, use that - if you aren't sure, wtf are you doing :P)

I don't know. I think it's a correctness issue. If you over/underflow into global counter, you need buffer space to be (practically) correct.

If modular calculation makes it unnecessary to
worry about over/underflows, we can't use more than 32bits for global sums, right?

I'm still a bit hazy but it looks like using all 32bit counters should work w/o worrying about over/underflows and that's really nice.

Quoted 7 lines

Quoted 6 lines

I'd really like to see just basic percpu refcnt with async and sync interfaces w/o dynamic allocation. At this point, we aren't really sure if there are gonna be enough benefits or use cases from dynamic allocation, so I don't really see the point in the added complexity. Let's start with basic stuff and add on dynamic alloc if it actually is necessary.

Quoted 4 lines

Well, with the alloc rework the dynamic allocation is completely hidden from the users - it's just get and put. Killing dynamic allocation would just mean that percpu_ref_init() could fail, as far as the api is concerned.

There really is no point in adding complexity which isn't needed. It slows down the code unnecessarily (however minute) and adds maintenance overhead. Why do that when there's no need?

It would definitely be cleaner if the global counter was also 32 bits (I probably should've done it that way at first) but it works with it being bigger _provided the sum of the percpu counters is sign extended_ when collecting them up at the end.

With the bias though we kind of want the global counter to be bigger... the bias needs to fit in the global counter.

Quoted 3 lines

I don't know. I think it's a correctness issue. If you over/underflow into global counter, you need buffer space to be (practically) correct.

Not quite sure what you mean...

It doesn't really make sense to use more than 32 bits for the global sum (ignoring the bias value... that complicates it) but it does work, with the sign extending. I haven't figured out a good way of explaining why though... I should probably look at what happens without it again. (I missed the sign extending when I first coded it, had to debug that).

Quoted 3 lines

There really is no point in adding complexity which isn't needed. It slows down the code unnecessarily (however minute) and adds maintenance overhead. Why do that when there's no need?

Well, like I said I want this to be a drop in replacement for atomic_t anyone can use without having to think about the potential overhead.

If it really does turn out there aren't enough uses for it to be worth it I'll be ok with ripping out the dynamic alloc... but the slowdown is pretty damn minute (one instruction in the fast path) and the additional complexity is fairly minor and quite self contained so I'm just not seeing the urgency.