[PATCH] cpu hotplug fix

[PATCH] cpu hotplug fix

[Anton Blanchard]

start_cpu_timer was changed to __init a few hours before the cpu hotplug
patches went in. With cpu hotplug it can be called at any time, so fix
this.

Anton

===== slab.c 1.125 vs edited =====
--- 1.125/mm/slab.c	Wed Mar 17 13:10:10 2004
+++ edited/slab.c	Sat Mar 20 17:34:57 2004
@@ -576,7 +576,7 @@
  * Add the CPU number into the expiry time to minimize the possibility of the
  * CPUs getting into lockstep and contending for the global cache chain lock.
  */
-static void __init start_cpu_timer(int cpu)
+static void start_cpu_timer(int cpu)
 {
 	struct timer_list *rt = &per_cpu(reap_timers, cpu);
 

Re: [PATCH] cpu hotplug fix

[Christoph Hellwig]

On Sat, Mar 20, 2004 at 05:36:42PM +1100, Anton Blanchard wrote:
> 
> start_cpu_timer was changed to __init a few hours before the cpu hotplug
> patches went in. With cpu hotplug it can be called at any time, so fix
> this.

So we're wasting memory due to the few machines supporting hot-plug cpus
in slab now?  What about adding __cpuinit ala __devinit instead?

Re: [PATCH] cpu hotplug fix

[Rusty Russell]

On Sat, 2004-03-20 at 18:40, Christoph Hellwig wrote:
> On Sat, Mar 20, 2004 at 05:36:42PM +1100, Anton Blanchard wrote:
> > 
> > start_cpu_timer was changed to __init a few hours before the cpu hotplug
> > patches went in. With cpu hotplug it can be called at any time, so fix
> > this.
> 
> So we're wasting memory due to the few machines supporting hot-plug cpus
> in slab now?  What about adding __cpuinit ala __devinit instead?

Just use __devinit.

More importantly, Christoph, you're missing the forest through the
trees: what the hell is start_cpu_timer trying to do?

Name: Remove Strange start_cpu_timer Code
Status: Untested

Why is start_cpu_timer checking for fn being NULL?  And why isn't
every CPU's reap_timer initialized in cpucache_init (which is an
__init function).

