Messages in this thread Patch in this message | | | From | Rusty Russell <> | Subject | Re: [PULL] x86 cpumask work | Date | Tue, 17 Mar 2009 14:50:34 +1030 |
| |
On Monday 16 March 2009 19:18:02 Ingo Molnar wrote: > > * Rusty Russell <rusty@rustcorp.com.au> wrote: > > As the comments says, it can only be NULL during boot of the first CPU. > > > > start_kernel -> check_bugs -> identify_boot_cpu -> identify_cpu > > -> select_idle_routine. > > > > Did you want me to panic if it fails? > > Ah, ok. But i think it's very unobvious to embedd a "once per > bootup" dynamic allocation like that. Please put it into a > separate init routine instead. That way select_idle() [which > runs during every CPU hotplug event] wont have this allocation.
There wasn't an obvious place to put the init; I've created one.
How's this? ===
Subject: cpumask: fix CONFIG_CPUMASK_OFFSTACK=y cpu hotunplug crash
Impact: Fix cpu offline when CONFIG_MAXSMP=y
Changeset bc9b83dd1f66402b870301c3c7117b9c1484abb4 "cpumask: convert c1e_mask in arch/x86/kernel/process.c to cpumask_var_t" contained a bug: c1e_mask is manipulated even if C1E isn't detected (and hence not allocated). This is simply fixed by checking for NULL (which gcc optimizes out anyway of CONFIG_CPUMASK_OFFSTACK=n, since it knows ce1_mask can never be NULL).
In addition, fix a leak where select_idle_routine re-allocates (and re-clears) c1e_mask on every cpu init.
Reported-by: Ingo Molnar <mingo@elte.hu> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index d794d94..9874dd9 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -733,6 +733,7 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx) extern void mwait_idle_with_hints(unsigned long eax, unsigned long ecx); extern void select_idle_routine(const struct cpuinfo_x86 *c); +extern void init_c1e_mask(void); extern unsigned long boot_option_idle_override; extern unsigned long idle_halt; diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 82f6cc0..d7dd3c2 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -812,6 +812,7 @@ static void vgetcpu_set_mode(void) void __init identify_boot_cpu(void) { identify_cpu(&boot_cpu_data); + init_c1e_mask(); #ifdef CONFIG_X86_32 sysenter_setup(); enable_sep_cpu(); diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index 6638294..78533a5 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -479,7 +479,8 @@ static int c1e_detected; void c1e_remove_cpu(int cpu) { - cpumask_clear_cpu(cpu, c1e_mask); + if (c1e_mask != NULL) + cpumask_clear_cpu(cpu, c1e_mask); } /* @@ -556,13 +557,20 @@ void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c) pm_idle = mwait_idle; } else if (check_c1e_idle(c)) { printk(KERN_INFO "using C1E aware idle routine\n"); - alloc_cpumask_var(&c1e_mask, GFP_KERNEL); - cpumask_clear(c1e_mask); pm_idle = c1e_idle; } else pm_idle = default_idle; } +void __init init_c1e_mask(void) +{ + /* If we're using c1e_idle, we need to allocate c1e_mask. */ + if (pm_idle == c1e_idle) { + alloc_cpumask_var(&c1e_mask, GFP_KERNEL); + cpumask_clear(c1e_mask); + } +} + static int __init idle_setup(char *str) { if (!str)
| |