percpu_counter: fix CPU unplug race in percpu_counter_destroy()

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 <[email protected]>

---
lib/percpu_counter.c | 4 ++--

1 files changed, 2 insertions(+), 2 deletions(-)

Quoted 11 lines · Eric Dumazet

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

Quoted 9 lines

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 <[email protected]>

---
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 <[email protected]>

---
 lib/percpu_counter.c |    5 -----
 1 files changed, 5 deletions(-)

plain text document attachment (__percpu_counter_sum.patch)

lib/percpu_counter.c +0 -5

index a866389..e79bbae 100644
@@ -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);

[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 <[email protected]>

Eric Dumazet a __crit :

Quoted 11 lines

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

Quoted 9 lines

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 <[email protected]>

---
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.

Quoted 11 lines · On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <[email protected]> wrote:

Eric Dumazet a __crit :

Quoted 10 lines

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

Quoted 17 lines

Quoted 10 lines

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 <[email protected]>

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

Quoted 5 lines

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

Quoted 1 line

and no locking needed.

No, we can't do this - it will break ext4.

Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at e8ced39d5e8911c662d4d69a342b9d053eaaac4e.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Please read the below changelogs carefully and check that I have got my
head around this correctly - I may not have done.

What a mess.

Quoted 8 lines · On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <[email protected]> wrote:

Eric Dumazet a __crit :

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.

No, we can't do this - it will break ext4.

Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at e8ced39d5e8911c662d4d69a342b9d053eaaac4e.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Please read the below changelogs carefully and check that I have got my head around this correctly - I may not have done.

Andrew Morton a __crit :

Quoted 27 lines · On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <[email protected]> wrote:

Quoted 9 lines

Eric Dumazet a __crit :

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.

No, we can't do this - it will break ext4.

Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at e8ced39d5e8911c662d4d69a342b9d053eaaac4e.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Quoted 3 lines

Please read the below changelogs carefully and check that I have got my head around this correctly - I may not have done.

Hum... e8ced39d5e8911c662d4d69a342b9d053eaaac4e is probably following the wrong path, but I see the intent. Even in the 'nr_files' case, it could help to reduce excessive calls to percpu_counter_sum()

What we can do is to use two s64 counters (only in SMP):

Quoted 27 lines · On Sun, 07 Dec 2008 14:28:00 +0100 Eric Dumazet <[email protected]> wrote:

Andrew Morton a __crit :

Quoted 26 lines · On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <[email protected]> wrote:

Quoted 9 lines

Eric Dumazet a __crit :

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.

No, we can't do this - it will break ext4.

Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at e8ced39d5e8911c662d4d69a342b9d053eaaac4e.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Quoted 8 lines

Quoted 3 lines

Please read the below changelogs carefully and check that I have got my head around this correctly - I may not have done.

Hum... e8ced39d5e8911c662d4d69a342b9d053eaaac4e is probably following the wrong path, but I see the intent. Even in the 'nr_files' case, it could help to reduce excessive calls to percpu_counter_sum()

We should fix this in 2.6.28 - right now percpu_counter_sum() is subtly corrupting the counter's value.

I sent two revert patches which I hope to merge into 2.6.28. Could you guys please read/review/maybe-test them?

Andrew Morton a __crit :

Quoted 41 lines · On Sun, 07 Dec 2008 14:28:00 +0100 Eric Dumazet <[email protected]> wrote:

Quoted 28 lines

Andrew Morton a __crit :

Quoted 26 lines · On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <[email protected]> wrote:

Quoted 9 lines

Eric Dumazet a __crit :

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.

No, we can't do this - it will break ext4.

Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at e8ced39d5e8911c662d4d69a342b9d053eaaac4e.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Quoted 8 lines

Quoted 3 lines

Please read the below changelogs carefully and check that I have got my head around this correctly - I may not have done.

Hum... e8ced39d5e8911c662d4d69a342b9d053eaaac4e is probably following the wrong path, but I see the intent. Even in the 'nr_files' case, it could help to reduce excessive calls to percpu_counter_sum()

We should fix this in 2.6.28 - right now percpu_counter_sum() is subtly corrupting the counter's value.

Quoted 2 lines

I sent two revert patches which I hope to merge into 2.6.28. Could you guys please read/review/maybe-test them?

Your revert patches have the same effect than my first patch : No writes in percpu_counter_sum()

I am lost here Andrew...

The first patch which was added (pre-2.6.27) was "percpu_counter: new function percpu_counter_sum_and_set". This added the broken-by-design percpu_counter_sum_and_set() function, **and used it in ext4**.

Quoted 5 lines · Andrew Morton

The first patch which was added (pre-2.6.27) was "percpu_counter: new function percpu_counter_sum_and_set". This added the broken-by-design percpu_counter_sum_and_set() function, **and used it in ext4**.

Mea culpa, I was the one who reviewed Mingming's patch, and missed this. Part of the problem was that percpu_counter.c isn't well documented, and I so saw the spinlock, but didn't realize it only protected reference counter, and not the per-cpu array. I should have read through code more thoroughly before approving the patch.

I suppose if we wanted we could add a rw spinlock which mediates access to a "foreign" cpu counter (i.e., percpu_counter_add gets a shared lock, and percpu_counter_set needs an exclusive lock) but it's probably not worth it.

Actually, if all popular architectures had a hardware-implemented atomic_t, I wonder how much ext4 really needs the percpu counter, especially given ext4's multiblock allocator; with ext3, given that each block allocation required taking a per-filesystem spin lock, optimizing away that spinlock was far more important for improving ext3's scalability. But with the multiblock allocator, it may that we're going through a lot more effort than what is truly necessary.

atomic_t is pretty good on all archs, but you get to keep the cacheline ping-pong.

Quoted 4 lines · Peter Zijlstra

atomic_t is pretty good on all archs, but you get to keep the cacheline ping-pong.

Stupid question --- if you're worried about cacheline ping-pongs, why aren't each cpu's delta counter cacheline aligned? With a 64-byte cache-line, and a 32-bit counters entry, with less than 16 CPU's we're going to be getting cache ping-pong effects with percpu_counter's, right? Or am I missing something?

Quoted 11 lines · Theodore Tso

Quoted 4 lines · Peter Zijlstra

atomic_t is pretty good on all archs, but you get to keep the cacheline ping-pong.

Stupid question --- if you're worried about cacheline ping-pongs, why aren't each cpu's delta counter cacheline aligned? With a 64-byte cache-line, and a 32-bit counters entry, with less than 16 CPU's we're going to be getting cache ping-pong effects with percpu_counter's, right? Or am I missing something?

sorta - a new per-cpu allocator is in the works, but we do cacheline align the per-cpu allocations (or used to), also, the allocations are node affine.

Quoted 9 lines · Theodore Tso

Quoted 3 lines · Peter Zijlstra

atomic_t is pretty good on all archs, but you get to keep the cacheline ping-pong.

Stupid question --- if you're worried about cacheline ping-pongs, why aren't each cpu's delta counter cacheline aligned? With a 64-byte cache-line, and a 32-bit counters entry, with less than 16 CPU's we're going to be getting cache ping-pong effects with percpu_counter's, right? Or am I missing something?

sorta - a new per-cpu allocator is in the works, but we do cacheline align the per-cpu allocations (or used to), also, the allocations are node affine.

Quoted 11 lines · Peter Zijlstra

Quoted 9 lines · Theodore Tso

Quoted 3 lines · Peter Zijlstra

atomic_t is pretty good on all archs, but you get to keep the cacheline ping-pong.

Stupid question --- if you're worried about cacheline ping-pongs, why aren't each cpu's delta counter cacheline aligned? With a 64-byte cache-line, and a 32-bit counters entry, with less than 16 CPU's we're going to be getting cache ping-pong effects with percpu_counter's, right? Or am I missing something?

Quoted 4 lines

sorta - a new per-cpu allocator is in the works, but we do cacheline align the per-cpu allocations (or used to), also, the allocations are node affine.

I did work on a 'light weight percpu counter', aka percpu_lcounter, for all metrics that dont need 64 bits wide, but a plain 'long' (network, nr_files, nr_dentry, nr_inodes, ...)

struct percpu_lcounter {
        atomic_long_t count;
#ifdef CONFIG_SMP
#ifdef CONFIG_HOTPLUG_CPU
        struct list_head list;  /* All percpu_counters are on a list */
#endif
        long *counters;

#endif
};

(No more spinlock)

Then I tried to have atomic_t (or atomic_long_t) for 'counters', but got a 10% slow down of __percpu_lcounter_add(), even if never hitting the 'slow path' atomic_long_add_return() is really expensiven, even on a non contended cache line.

struct percpu_lcounter {
        atomic_long_t count;
#ifdef CONFIG_SMP
#ifdef CONFIG_HOTPLUG_CPU
        struct list_head list;  /* All percpu_counters are on a list */
#endif
        atomic_long_t *counters;

#endif
};

So I believe the percpu_clounter_sum() that tries to reset to 0 all cpu local
counts would be really too expensive, if it slows down _add() so much.

long percpu_lcounter_sum(struct percpu_lcounter *fblc)

{
        long acc = 0;
        int cpu;

        for_each_online_cpu(cpu)
                acc += atomic_long_xchg(per_cpu_ptr(fblc->counters, cpu), 0);
        return atomic_long_add_return(acc, &fblc->count);
}

void __percpu_lcounter_add(struct percpu_lcounter *flbc, long amount, s32 batch)

{
        long count;
        atomic_long_t *pcount;

        pcount = per_cpu_ptr(flbc->counters, get_cpu());
        count = atomic_long_add_return(amount, pcount); /* way too expensive !!! */

        if (unlikely(count >= batch || count <= -batch)) {
                atomic_long_add(count, &flbc->count);
                atomic_long_sub(count, pcount);

        }
        put_cpu();
}

Just forget about it and let percpu_lcounter_sum() only read the values, and let percpu_lcounter_add() not using atomic ops in fast path.

void __percpu_lcounter_add(struct percpu_lcounter *flbc, long amount, s32 batch)

{
        long count;
        long *pcount;

        pcount = per_cpu_ptr(flbc->counters, get_cpu());
        count = *pcount + amount;
        if (unlikely(count >= batch || count <= -batch)) {
                atomic_long_add(count, &flbc->count);
                count = 0;
        }
        *pcount = count;
        put_cpu();
}

EXPORT_SYMBOL(__percpu_lcounter_add);

Also, with upcoming NR_CPUS=4096, it may be time to design a hierarchical percpu_counter, to avoid hitting one shared "fbc->count" all the time a local counter overflows.

Now percpu_counter_sum() is 'fixed', what about "percpu_counter_add()" ?

void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        s64 count;
        s32 *pcount;
        int cpu = get_cpu();

        pcount = per_cpu_ptr(fbc->counters, cpu);
        count = *pcount + amount;
        if (count >= batch || count <= -batch) {
                spin_lock(&fbc->lock);
                fbc->count += count;
                *pcount = 0;
                spin_unlock(&fbc->lock);
        } else {
                *pcount = count;
        }
        put_cpu();
}

If I read this well, this is not IRQ safe.

get_cpu() only disables preemption IMHO

For nr_files, nr_dentry, nr_inodes, it should not be a problem.

But for network counters (only in net-next-2.6)
and lib/proportions.c, we have a problem ?

Using local_t instead of s32 for cpu
local counter here is possible, so that fast path doesnt have to disable interrupts

(use a local_t instead of s32 for fbc->counters)

void __percpu_counter_add_irqsafe(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        long count;
        local_t *pcount;

	/* following code only matters on 32bit arches */
	if (sizeof(amount) != sizeof(local_t)) {
		if (unlikely(amount >= batch || amount <= -batch))) {
        	        spin_lock_irqsave(&fbc->lock, flags);
                	fbc->count += amount;
                	spin_unlock_irqrestore(&fbc->lock, flags);
			return;
		}
	}
        pcount = per_cpu_ptr(fbc->counters, get_cpu());
        count = local_add_return((long)amount, pcount);
        if (unlikely(count >= batch || count <= -batch)) {
		unsigned long flags;

		local_sub(count, pcount);
                spin_lock_irqsave(&fbc->lock, flags);
                fbc->count += count;
                spin_unlock_irqrestore(&fbc->lock, flags);
        }
        put_cpu();
}

Quoted 22 lines · On Wed, 10 Dec 2008 06:09:08 +0100 Eric Dumazet <[email protected]> wrote:

Now percpu_counter_sum() is 'fixed', what about "percpu_counter_add()" ?

void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        s64 count;
        s32 *pcount;
        int cpu = get_cpu();

        pcount = per_cpu_ptr(fbc->counters, cpu);
        count = *pcount + amount;
        if (count >= batch || count <= -batch) {
                spin_lock(&fbc->lock);
                fbc->count += count;
                *pcount = 0;
                spin_unlock(&fbc->lock);
        } else {
                *pcount = count;
        }
        put_cpu();
}

Quoted 1 line

If I read this well, this is not IRQ safe.

Sure. It's racy against interrupts on this cpu, it'll deadlock over the non-irq-safe spinlock and lockdep will have a coronary over it.

Quoted 1 line

get_cpu() only disables preemption IMHO

yes

Quoted 1 line

For nr_files, nr_dentry, nr_inodes, it should not be a problem.

yes

Quoted 2 lines

But for network counters (only in net-next-2.6)
and lib/proportions.c, we have a problem ?

yes

Quoted 25 lines

Using local_t instead of s32 for cpu
local counter here is possible, so that fast path doesnt have to disable interrupts

(use a local_t instead of s32 for fbc->counters)

void __percpu_counter_add_irqsafe(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        long count;
        local_t *pcount;

	/* following code only matters on 32bit arches */
	if (sizeof(amount) != sizeof(local_t)) {
		if (unlikely(amount >= batch || amount <= -batch))) {
        	        spin_lock_irqsave(&fbc->lock, flags);
                	fbc->count += amount;
                	spin_unlock_irqrestore(&fbc->lock, flags);
			return;
		}
	}
        pcount = per_cpu_ptr(fbc->counters, get_cpu());
        count = local_add_return((long)amount, pcount);
        if (unlikely(count >= batch || count <= -batch)) {
		unsigned long flags;

Quoted 7 lines

		local_sub(count, pcount);
                spin_lock_irqsave(&fbc->lock, flags);
                fbc->count += count;
                spin_unlock_irqrestore(&fbc->lock, flags);
        }
        put_cpu();
}

I think it's reasonable. If the batching is working as intended, the increased cost of s/spin_lock/spin_lock_irqsave/ should be insignificant.

In fact, if *at all* possible it would be best to make percpu_counters irq-safe under all circumstances and avoid fattening and complicating the interface.

But before adding more dependencies on local_t I do think we should refresh ourselves on Christoph's objections to them - I remember finding them fairly convincing at the time, but I don't recall the details.

<searches for a long time>

Here, I think:
http://lkml.indiana.edu/hypermail/linux/kernel/0805.3/2482.html

Rusty, Christoph: talk to me. If we add a new user of local_t in core kernel, will we regret it?

But then, some (all but x86 ;) ) arches dont have true local_t and we fallback to plain atomic_long_t, and this is wrong because it would add a LOCKED instruction in fast path.

I remember Christoph added FAST_CMPXCHG_LOCAL, but no more uses of it in current tree.

Ie : using local_t only if CONFIG_FAST_CMPXCHG_LOCAL, else something like :

void __percpu_counter_add_irqsafe(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        s64 count;
        s32 *pcount = per_cpu_ptr(fbc->counters, get_cpu());
        unsigned long flags;

        local_irq_save(flags);
        count = *pcount + amount;

Quoted 15 lines · On Thursday 11 December 2008 09:26:37 Eric Dumazet wrote:

But then, some (all but x86 ;) ) arches dont have true local_t and we fallback to plain atomic_long_t, and this is wrong because it would add a LOCKED instruction in fast path.

I remember Christoph added FAST_CMPXCHG_LOCAL, but no more uses of it in current tree.

Ie : using local_t only if CONFIG_FAST_CMPXCHG_LOCAL, else something like :

void __percpu_counter_add_irqsafe(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        s64 count;
        s32 *pcount = per_cpu_ptr(fbc->counters, get_cpu());
        unsigned long flags;

Quoted 2 lines

        local_irq_save(flags);
        count = *pcount + amount;

This is dumb though. If local_irq_save(), add, local_irq_restore() is faster than atomic_long_add on some arch, *that* is what that arch's local_add() should do!

Open coding it like this is obviously wrong.

Now, archs local.h need attention (x86-32 can be optimized today, for example), but that's not directly related.

Hope that clarifies,
Rusty.
PS. Yes, I should produce a documentation patch and fix the x86 version. Added to TODO list.

After discussions on percpu_counter subject, I cooked following patch

My goals were :

- IRQ safe percpu_counter (needed for net-next-2.6)
z- 64bit platforms can avoid spin_lock and reduce size of percpu_counter
- No increase of API

Final result, on x86_64, __percpu_counter_add() is really fast and irq safe :

Changes are :

We use local_t instead of s32 for the local storage (for each cpu)

We use atomic_long_t instead of s64 on 64bit arches, to avoid spin_lock.

On 32bit arches, we guard the shared s64 value with an irqsafe spin_lock. As this spin_lock is not taken in fast path, this should not make a real difference.

Signed-off-by Eric Dumazet <[email protected]>

---
 include/linux/percpu_counter.h |   38 +++++++++--
 lib/percpu_counter.c           |  104 ++++++++++++++++++-------------
 2 files changed, 94 insertions(+), 48 deletions(-)

include/linux/percpu_counter.h +12 -0

index 9007ccd..f5133ce 100644
@@ -12,16 +12,42 @@
 #include <linux/threads.h>
 #include <linux/percpu.h>
 #include <linux/types.h>
+#include <asm/local.h>
 
 #ifdef CONFIG_SMP
 
+#ifdef CONFIG_64BIT
+struct s64_counter {
+	atomic_long_t val;
+};
+
+static inline s64 s64c_read(struct s64_counter *c)
+{
+	return atomic_long_read(&c->val);
+}
+#else
+struct s64_counter {

include/linux/percpu_counter.h +4 -0

+	return c->val;
+}
+
+#endif
 

lib/percpu_counter.c +21 -0

index b255b93..6ef4a44 100644
@@ -14,35 +14,58 @@ static LIST_HEAD(percpu_counters);
 static DEFINE_MUTEX(percpu_counters_lock);
 #endif
 
+#ifdef CONFIG_64BIT
+static inline void s64c_add(s64 amount, struct s64_counter *c)
+{
+	atomic_long_add(amount, &c->val);
+}
+
+static inline void s64c_set(struct s64_counter *c, s64 amount)
+{
+	atomic_long_set(&c->val, amount);
+}
+
+#else
+
+static inline void s64c_add(s64 amount, struct s64_counter *c)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->lock, flags);
+	c->val += amount;
+	spin_unlock_irqrestore(&c->lock, flags);
+}

lib/percpu_counter.c +0 -0

 	return err;
 }

One think that sprung to mind,..

IFF we're going to implement percpu_counter with local_t and make local_t this funny tri-counter thing which has its own error, you need to fix up bdi_stat_error() - it expects to be an upper bound for the counter error, getting that wrong _will_ cause deadlocks.

Rusty, Christoph: talk to me. If we add a new user of local_t in core kernel, will we regret it?

Quoted 2 lines · On Wednesday 10 December 2008 16:19:21 Andrew Morton wrote:

Rusty, Christoph: talk to me. If we add a new user of local_t in core kernel, will we regret it?

Interesting. There's new local_t infrastructure, but it's not used.

Here's a benchmark patch showing some results. x86 is pretty close to optimal already, though my results on Power show atomic_long is a bad choice there.

I'll do an audit of the users, then send out some local_t cleanup patches etc.

Benchmarks for local_t variants

(This patch also fixes the x86 cpu_local_* macros, which are obviously unused).

I chose a large array (1M longs) for the inc/add/add_return tests so the trivalue case would show some cache pressure.

The cpu_local_inc case is always cache-hot, so it's not comparable to the others.

Time in ns per iteration (brackets is with CONFIG_PREEMPT=y):

inc add add_return cpu_local_inc read
x86-32: 2.13 Ghz Core Duo 2
atomic_long 118 118 115 17 17
irqsave/rest 77 78 77 23 16
trivalue 45 45 127 3(6) 21
local_t 36 36 36 1(5) 17

x86-64: 2.6 GHz Dual-Core AMD Opteron 2218
atomic_long 55 60 - 6 19
irqsave/rest 54 54 - 11 19
trivalue 47 47 - 5 28
local_t 47 46 - 1 19

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/include/asm/local.h |   20 ++--
 init/main.c                  |  198 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 208 insertions(+), 10 deletions(-)

Now percpu_counter_sum() is 'fixed', what about "percpu_counter_add()" ?

void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)

{
        s64 count;
        s32 *pcount;
        int cpu = get_cpu();

        pcount = per_cpu_ptr(fbc->counters, cpu);
        count = *pcount + amount;
        if (count >= batch || count <= -batch) {
                spin_lock(&fbc->lock);
                fbc->count += count;
                *pcount = 0;
                spin_unlock(&fbc->lock);
        } else {
                *pcount = count;
        }
        put_cpu();
}

If I read this well, this is not IRQ safe.

get_cpu() only disables preemption IMHO

For nr_files, nr_dentry, nr_inodes, it should not be a problem.

But for network counters (only in net-next-2.6)
and lib/proportions.c, we have a problem ?

Quoted 4 lines · Peter Zijlstra

atomic_t is pretty good on all archs, but you get to keep the cacheline ping-pong.

Stupid question --- if you're worried about cacheline ping-pongs, why aren't each cpu's delta counter cacheline aligned?

Quoted 2 lines

Stupid question --- if you're worried about cacheline ping-pongs, why aren't each cpu's delta counter cacheline aligned?

They are allocated with alloc_percpu(), so each CPU's counter lives in a per-cpu area. If you chase through seventeen layers of Rustyness you end up at mm/allocpercpu.c:percpu_populate() which is where that little s32 ends up getting allocated.

Actually, if all popular architectures had a hardware-implemented atomic_t, I wonder how much ext4 really needs the percpu counter, especially given ext4's multiblock allocator; with ext3, given that each block allocation required taking a per-filesystem spin lock, optimizing away that spinlock was far more important for improving ext3's scalability. But with the multiblock allocator, it may that we're going through a lot more effort than what is truly necessary.

Quoted 5 lines · Andrew Morton

The first patch which was added (pre-2.6.27) was "percpu_counter: new function percpu_counter_sum_and_set". This added the broken-by-design percpu_counter_sum_and_set() function, **and used it in ext4**.

Mea culpa, I was the one who reviewed Mingming's patch, and missed this. Part of the problem was that percpu_counter.c isn't well documented, and I so saw the spinlock, but didn't realize it only protected reference counter, and not the per-cpu array. I should have read through code more thoroughly before approving the patch.

I suppose if we wanted we could add a rw spinlock which mediates access to a "foreign" cpu counter (i.e., percpu_counter_add gets a shared lock, and percpu_counter_set needs an exclusive lock) but it's probably not worth it.

Actually, if all popular architectures had a hardware-implemented atomic_t, I wonder how much ext4 really needs the percpu counter, especially given ext4's multiblock allocator;

We can do lots of things in 2.6.29. Including just making ->counters an array of atomic_t.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Quoted 10 lines · Andrew Morton

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

I've looked the two patches which you've queued in the -mm branch, and they look correct to me.

The bugs fixed by these patches can potentially lead to filesystem corruption, since we ultimately use these fields to set the superblock values. This in my mind makes them stable candidates at the very least, and if we weren't so late in the 2.6.28 cycle, I'd be strongly tempted to push them to Linus as a bugfix before the merge window.

Andrew, any strong objections for me to grab them for the ext4 tree? Or would you rather carry them? I would prefer that they get pushed to Linus as soon as the merge window opens, which is one reason why I'd prefer carry them, but we can do this either way.

(cc stable)

Quoted 21 lines · On Sun, 7 Dec 2008 10:28:21 -0500 Theodore Tso <[email protected]> wrote:

