xfs
[Top] [All Lists]

Re: [PATCH 2/5] percpu_counter: avoid potential underflow in add_unless_

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 2/5] percpu_counter: avoid potential underflow in add_unless_lt
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 23 Dec 2010 17:39:57 +1100
Cc: XFS Mailing List <xfs@xxxxxxxxxxx>
In-reply-to: <1293076587.2408.431.camel@doink>
References: <1293076587.2408.431.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
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

<Prev in Thread] Current Thread [Next in Thread>