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);
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?
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
Rusty Russell <[email protected]> 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.
On Sun, 2004-03-21 at 10:12, Andrew Morton wrote:
> Rusty Russell <[email protected]> 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