[PATCH] percpu_counter: Fix __percpu_counter_sum()

[Eric Dumazet <dada1@cosmosbay.com>]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

Eric Dumazet a écrit :
> Hi Andrew
> 
> While working on percpu_counter on net-next-2.6, I found
> a CPU unplug race in percpu_counter_destroy()
> 
> (Very unlikely of course)
> 
> Thank you
> 
> [PATCH] percpu_counter: fix CPU unplug race in percpu_counter_destroy()
> 
> We should first delete the counter from percpu_counters list
> before freeing memory, or a percpu_counter_hotcpu_callback()
> could dereference a NULL pointer.
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> ---
> lib/percpu_counter.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
> 

Well, this percpu_counter stuff is simply not working at all.

We added some percpu_counters to network tree for 2.6.29 and we get
drift bugs if calling __percpu_counter_sum() while some heavy duty
benches are running, on a 8 cpus machine

1) __percpu_counter_sum() is buggy, it should not write
on per_cpu_ptr(fbc->counters, cpu), or another cpu
could get its changes lost.

__percpu_counter_sum should be read only (const struct percpu_counter *fbc),
and no locking needed.


2) Un-needed lock in percpu_counter_set()
   This wont block another cpu doing an _add anyway.
   Not a bug, but disturbing, giving false feeling of protection.
   percpu_counter are not precise, we cannot reliably set them
   or read them. Period.
   In fact percpu_counter_set() callers should use
   percpu_counter_add(). (only used from lib/proportions.c )


Thank you

[PATCH] percpu_counter: Fix __percpu_counter_sum()

This function should not write into percpu local storage,
without proper locking, or some changes done on other cpus
might be lost.

Adding proper locking would need to use atomic
operations in fast path and would be expensive.

Results of __percpu_counter_sum() can be wrong, this is a
known fact.

We also dont need to acquire the lock, this gives
no better results.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
---
 lib/percpu_counter.c |    5 -----
 1 files changed, 5 deletions(-)

[-- Attachment #2: __percpu_counter_sum.patch --]
[-- Type: text/plain, Size: 505 bytes --]

diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index a866389..e79bbae 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -57,16 +57,11 @@ s64 __percpu_counter_sum(struct percpu_counter *fbc)
 	s64 ret;
 	int cpu;
 
-	spin_lock(&fbc->lock);
 	ret = fbc->count;
 	for_each_online_cpu(cpu) {
 		s32 *pcount = per_cpu_ptr(fbc->counters, cpu);
 		ret += *pcount;
-		*pcount = 0;
 	}
-	fbc->count = ret;
-
-	spin_unlock(&fbc->lock);
 	return ret;
 }
 EXPORT_SYMBOL(__percpu_counter_sum);