2011-04-13 08:06:32

by Shaohua Li

Subject: [patch v2 3/4] percpu_counter: fix code for 32bit systems

percpu_counter.counter is a 's64'. Accessing it in 32-bit system is racing.
we need some locking to protect it otherwise some very wrong value could be
accessed.

Signed-off-by: Shaohua Li <[email protected]>
---
include/linux/percpu_counter.h | 48 ++++++++++++++++++++++++++++++-----------
1 file changed, 36 insertions(+), 12 deletions(-)

Index: linux/include/linux/percpu_counter.h
===================================================================
--- linux.orig/include/linux/percpu_counter.h 2011-04-13 13:21:21.000000000 +0800
+++ linux/include/linux/percpu_counter.h 2011-04-13 13:27:22.000000000 +0800
@@ -60,7 +60,16 @@ static inline s64 percpu_counter_sum(str

static inline s64 percpu_counter_read(struct percpu_counter *fbc)
{
+#if BITS_PER_LONG == 32
+ s64 count;
+ unsigned long flags;
+ spin_lock_irqsave(&fbc->lock, flags);
+ count = fbc->count;
+ spin_unlock_irqrestore(&fbc->lock, flags);
+ return count;
+#else
return fbc->count;
+#endif
}

/*
@@ -70,7 +79,7 @@ static inline s64 percpu_counter_read(st
*/
static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
{
- s64 ret = fbc->count;
+ s64 ret = percpu_counter_read(fbc);

barrier(); /* Prevent reloads of fbc->count */
if (ret >= 0)
@@ -89,9 +98,20 @@ struct percpu_counter {
s64 count;
};

-static inline int percpu_counter_init(struct percpu_counter *fbc, s64 amount)
+static inline void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
{
+#if BITS_PER_LONG == 32
+ preempt_disable();
fbc->count = amount;
+ preempt_enable();
+#else
+ fbc->count = amount;
+#endif
+}
+
+static inline int percpu_counter_init(struct percpu_counter *fbc, s64 amount)
+{
+ percpu_counter_set(fbc, amount);
return 0;
}

@@ -99,16 +119,25 @@ static inline void percpu_counter_destro
{
}

-static inline void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
+static inline s64 percpu_counter_read(struct percpu_counter *fbc)
{
- fbc->count = amount;
+#if BITS_PER_LONG == 32
+ s64 count;
+ preempt_disable();
+ count = fbc->count;
+ preempt_enable();
+ return count;
+#else
+ return fbc->count;
+#endif
}

static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
{
- if (fbc->count > rhs)
+ s64 count = percpu_counter_read(fbc);
+ if (count > rhs)
return 1;
- else if (fbc->count < rhs)
+ else if (count < rhs)
return -1;
else
return 0;
@@ -128,18 +157,13 @@ __percpu_counter_add(struct percpu_count
percpu_counter_add(fbc, amount);
}

-static inline s64 percpu_counter_read(struct percpu_counter *fbc)
-{
- return fbc->count;
-}
-
static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
{
/*
* percpu_counter is intended to track positive number. In UP case, the
* number should never be negative.
*/
- return fbc->count;
+ return percpu_counter_read(fbc);
}

static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)

2011-04-13 19:04:22

by Tejun Heo

Subject: Re: [patch v2 3/4] percpu_counter: fix code for 32bit systems

On Wed, Apr 13, 2011 at 03:57:18PM +0800, [email protected] wrote:
> static inline s64 percpu_counter_read(struct percpu_counter *fbc)
> {
> +#if BITS_PER_LONG == 32
> + s64 count;
> + unsigned long flags;
> + spin_lock_irqsave(&fbc->lock, flags);
> + count = fbc->count;
> + spin_unlock_irqrestore(&fbc->lock, flags);
> + return count;
> +#else
> return fbc->count;
> +#endif

I don't think this is safe. The possible deadlock scenario was
percpu_counter_read() being called from irq context and adding irq
locking to percpu_counter_read() doesn't change that in any way. You
should be changing locking in other places. Given that the next patch
would make this dancing with locks all pointless, my suggestion is to
drop this patch and proceed with atomic64_t conversion directly and
note that the conversion also removes possible 32bit deviation on
32bit archs.

Thanks.

--
tejun