Messages in this thread | | | Subject | Re: [PATCH] percpu_counter: Fix __percpu_counter_sum() | From | Peter Zijlstra <> | Date | Wed, 03 Dec 2008 21:45:46 +0100 |
| |
On Wed, 2008-12-03 at 21:24 +0100, Eric Dumazet wrote: > 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
Yeah, I see the race, and should have seen it much earlier. Thanks for spotting it.
ext4 added this, and somehow relies on it (non of the other users cares), mingming?
> [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>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> --- > lib/percpu_counter.c | 5 ----- > 1 files changed, 5 deletions(-) > plain text document attachment (__percpu_counter_sum.patch) > 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);
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |