On Wed, Dec 22, 2010 at 09:56:27PM -0600, Alex Elder wrote:
> In __percpu_counter_add_unless_lt(), an assumption is made that
> under certain conditions it's possible to determine that an amount
> can be safely added to a counter, possibly without having to acquire
> the lock. This assumption is not valid, however.
>
> These lines encode the assumption:
> if (count + amount > threshold + error) {
> __percpu_counter_add(fbc, amount, batch);
>
> Inside __percpu_counter_add(), the addition is performed
> without acquiring the lock if the *sum* of the batch size
> and the CPU-local delta is within the batch size. Otherwise
> it does the addition after acquiring the lock.
>
> The problem is that *that* sum may actually end up being greater
> than the batch size, forcing the addition to be performed under
> protection of the lock. And by the time the lock is acquired, the
> value of fbc->count may have been updated such that adding the given
> amount allows the result to go negative.
>
> Fix this by open-coding the portion of the __percpu_counter_add()
> that avoids the lock.
>
> Signed-off-by: Alex Elder <aelder@xxxxxxx>
>
> ---
> lib/percpu_counter.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> Index: b/lib/percpu_counter.c
> ===================================================================
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -243,9 +243,14 @@ int __percpu_counter_add_unless_lt(struc
> * we can safely add, and might be able to avoid locking.
> */
> if (count + amount > threshold + error) {
> - __percpu_counter_add(fbc, amount, batch);
> - ret = 1;
> - goto out;
> + s32 *pcount = this_cpu_ptr(fbc->counters);
> +
> + count = *pcount + amount;
> + if (abs(count) < batch) {
> + *pcount = count;
> + ret = 1;
> + goto out;
> + }
> }
The problem with this is that it never zeros pcount. That means
after a bunch of increments or decrements, abs(*pcount) == 31,
and ever further increment/decrement will drop through to the path
that requires locking. Then we simply have a very expensive global
counter.
We need to take the lock to zero the pcount value because it has to
be added to fbc->count. i.e. if you want this path to remain mostly
lockless, then it needs to do exactly what __percpu_counter_add()
does....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
