Re: [PATCH] percpu_counter: FBC_BATCH should be a variable

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 08 Dec 2008 09:05:04 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:

> Andrew Morton a __crit :
> > On Sun, 07 Dec 2008 19:30:10 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:
> > 
> >>> Do
> >>>
> >>> 	$EDITOR  $(grep -l hotcpu_notifier */*.c)
> >>>
> >>> and you'll see lots of code gets it right, and lots of code gets it wrong.
> >> I see nothing interesting, I must be blind.
> >>
> >> lib/percpu_counter.c: In function 'percpu_counter_startup':
> >> lib/percpu_counter.c:158: error: 'percpu_counter_hotcpu_callback' undeclared (first use in this function)
> >> lib/percpu_counter.c:158: error: (Each undeclared identifier is reported only once
> >> lib/percpu_counter.c:158: error: for each function it appears in.)
> >> make[1]: *** [lib/percpu_counter.o] Error 1
> > 
> > Perhaps you still had percpu_counter_hotcpu_callback inside #ifdef.
> > 
> > That a look at kernel/workqueue.c, fs/buffer.c.  No #ifdef CONFIG_HOTPLUG_CPU
> > needed at all.
> 
> We still need some #ifdef or we also must also delete them around "struct list_head list"
> in include/linux/percpu_counter.h and grow struct percpu_counter.
> 
> lib/percpu_counter.c: In function 'percpu_counter_hotcpu_callback':
> lib/percpu_counter.c:137: error: 'struct percpu_counter' has no member named 'list'
> lib/percpu_counter.c:137: warning: type defaults to 'int' in declaration of '__mptr'
> lib/percpu_counter.c:137: warning: initialization from incompatible pointer type
> ...
> 

Yes, this conditionality:

struct percpu_counter {
	spinlock_t lock;
	s64 count;
#ifdef CONFIG_HOTPLUG_CPU
	struct list_head list;	/* All percpu_counters are on a list */
#endif
	s32 *counters;
};

mucked up our nice scheme.  Oh well.

> +static void compute_batch_value(void)
> +{
> +	int nr = num_online_cpus();
> +
> +	percpu_counter_batch = max(32, nr*2);
> +}
> +
>  static int __cpuinit percpu_counter_hotcpu_callback(struct notifier_block *nb,
>  					unsigned long action, void *hcpu)
>  {
> +#ifdef CONFIG_HOTPLUG_CPU
>  	unsigned int cpu;
>  	struct percpu_counter *fbc;
>  
> +	compute_batch_value();
>  	if (action != CPU_DEAD)
>  		return NOTIFY_OK;
>  
> @@ -137,13 +146,14 @@ static int __cpuinit percpu_counter_hotcpu_callback(struct notifier_block *nb,
>  		spin_unlock_irqrestore(&fbc->lock, flags);
>  	}
>  	mutex_unlock(&percpu_counters_lock);
> +#endif
>  	return NOTIFY_OK;
>  }
>  
>  static int __init percpu_counter_startup(void)
>  {
> +	compute_batch_value();
>  	hotcpu_notifier(percpu_counter_hotcpu_callback, 0);
>  	return 0;
>  }
>  module_init(percpu_counter_startup);

compute_batch_value() can be __cpuinit, but I think we're close enough
here ;)