Re: [PATCH] percpu_counter: Fix __percpu_counter_sum()

Peter Zijlstra
2008-12-03T20:46:40Z

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);