Quoted 10 lines · Andrew Morton

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

I've looked the two patches which you've queued in the -mm branch, and they look correct to me.

The bugs fixed by these patches can potentially lead to filesystem corruption, since we ultimately use these fields to set the superblock values. This in my mind makes them stable candidates at the very least, and if we weren't so late in the 2.6.28 cycle, I'd be strongly tempted to push them to Linus as a bugfix before the merge window.

Quoted 5 lines

Andrew, any strong objections for me to grab them for the ext4 tree? Or would you rather carry them? I would prefer that they get pushed to Linus as soon as the merge window opens, which is one reason why I'd prefer carry them, but we can do this either way.

I'm planning on sending them off to Linus for 2.6.28 this week, assuming nobody can think of a plausible reason to not do that.

Now I didn't look _very_ closely at the chronology, but I think that revert-percpu-counter-clean-up-percpu_counter_sum_and_set.patch reverts a post-2.6.27 change, and is not needed in stable.

revert-percpu_counter-new-function-percpu_counter_sum_and_set.patch however reverts a pre-2.6.27 change, and should be merged into 2.6.27. This patch reverts the addition and use of percpu_counter_sum_and_set(), which is racy and can corrupt the counters.

However revert-percpu_counter-new-function-percpu_counter_sum_and_set.patch won't apply to 2.6.27 because the dirty_blocks stuff was added and generates rejects.

So if all the above is correct, I'd propose that if and when revert-percpu_counter-new-function-percpu_counter_sum_and_set.patch hits mainline, we should ask the -stable guys to directly revert

commit e8ced39d5e8911c662d4d69a342b9d053eaaac4e
Author: Mingming Cao <[email protected]>
Date: Fri Jul 11 19:27:31 2008 -0400

percpu_counter: new function percpu_counter_sum_and_set

which should be all that 2.6.27.x needs.

