Skip to contentSkip to conversation

cpu hotplug fix

PATCH In review Anton Blanchard 4 participants 5 messages Mar 20, 2004 – Mar 22, 2004 Memory management

lore
Share & tools
lore

Anton Blanchard

author
#0 lore raw

PATCH 1/1 cpu hotplug fix

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 =====

slab.c renamed from mm/slab.c +1 -1
@@ -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);
#1 lore raw
Quoted 4 lines · 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.

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

#2 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · 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.

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.

mm/slab.c +10 -9
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
@@ -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);
 	}
Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
#3 lore raw
Quoted 9 lines · Rusty Russell

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.

#4 lore raw
Quoted 7 lines · Andrew Morton
Quoted 3 lines · Rusty Russell

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).

mm/slab.c +9 -35
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
@@ -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.
  *
Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell