Re: [patch] Avoid use of spinlock for percpu_counter

[Ravikiran G Thirumalai <kiran@scalex86.org>]

On Thu, Jan 26, 2006 at 03:17:35PM +0100, Eric Dumazet wrote:
> Ravikiran G Thirumalai a écrit :
> >The spinlock in struct percpu_counter protects just one counter.  It's
> >not obvious why it was done this way (I am guessing it was because earlier,
> >atomic_t was guaranteed 24 bits only on some arches).  Since we have
> >atomic_long_t now, I don't see why this cannot be replaced with an 
> >atomic_t.
> >
> >Comments?
> 
> Yes this makes sense.
> 
> Furthermore, we could try to fix 'struct percpu_counter' management (if 
> SMP) if alloc_percpu(long) call done in percpu_counter_init() fails. This 
> is currently ignored and can crash.
> Something like (hybrid patch, to get the idea) :
> 
> --- a/mm/swap.c 2006-01-26 15:58:42.000000000 +0100
> +++ b/mm/swap.c 2006-01-26 16:00:54.000000000 +0100
> @@ -472,9 +472,12 @@
>  {
>         long count;
>         long *pcount;
> -       int cpu = get_cpu();
> 
> -       pcount = per_cpu_ptr(fbc->counters, cpu);
> +       if (unlikely(fbc->counters == NULL)) {
> +               atomic_long_add(amount, &fbc->count);
> +               return;

I don't know if adding another branch to the fast path is a good idea, would
it not be better if this was handled by returning an error at
percpu_counter_init?  If we are in agreement, then I can make a patch for
the same.

Thanks,
Kiran