[PATCH 1/4]percpu_counter: make API return consistent value

[PATCH 1/4]percpu_counter: make API return consistent value

[Shaohua Li]

the percpu_counter_*_positive() API SMP and !SMP aren't consistent. From
the API name, we should return a non-negative value for them.
Also if count < 0, returns 0 instead of 1 for *read_positive().

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

---
 include/linux/percpu_counter.h |   52 ++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

Index: linux/include/linux/percpu_counter.h
===================================================================
--- linux.orig/include/linux/percpu_counter.h	2011-04-12 15:48:42.000000000 +0800
+++ linux/include/linux/percpu_counter.h	2011-04-12 15:48:44.000000000 +0800
@@ -47,12 +47,6 @@ static inline void percpu_counter_add(st
 	__percpu_counter_add(fbc, amount, percpu_counter_batch);
 }
 
-static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
-{
-	s64 ret = __percpu_counter_sum(fbc);
-	return ret < 0 ? 0 : ret;
-}
-
 static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
 {
 	return __percpu_counter_sum(fbc);
@@ -63,21 +57,6 @@ static inline s64 percpu_counter_read(st
 	return fbc->count;
 }
 
-/*
- * It is possible for the percpu_counter_read() to return a small negative
- * number for some counter which should never be negative.
- *
- */
-static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
-{
-	s64 ret = fbc->count;
-
-	barrier();		/* Prevent reloads of fbc->count */
-	if (ret >= 0)
-		return ret;
-	return 1;
-}
-
 static inline int percpu_counter_initialized(struct percpu_counter *fbc)
 {
 	return (fbc->counters != NULL);
@@ -133,16 +112,6 @@ static inline s64 percpu_counter_read(st
 	return fbc->count;
 }
 
-static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
-{
-	return fbc->count;
-}
-
-static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
-{
-	return percpu_counter_read_positive(fbc);
-}
-
 static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
 {
 	return percpu_counter_read(fbc);
@@ -170,4 +139,25 @@ static inline void percpu_counter_sub(st
 	percpu_counter_add(fbc, -amount);
 }
 
+/*
+ * It is possible for the percpu_counter_read() to return a small negative
+ * number for some counter which should never be negative.
+ *
+ */
+static inline s64 percpu_counter_read_positive(struct percpu_counter *fbc)
+{
+	s64 ret = percpu_counter_read(fbc);
+
+	barrier();		/* Prevent reloads of fbc->count */
+	if (ret >= 0)
+		return ret;
+	return 0;
+}
+
+static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
+{
+	s64 ret = percpu_counter_sum(fbc);
+	return ret < 0 ? 0 : ret;
+}
+
 #endif /* _LINUX_PERCPU_COUNTER_H */

Re: [PATCH 1/4]percpu_counter: make API return consistent value

[Tejun Heo]

Hello,

First of all, please somehow link patches of the same series.  Either
write a head message and make all the patches replies to it
(preferred) or chain reply the patches (only when the number of
patches is small).

On Tue, Apr 12, 2011 at 04:03:57PM +0800, Shaohua Li wrote:
> the percpu_counter_*_positive() API SMP and !SMP aren't consistent. From
> the API name, we should return a non-negative value for them.
> Also if count < 0, returns 0 instead of 1 for *read_positive().

Ummm, on UP, the counters cannot be positive.  The _positive interface
is there to make it easier to cope with deviations introduced by
unsynchronized modifications by different CPUs.  On UP, such
deviations don't happen at all so _positive interface is the same as
the counterpart without the postfix.

Thanks.

-- 
tejun

Re: [PATCH 1/4]percpu_counter: make API return consistent value

[Shaohua Li]

On Wed, 2011-04-13 at 02:49 +0800, Tejun Heo wrote:
> Hello,
> 
> First of all, please somehow link patches of the same series.  Either
> write a head message and make all the patches replies to it
> (preferred) or chain reply the patches (only when the number of
> patches is small).
> 
> On Tue, Apr 12, 2011 at 04:03:57PM +0800, Shaohua Li wrote:
> > the percpu_counter_*_positive() API SMP and !SMP aren't consistent. From
> > the API name, we should return a non-negative value for them.
> > Also if count < 0, returns 0 instead of 1 for *read_positive().
> 
> Ummm, on UP, the counters cannot be positive.
s/positive/negative?

>   The _positive interface
> is there to make it easier to cope with deviations introduced by
> unsynchronized modifications by different CPUs.  On UP, such
> deviations don't happen at all so _positive interface is the same as
> the counterpart without the postfix.
I'm confused. the counter could be negative, we have *_dec, *_sub.

Thanks,
Shaohua

Re: [PATCH 1/4]percpu_counter: make API return consistent value

[Tejun Heo]

Hello,

On Wed, Apr 13, 2011 at 09:24:15AM +0800, Shaohua Li wrote:
> > Ummm, on UP, the counters cannot be positive.
> s/positive/negative?

Yeap.

> >   The _positive interface
> > is there to make it easier to cope with deviations introduced by
> > unsynchronized modifications by different CPUs.  On UP, such
> > deviations don't happen at all so _positive interface is the same as
> > the counterpart without the postfix.
> I'm confused. the counter could be negative, we have *_dec, *_sub.

Yes it can technically but I was referring to the intent of the API.
The whole percpu counter is supposed to track a positive number.  The
_positive interface is there just to cope with deviations caused by
distribution over multiple per-cpu counters, which can't happen on UP.
When the counter can actually be negative, the API doesn't make whole
lot of sense.  I agree it's poorly documented.

Thanks.

-- 
tejun