lkml.org 
[lkml]   [2008]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] percpu_counter: Fix __percpu_counter_sum()
    From
    Date
    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/

    \
     
     \ /
      Last update: 2008-12-03 21:49    [W:0.028 / U:0.232 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site