Re: [PULL] x86 cpumask work

[Ingo Molnar <mingo@elte.hu>]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Saturday 14 March 2009 01:57:42 Ingo Molnar wrote:
> > Note, it might have crashed in a cpu hotplug test i'm conducting 
> > during bootup:
> > 
> > echo 0 > /sys/devices/system/cpu/cpu1/online
> 
> Indeed, thanks!
> 
> Subject: cpumask: fix crash when offlining cpus
> 
> 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/kernel/process.c b/arch/x86/kernel/process.c
> index cad5431..91a8c26 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,8 +557,11 @@ 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);
> +		/* c1e_mask can only be NULL during boot of first cpu. */
> +		if (c1e_mask == NULL) {
> +			alloc_cpumask_var(&c1e_mask, GFP_KERNEL);

Sigh, there are two bugs here:

   1) what if the GFP_KERNEL allocation fails?

   2) this code is called with interrupts disabled, so a 
      GFP_KERNEL allocation can be lethal.

c1e_mask should stay a static cpumask...

Why do we convert static, standalone masks to cpumask_var?

	Ingo