Agree? If so, can you please take care of getting that patch over to [email protected]? (I added the cc:stable to the diff, so there's probably nothing which you need to do..)
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 44 lines · 在 2008-12-07日的 20:42 -0800，Andrew Morton写道：

(cc stable)

Quoted 21 lines · On Sun, 7 Dec 2008 10:28:21 -0500 Theodore Tso <[email protected]> wrote:

Quoted 10 lines · Andrew Morton

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

I've looked the two patches which you've queued in the -mm branch, and they look correct to me.

The bugs fixed by these patches can potentially lead to filesystem corruption, since we ultimately use these fields to set the superblock values. This in my mind makes them stable candidates at the very least, and if we weren't so late in the 2.6.28 cycle, I'd be strongly tempted to push them to Linus as a bugfix before the merge window.

Quoted 5 lines

Andrew, any strong objections for me to grab them for the ext4 tree? Or would you rather carry them? I would prefer that they get pushed to Linus as soon as the merge window opens, which is one reason why I'd prefer carry them, but we can do this either way.

I'm planning on sending them off to Linus for 2.6.28 this week, assuming nobody can think of a plausible reason to not do that.

Now I didn't look _very_ closely at the chronology, but I think that revert-percpu-counter-clean-up-percpu_counter_sum_and_set.patch reverts a post-2.6.27 change, and is not needed in stable.

revert-percpu_counter-new-function-percpu_counter_sum_and_set.patch however reverts a pre-2.6.27 change, and should be merged into 2.6.27. This patch reverts the addition and use of percpu_counter_sum_and_set(), which is racy and can corrupt the counters.

Quoted 9 lines

However revert-percpu_counter-new-function-percpu_counter_sum_and_set.patch won't apply to 2.6.27 because the dirty_blocks stuff was added and generates rejects.

So if all the above is correct, I'd propose that if and when revert-percpu_counter-new-function-percpu_counter_sum_and_set.patch hits mainline, we should ask the -stable guys to directly revert

Agreed.

I checked 2.6.27.8, above are correct, the revert-percpu-counter-clean-up-percpu_counter_sum_and_set.patch is not needed for 2.6.27.x stable tree.

Quoted 24 lines · On Wed, 03 Dec 2008 21:24:36 +0100 Eric Dumazet <[email protected]> wrote:

Eric Dumazet a __crit :

Quoted 11 lines

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

Quoted 9 lines

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 <[email protected]>

---
lib/percpu_counter.c | 4 ++--

1 files changed, 2 insertions(+), 2 deletions(-)

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

Quoted 8 lines

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.

Quoted 3 lines

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

No, we can't do this - it will break ext4.

Take a closer look at 1f7c14c62ce63805f9574664a6c6de3633d4a354 and at e8ced39d5e8911c662d4d69a342b9d053eaaac4e.

I suggest that what we do is to revert both those changes. We can worry about the possibly-unneeded spin_lock later, in a separate patch.

It should have been a separate patch anyway. It's conceptually unrelated and is not a bugfix, but it was mixed in with a bugfix.

Mingming, this needs urgent consideration, please. Note that I had to make additional changes to ext4 due to the subsequent introduction of the dirty_blocks counter.

Please read the below changelogs carefully and check that I have got my head around this correctly - I may not have done.

What a mess.

From: Andrew Morton <[email protected]>

Revert

commit 1f7c14c62ce63805f9574664a6c6de3633d4a354
Author: Mingming Cao <[email protected]>
Date: Thu Oct 9 12:50:59 2008 -0400

percpu counter: clean up percpu_counter_sum_and_set()

Before this patch we had the following:

percpu_counter_sum(): return the percpu_counter's value

percpu_counter_sum_and_set(): return the percpu_counter's value, copying that value into the central value and zeroing the per-cpu counters before returning.

After this patch, percpu_counter_sum_and_set() has gone, and percpu_counter_sum() gets the old percpu_counter_sum_and_set() functionality.

Problem is, as Eric points out, the old percpu_counter_sum_and_set() functionality was racy and wrong. It zeroes out counters on "other" cpus, without holding any locks which will prevent races agaist updates from those other CPUS.

This patch reverts 1f7c14c62ce63805f9574664a6c6de3633d4a354. This means that percpu_counter_sum_and_set() still has the race, but percpu_counter_sum() does not.

Note that this is not a simple revert - ext4 has since started using percpu_counter_sum() for its dirty_blocks counter as well.

Note that this revert patch changes percpu_counter_sum() semantics.

Before the patch, a call to percpu_counter_sum() will bring the counter's central counter mostly up-to-date, so a following percpu_counter_read() will return a close value.

After this patch, a call to percpu_counter_sum() will leave the counter's central accumulator unaltered, so a subsequent call to percpu_counter_read() can now return a significantly inaccurate result.

If there is any code in the tree which was introduced after e8ced39d5e8911c662d4d69a342b9d053eaaac4e was merged, and which depends upon the new percpu_counter_sum() semantics, that code will break.

Reported-by Eric Dumazet <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Mingming Cao <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 fs/ext4/balloc.c               |    4 ++--
 include/linux/percpu_counter.h |   12 +++++++++---
 lib/percpu_counter.c           |    8 +++++---
 3 files changed, 16 insertions(+), 8 deletions(-)

diff -puN fs/ext4/balloc.c~revert-percpu-counter-clean-up-percpu_counter_sum_and_set fs/ext4/balloc.c

fs/ext4/balloc.c renamed from fs/ext4/balloc.c~revert-percpu-counter-clean-up-percpu_counter_sum_and_set +2 -2

@@ -609,8 +609,8 @@ int ext4_has_free_blocks(struct ext4_sb_
 
 	if (free_blocks - (nblocks + root_blocks + dirty_blocks) <
 						EXT4_FREEBLOCKS_WATERMARK) {
-		free_blocks  = percpu_counter_sum(fbc);
-		dirty_blocks = percpu_counter_sum(dbc);
+		free_blocks  = percpu_counter_sum_and_set(fbc);
+		dirty_blocks = percpu_counter_sum_and_set(dbc);
 		if (dirty_blocks < 0) {
 			printk(KERN_CRIT "Dirty block accounting "
 					"went wrong %lld\n",

diff -puN include/linux/percpu_counter.h~revert-percpu-counter-clean-up-percpu_counter_sum_and_set include/linux/percpu_counter.h

include/linux/percpu_counter.h renamed from include/linux/percpu_counter.h~revert-percpu-counter-clean-up-percpu_counter_sum_and_set +9 -3

@@ -35,7 +35,7 @@ int percpu_counter_init_irq(struct percp
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
 void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
-s64 __percpu_counter_sum(struct percpu_counter *fbc);
+s64 __percpu_counter_sum(struct percpu_counter *fbc, int set);
 
 static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 {

@@ -44,13 +44,19 @@ static inline void percpu_counter_add(st
 
 static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
 {
-	s64 ret = __percpu_counter_sum(fbc);
+	s64 ret = __percpu_counter_sum(fbc, 0);
 	return ret < 0 ? 0 : ret;
 }
 
+static inline s64 percpu_counter_sum_and_set(struct percpu_counter *fbc)
+{
+	return __percpu_counter_sum(fbc, 1);
+}
+
+
 static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
 {
-	return __percpu_counter_sum(fbc);
+	return __percpu_counter_sum(fbc, 0);
 }
 
 static inline s64 percpu_counter_read(struct percpu_counter *fbc)

diff -puN lib/percpu_counter.c~revert-percpu-counter-clean-up-percpu_counter_sum_and_set lib/percpu_counter.c

lib/percpu_counter.c renamed from lib/percpu_counter.c~revert-percpu-counter-clean-up-percpu_counter_sum_and_set +5 -3

@@ -52,7 +52,7 @@ EXPORT_SYMBOL(__percpu_counter_add);
  * Add up all the per-cpu counts, return the result.  This is a more accurate
  * but much slower version of percpu_counter_read_positive()
  */
-s64 __percpu_counter_sum(struct percpu_counter *fbc)
+s64 __percpu_counter_sum(struct percpu_counter *fbc, int set)
 {
 	s64 ret;
 	int cpu;

@@ -62,9 +62,11 @@ s64 __percpu_counter_sum(struct percpu_c
 	for_each_online_cpu(cpu) {
 		s32 *pcount = per_cpu_ptr(fbc->counters, cpu);
 		ret += *pcount;
-		*pcount = 0;
+		if (set)
+			*pcount = 0;
 	}
-	fbc->count = ret;
+	if (set)
+		fbc->count = ret;
 
 	spin_unlock(&fbc->lock);
 	return ret;

_

From: Andrew Morton <[email protected]>

Revert

commit e8ced39d5e8911c662d4d69a342b9d053eaaac4e
Author: Mingming Cao <[email protected]>
Date: Fri Jul 11 19:27:31 2008 -0400

percpu_counter: new function percpu_counter_sum_and_set

As described in

revert "percpu counter: clean up percpu_counter_sum_and_set()"

the new percpu_counter_sum_and_set() is racy against updates to the cpu-local accumulators on other CPUs. Revert that change.

This means that ext4 will be slow again. But correct.

Reported-by Eric Dumazet <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Mingming Cao <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 fs/ext4/balloc.c               |    4 ++--
 include/linux/percpu_counter.h |   12 +++---------
 lib/percpu_counter.c           |    7 +------
 3 files changed, 6 insertions(+), 17 deletions(-)

diff -puN fs/ext4/balloc.c~revert-percpu_counter-new-function-percpu_counter_sum_and_set fs/ext4/balloc.c

fs/ext4/balloc.c renamed from fs/ext4/balloc.c~revert-percpu_counter-new-function-percpu_counter_sum_and_set +2 -2

@@ -609,8 +609,8 @@ int ext4_has_free_blocks(struct ext4_sb_
 
 	if (free_blocks - (nblocks + root_blocks + dirty_blocks) <
 						EXT4_FREEBLOCKS_WATERMARK) {
-		free_blocks  = percpu_counter_sum_and_set(fbc);
-		dirty_blocks = percpu_counter_sum_and_set(dbc);
+		free_blocks  = percpu_counter_sum_positive(fbc);
+		dirty_blocks = percpu_counter_sum_positive(dbc);
 		if (dirty_blocks < 0) {
 			printk(KERN_CRIT "Dirty block accounting "
 					"went wrong %lld\n",

diff -puN include/linux/percpu_counter.h~revert-percpu_counter-new-function-percpu_counter_sum_and_set include/linux/percpu_counter.h

include/linux/percpu_counter.h renamed from include/linux/percpu_counter.h~revert-percpu_counter-new-function-percpu_counter_sum_and_set +3 -9

@@ -35,7 +35,7 @@ int percpu_counter_init_irq(struct percp
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
 void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
-s64 __percpu_counter_sum(struct percpu_counter *fbc, int set);
+s64 __percpu_counter_sum(struct percpu_counter *fbc);
 
 static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 {

@@ -44,19 +44,13 @@ static inline void percpu_counter_add(st
 
 static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
 {
-	s64 ret = __percpu_counter_sum(fbc, 0);
+	s64 ret = __percpu_counter_sum(fbc);
 	return ret < 0 ? 0 : ret;
 }
 
-static inline s64 percpu_counter_sum_and_set(struct percpu_counter *fbc)
-{
-	return __percpu_counter_sum(fbc, 1);
-}
-
-
 static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
 {
-	return __percpu_counter_sum(fbc, 0);
+	return __percpu_counter_sum(fbc);
 }
 
 static inline s64 percpu_counter_read(struct percpu_counter *fbc)

diff -puN lib/percpu_counter.c~revert-percpu_counter-new-function-percpu_counter_sum_and_set lib/percpu_counter.c

lib/percpu_counter.c renamed from lib/percpu_counter.c~revert-percpu_counter-new-function-percpu_counter_sum_and_set +1 -6

@@ -52,7 +52,7 @@ EXPORT_SYMBOL(__percpu_counter_add);
  * Add up all the per-cpu counts, return the result.  This is a more accurate
  * but much slower version of percpu_counter_read_positive()
  */
-s64 __percpu_counter_sum(struct percpu_counter *fbc, int set)
+s64 __percpu_counter_sum(struct percpu_counter *fbc)
 {
 	s64 ret;
 	int cpu;

@@ -62,12 +62,7 @@ s64 __percpu_counter_sum(struct percpu_c
 	for_each_online_cpu(cpu) {
 		s32 *pcount = per_cpu_ptr(fbc->counters, cpu);
 		ret += *pcount;
-		if (set)
-			*pcount = 0;
 	}
-	if (set)
-		fbc->count = ret;
-
 	spin_unlock(&fbc->lock);
 	return ret;
 }

_

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>