Re: [PATCH] percpu: preemptless __per_cpu_counter_add

[Christoph Lameter <cl@linux.com>]

On Sat, 16 Apr 2011, Tejun Heo wrote:

> Maybe, I don't know.  On x86, it shouldn't be a problem on both 32 and
> 64bit.  Even on archs which lack local cmpxchg, preemption flips are
> cheap anyway so yeah maybe.

Preemption flips are not cheap since enabling preemption may mean a call
into the scheduler. On RT things get more expensive.

Preempt_enable means at least one additional branch. We are saving a
branch by not using preempt.

> > The branches are not an issue since they are forward branches over one
> > (after converting to an atomic operation) or two instructions each. A
> > possible stall is only possible in case of the cmpxchg failing.
>
> It's slow path and IMHO it's needlessly complex.  I really don't care
> whether the counter is reloaded once more or the task gets migrated to
> another cpu before spin_lock() and ends up flushing local counter on a
> cpu where it isn't strictly necessary.  Let's keep it simple.

In order to make it simple I avoided an preempt enable/disable. With
Shaohua's patches there will be a simple atomic_add within the last if
cluase. I was able to consolidate multiple code paths into the cmpxchg
loop with this approach.

The one below avoids the #ifdef that is ugly...



Subject: percpu: preemptless __per_cpu_counter_add V4

Use this_cpu_cmpxchg to avoid preempt_disable/enable in __percpu_add.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 lib/percpu_counter.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Index: linux-2.6/lib/percpu_counter.c
===================================================================
--- linux-2.6.orig/lib/percpu_counter.c	2011-04-15 15:34:23.000000000 -0500
+++ linux-2.6/lib/percpu_counter.c	2011-04-18 09:31:37.000000000 -0500
@@ -71,19 +71,25 @@ EXPORT_SYMBOL(percpu_counter_set);

 void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
-	s64 count;
+	s64 count, new, overflow;

-	preempt_disable();
-	count = __this_cpu_read(*fbc->counters) + amount;
-	if (count >= batch || count <= -batch) {
+	do {
+		count = this_cpu_read(*fbc->counters);
+
+		new = count + amount;
+		/* In case of overflow fold it into the global counter instead */
+		if (new >= batch || new <= -batch) {
+			overflow = new;
+			new = 0;
+		} else
+			overflow = 0;
+	} while (this_cpu_cmpxchg(*fbc->counters, count, new) != count);
+
+	if (unlikely(overflow)) {
 		spin_lock(&fbc->lock);
-		fbc->count += count;
-		__this_cpu_write(*fbc->counters, 0);
+		fbc->count += overflow;
 		spin_unlock(&fbc->lock);
-	} else {
-		__this_cpu_write(*fbc->counters, count);
 	}
-	preempt_enable();
 }
 EXPORT_SYMBOL(__percpu_counter_add);


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>