Clean up this mess by just starting the timer in start_cpu_timer.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .15051-linux-2.6.5-rc2/mm/slab.c .15051-linux-2.6.5-rc2.updated/mm/slab.c
--- .15051-linux-2.6.5-rc2/mm/slab.c	2004-03-20 21:21:33.000000000 +1100
+++ .15051-linux-2.6.5-rc2.updated/mm/slab.c	2004-03-20 21:45:06.000000000 +1100
@@ -576,17 +576,12 @@ static void __slab_error(const char *fun
  * Add the CPU number into the expiry time to minimize the possibility of the
  * CPUs getting into lockstep and contending for the global cache chain lock.
  */
-static void __init start_cpu_timer(int cpu)
+static inline void start_cpu_timer(int cpu)
 {
 	struct timer_list *rt = &per_cpu(reap_timers, cpu);
 
-	if (rt->function == NULL) {
-		init_timer(rt);
-		rt->expires = jiffies + HZ + 3*cpu;
-		rt->data = cpu;
-		rt->function = reap_timer_fnc;
-		add_timer_on(rt, cpu);
-	}
+	rt->expires = jiffies + HZ + 3*cpu;
+	add_timer_on(rt, cpu);
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -798,7 +793,13 @@ int __init cpucache_init(void)
 	 * Register the timers that return unneeded
 	 * pages to gfp.
 	 */
-	for (cpu = 0; cpu < NR_CPUS; cpu++) {
+	for_each_cpu(cpu) {
+		struct timer_list *rt = &per_cpu(reap_timers, cpu);
+
+		init_timer(rt);
+		rt->data = cpu;
+		rt->function = reap_timer_fnc;
+		
 		if (cpu_online(cpu))
 			start_cpu_timer(cpu);
 	}



-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [PATCH] cpu hotplug fix

[Andrew Morton]

Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Name: Remove Strange start_cpu_timer Code
>  Status: Untested
> 
>  Why is start_cpu_timer checking for fn being NULL?  And why isn't
>  every CPU's reap_timer initialized in cpucache_init (which is an
>  __init function).
> 
>  Clean up this mess by just starting the timer in start_cpu_timer.

oy.

Program received signal SIGTRAP, Trace/breakpoint trap.
0xc0128e32 in add_timer_on (timer=0xc120d880, cpu=1) at kernel/timer.c:225
225             BUG_ON(timer_pending(timer) || !timer->function);
(gdb) bt
#0  0xc0128e32 in add_timer_on (timer=0xc120d880, cpu=1) at kernel/timer.c:225
#1  0xc01446b5 in cpuup_callback (nfb=0xc0472800, action=4, hcpu=0x1) at mm/slab.c:595
#2  0xc012dc67 in notifier_call_chain (n=0x4, val=2, v=0x1) at kernel/sys.c:169
#3  0xc0134b63 in cpu_up (cpu=1) at kernel/cpu.c:192
#4  0xc05e0650 in smp_init () at init/main.c:364
#5  0xc0103161 in init (unused=0x0) at init/main.c:626
#6  0xc010728d in kernel_thread_helper () at arch/i386/kernel/process.c:246
(gdb) p timer->function
$1 = (void (*)(long unsigned int)) 0


I'll simply mark start_cpu_timer() as __devinit.

Re: [PATCH] cpu hotplug fix

[Rusty Russell]

On Sun, 2004-03-21 at 10:12, Andrew Morton wrote:
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> >
> > Name: Remove Strange start_cpu_timer Code
> >  Status: Untested
> oy.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.

Yep.  This one is better, and actually tested.

Name: Remove Strange start_cpu_timer Code
Status: Tested on 2.6.5-rc2-bk1

start_cpu_timer is called once for every online CPU, then again in the
cpu up callback, hence the rt->function re-entrance check.  Just call
it manually for the boot cpu, and use the notifier for the others.

Perhaps at one stage this code was run before timers were available:
this is no longer true (timers are initialized as part of scheduler
startup).

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.5-rc2-bk1/mm/slab.c working-2.6.5-rc2-bk1-start_cpu_timer-cleanup/mm/slab.c
--- linux-2.6.5-rc2-bk1/mm/slab.c	2004-03-20 21:21:33.000000000 +1100
+++ working-2.6.5-rc2-bk1-start_cpu_timer-cleanup/mm/slab.c	2004-03-22 14:41:07.000000000 +1100
@@ -576,13 +576,11 @@ static void __slab_error(const char *fun
 {
 	struct timer_list *rt = &per_cpu(reap_timers, cpu);
 
-	if (rt->function == NULL) {
-		init_timer(rt);
-		rt->expires = jiffies + HZ + 3*cpu;
-		rt->data = cpu;
-		rt->function = reap_timer_fnc;
-		add_timer_on(rt, cpu);
-	}
+	init_timer(rt);
+	rt->data = cpu;
+	rt->function = reap_timer_fnc;
+	rt->expires = jiffies + HZ + 3*cpu;
+	add_timer_on(rt, cpu);
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -594,11 +592,8 @@ static void stop_cpu_timer(int cpu)
 {
 	struct timer_list *rt = &per_cpu(reap_timers, cpu);
 
-	if (rt->function) {
-		del_timer_sync(rt);
-		WARN_ON(timer_pending(rt));
-		rt->function = NULL;
-	}
+	del_timer_sync(rt);
+	WARN_ON(timer_pending(rt));
 }
 #endif
 
@@ -779,35 +774,14 @@ void __init kmem_cache_init(void)
 	/* Done! */
 	g_cpucache_up = FULL;
 
+	start_cpu_timer(smp_processor_id());
+
 	/* Register a cpu startup notifier callback
 	 * that initializes ac_data for all new cpus
 	 */
 	register_cpu_notifier(&cpucache_notifier);
-	
-
-	/* The reap timers are started later, with a module init call:
-	 * That part of the kernel is not yet operational.
-	 */
 }
 
-int __init cpucache_init(void)
-{
-	int cpu;
-
-	/* 
-	 * Register the timers that return unneeded
-	 * pages to gfp.
-	 */
-	for (cpu = 0; cpu < NR_CPUS; cpu++) {
-		if (cpu_online(cpu))
-			start_cpu_timer(cpu);
-	}
-
-	return 0;
-}
-
-__initcall(cpucache_init);
-
 /*
  * Interface to system's page allocator. No need to hold the cache-lock.
  *


-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell