SLAB uses get/put_online_cpus() which use a mutex which is itself
only initialized when cpu_hotplug_init() is called. Currently
we hang suring boot in SLAB due to doing that too late. This
moves the call to cpu_hotplug_init() to before mm_init() (it
should be safe to call that early).
This fixes boot with SLAB on some PowerPC machines.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
Sachin, James, do that fix the boot failures you've been seeing ?
Index: linux-work/init/main.c
===================================================================
--- linux-work.orig/init/main.c 2009-06-23 10:17:46.000000000 +1000
+++ linux-work/init/main.c 2009-06-23 10:20:30.000000000 +1000
@@ -608,7 +608,18 @@ asmlinkage void __init start_kernel(void
vfs_caches_init_early();
sort_main_extable();
trap_init();
+
+ /*
+ * This initializes the mutex used by get/put_online_cpus()
+ * which is used by SLAB
+ */
+ cpu_hotplug_init();
+
+ /*
+ * Initialize the page allocator, SL*B and vmalloc
+ */
mm_init();
+
/*
* Set up the scheduler prior starting any interrupts (such as the
* timer interrupt). Full topology setup happens at smp_init()
@@ -678,7 +689,6 @@ asmlinkage void __init start_kernel(void
#endif
page_cgroup_init();
enable_debug_pagealloc();
- cpu_hotplug_init();
kmemtrace_init();
kmemleak_init();
debug_objects_mem_init();
On Tue, 2009-06-23 at 10:31 +1000, Benjamin Herrenschmidt wrote:
> SLAB uses get/put_online_cpus() which use a mutex which is itself
> only initialized when cpu_hotplug_init() is called. Currently
> we hang suring boot in SLAB due to doing that too late. This
> moves the call to cpu_hotplug_init() to before mm_init() (it
> should be safe to call that early).
>
> This fixes boot with SLAB on some PowerPC machines.
>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> ---
>
> Sachin, James, do that fix the boot failures you've been seeing ?
Yes, that seems to fix it for me.
James
On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
> + /*
> + * This initializes the mutex used by get/put_online_cpus()
> + * which is used by SLAB
> + */
> + cpu_hotplug_init();
Oh fudge it.
Why not get rid of that stupid thing, rather than have that subtle
dependency and a comment for it.
IOW, does this simpler alternative also work?
When given the choice of simplifying and removing code, always do so.
Static initializers are much nicer than having to worry about when
something gets initialized, no?
Linus
---
include/linux/cpu.h | 5 -----
init/main.c | 1 -
kernel/cpu.c | 13 +++++--------
3 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 2643d84..4d668e0 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -69,7 +69,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
int cpu_up(unsigned int cpu);
void notify_cpu_starting(unsigned int cpu);
-extern void cpu_hotplug_init(void);
extern void cpu_maps_update_begin(void);
extern void cpu_maps_update_done(void);
@@ -84,10 +83,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
{
}
-static inline void cpu_hotplug_init(void)
-{
-}
-
static inline void cpu_maps_update_begin(void)
{
}
diff --git a/init/main.c b/init/main.c
index 09131ec..4870dfe 100644
--- a/init/main.c
+++ b/init/main.c
@@ -678,7 +678,6 @@ asmlinkage void __init start_kernel(void)
#endif
page_cgroup_init();
enable_debug_pagealloc();
- cpu_hotplug_init();
kmemtrace_init();
kmemleak_init();
debug_objects_mem_init();
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 395b697..8ce1004 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -34,14 +34,11 @@ static struct {
* an ongoing cpu hotplug operation.
*/
int refcount;
-} cpu_hotplug;
-
-void __init cpu_hotplug_init(void)
-{
- cpu_hotplug.active_writer = NULL;
- mutex_init(&cpu_hotplug.lock);
- cpu_hotplug.refcount = 0;
-}
+} cpu_hotplug = {
+ .active_writer = NULL,
+ .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
+ .refcount = 0,
+};
#ifdef CONFIG_HOTPLUG_CPU
On Mon, 2009-06-22 at 19:29 -0700, Linus Torvalds wrote:
>
> On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
>
> > + /*
> > + * This initializes the mutex used by get/put_online_cpus()
> > + * which is used by SLAB
> > + */
> > + cpu_hotplug_init();
>
> Oh fudge it.
>
> Why not get rid of that stupid thing, rather than have that subtle
> dependency and a comment for it.
>
> IOW, does this simpler alternative also work?
You mean static init ? Sure, it should do.
> When given the choice of simplifying and removing code, always do so.
Yup. Agreed. Thought about it... got shy :-)
> Static initializers are much nicer than having to worry about when
> something gets initialized, no?
I'll test that asap.
> Linus
>
> ---
> include/linux/cpu.h | 5 -----
> init/main.c | 1 -
> kernel/cpu.c | 13 +++++--------
> 3 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 2643d84..4d668e0 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -69,7 +69,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
>
> int cpu_up(unsigned int cpu);
> void notify_cpu_starting(unsigned int cpu);
> -extern void cpu_hotplug_init(void);
> extern void cpu_maps_update_begin(void);
> extern void cpu_maps_update_done(void);
>
> @@ -84,10 +83,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
> {
> }
>
> -static inline void cpu_hotplug_init(void)
> -{
> -}
> -
> static inline void cpu_maps_update_begin(void)
> {
> }
> diff --git a/init/main.c b/init/main.c
> index 09131ec..4870dfe 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -678,7 +678,6 @@ asmlinkage void __init start_kernel(void)
> #endif
> page_cgroup_init();
> enable_debug_pagealloc();
> - cpu_hotplug_init();
> kmemtrace_init();
> kmemleak_init();
> debug_objects_mem_init();
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 395b697..8ce1004 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -34,14 +34,11 @@ static struct {
> * an ongoing cpu hotplug operation.
> */
> int refcount;
> -} cpu_hotplug;
> -
> -void __init cpu_hotplug_init(void)
> -{
> - cpu_hotplug.active_writer = NULL;
> - mutex_init(&cpu_hotplug.lock);
> - cpu_hotplug.refcount = 0;
> -}
> +} cpu_hotplug = {
> + .active_writer = NULL,
> + .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
> + .refcount = 0,
> +};
>
> #ifdef CONFIG_HOTPLUG_CPU
>
On Mon, 2009-06-22 at 19:29 -0700, Linus Torvalds wrote:
> Static initializers are much nicer than having to worry about when
> something gets initialized, no?
Your patch works here and fixes the problem too.
Thanks !
Cheers,
Ben.
> Linus
>
> ---
> include/linux/cpu.h | 5 -----
> init/main.c | 1 -
> kernel/cpu.c | 13 +++++--------
> 3 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 2643d84..4d668e0 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -69,7 +69,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
>
> int cpu_up(unsigned int cpu);
> void notify_cpu_starting(unsigned int cpu);
> -extern void cpu_hotplug_init(void);
> extern void cpu_maps_update_begin(void);
> extern void cpu_maps_update_done(void);
>
> @@ -84,10 +83,6 @@ static inline void unregister_cpu_notifier(struct notifier_block *nb)
> {
> }
>
> -static inline void cpu_hotplug_init(void)
> -{
> -}
> -
> static inline void cpu_maps_update_begin(void)
> {
> }
> diff --git a/init/main.c b/init/main.c
> index 09131ec..4870dfe 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -678,7 +678,6 @@ asmlinkage void __init start_kernel(void)
> #endif
> page_cgroup_init();
> enable_debug_pagealloc();
> - cpu_hotplug_init();
> kmemtrace_init();
> kmemleak_init();
> debug_objects_mem_init();
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 395b697..8ce1004 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -34,14 +34,11 @@ static struct {
> * an ongoing cpu hotplug operation.
> */
> int refcount;
> -} cpu_hotplug;
> -
> -void __init cpu_hotplug_init(void)
> -{
> - cpu_hotplug.active_writer = NULL;
> - mutex_init(&cpu_hotplug.lock);
> - cpu_hotplug.refcount = 0;
> -}
> +} cpu_hotplug = {
> + .active_writer = NULL,
> + .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
> + .refcount = 0,
> +};
>
> #ifdef CONFIG_HOTPLUG_CPU
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Linus Torvalds wrote:
> On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
>
>
>> + /*
>> + * This initializes the mutex used by get/put_online_cpus()
>> + * which is used by SLAB
>> + */
>> + cpu_hotplug_init();
>>
>
> Oh fudge it.
>
> Why not get rid of that stupid thing, rather than have that subtle
> dependency and a comment for it.
>
> IOW, does this simpler alternative also work?
This fixes the problem for me.
Thanks
-Sachin
--
---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------
On Mon, 2009-06-22 at 19:29 -0700, Linus Torvalds wrote:
>
> On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
>
> > + /*
> > + * This initializes the mutex used by get/put_online_cpus()
> > + * which is used by SLAB
> > + */
> > + cpu_hotplug_init();
>
> Oh fudge it.
>
> Why not get rid of that stupid thing, rather than have that subtle
> dependency and a comment for it.
>
> IOW, does this simpler alternative also work?
Yes, works for me.
James
On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
> > + /*
> > + * This initializes the mutex used by get/put_online_cpus()
> > + * which is used by SLAB
> > + */
> > + cpu_hotplug_init();
On Mon, 2009-06-22 at 19:29 -0700, Linus Torvalds wrote:
> Oh fudge it.
>
> Why not get rid of that stupid thing, rather than have that subtle
> dependency and a comment for it.
>
> IOW, does this simpler alternative also work?
>
> When given the choice of simplifying and removing code, always do so.
>
> Static initializers are much nicer than having to worry about when
> something gets initialized, no?
Looks good to me. Thanks for fixing it up, Linus!
Pekka