Re: [PATCH] Fix cpu online/offline bug: mce memory leak.

[Keir Fraser <keir.xen@gmail.com>]

Some comments inline below. Overall a good patch to have, but needs some
minor adjustments!

 -- Keir

On 01/03/2011 09:09, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

> Fix cpu online/offline bug: mce memory leak.
> 
> Current Xen mce logic didn't free mcabanks. This would be a memory leak when
> cpu offline.
> When repeatly do cpu online/offline, this memory leak would make xenpool
> shrink, and at a time point,
> will call alloc_heap_pages --> flush_area_mask, which
> ASSERT(local_irq_is_enabled()).
> However, cpu online is irq disable, so it finally result in Xen crash.
> 
> This patch fix the memory leak bug, and tested OK over 110,000 round cpu
> online/offline.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> diff -r 1a364b17d66a xen/arch/x86/cpu/mcheck/mce_intel.c
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c Fri Feb 25 01:26:01 2011 +0800
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Mon Feb 28 19:19:20 2011 +0800
> @@ -1227,9 +1227,24 @@ static void intel_init_mce(void)
>      mce_uhandler_num = sizeof(intel_mce_uhandlers)/sizeof(struct
> mca_error_handler);
>  }
>  
> -static int intel_init_mca_banks(void)
> +static void cpu_mcabank_free(unsigned int cpu)
>  {
> -    struct mca_banks *mb1, *mb2, * mb3;
> +    struct mca_banks *mb1, *mb2, *mb3, *mb4;
> +
> +    mb1 = per_cpu(mce_clear_banks, cpu);
> +    mb2 = per_cpu(no_cmci_banks, cpu);
> +    mb3 = per_cpu(mce_banks_owned, cpu);
> +    mb4 = per_cpu(poll_bankmask, cpu);
> +
> +    mcabanks_free(mb1);
> +    mcabanks_free(mb2);
> +    mcabanks_free(mb3);
> +    mcabanks_free(mb4);
> +}
> +
> +static void cpu_mcabank_alloc(unsigned int cpu)
> +{
> +    struct mca_banks *mb1, *mb2, *mb3;
>  
>      mb1 = mcabanks_alloc();
>      mb2 = mcabanks_alloc();
> @@ -1237,22 +1252,23 @@ static int intel_init_mca_banks(void)
>      if (!mb1 || !mb2 || !mb3)
>          goto out;
>  
> -    __get_cpu_var(mce_clear_banks) = mb1;
> -    __get_cpu_var(no_cmci_banks) = mb2;
> -    __get_cpu_var(mce_banks_owned) = mb3;
> +    per_cpu(mce_clear_banks, cpu) = mb1;
> +    per_cpu(no_cmci_banks, cpu) = mb2;
> +    per_cpu(mce_banks_owned, cpu) = mb3;
> +    return;
>  
> -    return 0;
>  out:
>      mcabanks_free(mb1);
>      mcabanks_free(mb2);
>      mcabanks_free(mb3);
> -    return -ENOMEM;
>  }
>  
>  /* p4/p6 family have similar MCA initialization process */
>  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c)
>  {
> -    if (intel_init_mca_banks())
> +    if ( !this_cpu(mce_clear_banks) ||
> +         !this_cpu(no_cmci_banks)   ||
> +         !this_cpu(mce_banks_owned) )
>           return mcheck_none;
>  
>      intel_init_mca(c);
> @@ -1301,13 +1317,19 @@ static int cpu_callback(
>  static int cpu_callback(
>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>  {
> +    unsigned int cpu = (unsigned long)hcpu;
> +
>      switch ( action )
>      {
> +    case CPU_UP_PREPARE:
> +        cpu_mcabank_alloc(cpu);
> +        break;
>      case CPU_DYING:
>          cpu_mcheck_disable();
>          break;
>      case CPU_DEAD:
>          cpu_mcheck_distribute_cmci();
> +        cpu_mcabank_free(cpu);
>          break;

You also need to cpu_mcabank_free(cpu) on CPU_UP_CANCELED. Else there's a
memory leak on failed CPU bringup.

>      default:
>          break;
> @@ -1322,6 +1344,8 @@ static struct notifier_block cpu_nfb = {
>  
>  static int __init intel_mce_initcall(void)
>  {
> +    void *hcpu = (void *)(long)smp_processor_id();
> +    cpu_callback(&cpu_nfb, CPU_UP_PREPARE, hcpu);
>      register_cpu_notifier(&cpu_nfb);
>      return 0;
>  }
> diff -r 1a364b17d66a xen/arch/x86/setup.c
> --- a/xen/arch/x86/setup.c Fri Feb 25 01:26:01 2011 +0800
> +++ b/xen/arch/x86/setup.c Mon Feb 28 19:19:20 2011 +0800
> @@ -1203,6 +1203,8 @@ void __init __start_xen(unsigned long mb
>  
>      arch_init_memory();
>  
> +    do_presmp_initcalls();
> +
>      identify_cpu(&boot_cpu_data);
>      if ( cpu_has_fxsr )
>          set_in_cr4(X86_CR4_OSFXSR);
> @@ -1235,8 +1237,6 @@ void __init __start_xen(unsigned long mb
>      initialize_keytable();
>  
>      console_init_postirq();
> -
> -    do_presmp_initcalls();

This looks risky, especially so close to 4.1 release. Instead of moving
do_presmp_initcalls(), please special-case cpu_mcabank_alloc() for CPU0 in
intel_mcheck_init(), and remove your direct call to
cpu_callback(...CPU_UP_PREPARE...) in intel_mce_initcall().

>      for_each_present_cpu ( i )
>      {