Re: [RFC PATCH] percpu_counters: Add new function percpu_counter_sum_and_sub

[Mingming Cao <cmm@us.ibm.com>]

在 2008-08-22五的 19:04 +0530，Aneesh Kumar K.V写道：
> percpu_counter_sum_and_sub(struct percpu_counter *fbc, s64 amount)
> 
> This will be later used by ext4 code. It adds up the local
> per cpu counter values to global count and subtract amount
> from the gloabl count if gloabl count is > amount. This is
> used during block resrevation to check within a lock we
> have sufficient free blocks if so also claim the blocks.
> 

I'd  suggest repost the patch to lkml for this new interface change when
this patch is ready. It would be nice to have ext4 part of change which
use this interface being sent together in a series.


Patch looks fine except a small place, see comment below.

You could add Reviewed-by: Mingming Cao <cmm@us.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>  include/linux/percpu_counter.h |   17 +++++++++++++++++
>  lib/percpu_counter.c           |   27 +++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index af485b1..978db67 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -36,6 +36,7 @@ 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_and_sub(struct percpu_counter *fbc, s64 amount);
> 
>  static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
>  {
> @@ -53,6 +54,12 @@ static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
>  	return __percpu_counter_sum(fbc);
>  }
> 
> +static inline int percpu_counter_sum_and_sub(struct percpu_counter *fbc,
> +						s64 amount)
> +{
> +	return __percpu_counter_sum_and_sub(fbc, amount);
> +}
> +
>  #if BITS_PER_LONG == 64
>  static inline s64 fbc_count(struct percpu_counter *fbc)
>  {
> @@ -146,6 +153,16 @@ static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
>  	return percpu_counter_read(fbc);
>  }
> 
> +static inline int percpu_counter_sum_and_sub(struct percpu_counter *fbc,
> +						s64 amount)
> +{
> +	if (fbc->count >= amount) {
> +		percpu_counter_sub(fbc, amount);
> +		return 0;
> +	}
> +	return -E2BIG;
> +}
> +
>  #endif	/* CONFIG_SMP */
> 
>  static inline void percpu_counter_inc(struct percpu_counter *fbc)
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index a866389..0336062 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -71,6 +71,33 @@ s64 __percpu_counter_sum(struct percpu_counter *fbc)
>  }
>  EXPORT_SYMBOL(__percpu_counter_sum);
> 
> + /*
> +  * Add up all the per-cpu counts. If the result is greater than
> +  * amount subtract amount from result and return 0. Otherwise return
> +  * -E2BIG
> +  */
> +s64 __percpu_counter_sum_and_sub(struct percpu_counter *fbc, s64 amount)

Seems it returns 0 for success, and error code on failur. In this case,
should the return value type as s64?

I noticed previous ly  the caller returns "int" type, so this might be
just a copy-and-paste error.

+static inline int percpu_counter_sum_and_sub(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;
> +	}
> +	if (ret >= amount) {
> +		fbc->count = ret - amount;
> +		spin_unlock(&fbc->lock);
> +		return 0;
> +	}
> +	spin_unlock(&fbc->lock);
> +	return -E2BIG;
> +}
> +EXPORT_SYMBOL(__percpu_counter_sum_and_sub);
> +
>  static struct lock_class_key percpu_counter_irqsafe;
> 
>  int percpu_counter_init(struct percpu_counter *fbc, s64 amount)

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