Re: [PATCH] x86: fix app crashes after SMP resume

["Glauber Costa" <glommer@gmail.com>]

On Tue, May 13, 2008 at 10:26 AM, Hugh Dickins <hugh@veritas.com> wrote:
> After resume on a 2cpu laptop, kernel builds collapse with a sed hang,
>  sh or make segfault (often on 20295564), real-time signal to cc1 etc.
>
>  Several hurdles to jump, but a manually-assisted bisect led to -rc1's
>  d2bcbad5f3ad38a1c09861bca7e252dde7bb8259 x86: do not zap_low_mappings
>  in __smp_prepare_cpus.  Though the low mappings were removed at bootup,
>  they were left behind (with Global flags helping to keep them in TLB)
>  after resume or cpu online, causing the crashes seen.
>
>  Reinstate zap_low_mappings (with local __flush_tlb_all) for each cpu_up
>  on x86_32.  This used to be serialized by smp_commenced_mask: that's now
>  gone, but a low_mappings flag will do.  No need for native_smp_cpus_done
>  to repeat the zap: let mem_init zap BSP's low mappings just like on UP.

Great!

Thanks for finding and fixing this, hugh.

>  --- 2.6.26-rc2/arch/x86/kernel/smpboot.c        2008-05-12 02:01:05.000000000 +0100
>  +++ linux/arch/x86/kernel/smpboot.c     2008-05-12 14:07:06.000000000 +0100
>  @@ -86,6 +86,7 @@ void *x86_bios_cpu_apicid_early_ptr;
>
>   #ifdef CONFIG_X86_32
>   u8 apicid_2_node[MAX_APICID];
>  +static int low_mappings;
>   #endif
>
>   /* State of each CPU */
>  @@ -326,6 +327,12 @@ static void __cpuinit start_secondary(vo
>                 enable_8259A_irq(0);
>         }
>
>  +#ifdef CONFIG_X86_32
>  +       while (low_mappings)
>  +               cpu_relax();
>  +       __flush_tlb_all();
>  +#endif
>  +

If possible, we don't want to introduce any more ifdefs. Should be
better to do it openly, define low_mappings as always 0 for x86_64. We
can use an unlikely() test to make it faster too.

>         /* This must be done before setting cpu_online_map */
>         set_cpu_sibling_map(raw_smp_processor_id());
>         wmb();
>  @@ -1040,14 +1047,20 @@ int __cpuinit native_cpu_up(unsigned int
>   #ifdef CONFIG_X86_32
>         /* init low mem mapping */
>         clone_pgd_range(swapper_pg_dir, swapper_pg_dir + KERNEL_PGD_BOUNDARY,
>  -                       min_t(unsigned long, KERNEL_PGD_PTRS, KERNEL_PGD_BOUNDARY));
>  +               min_t(unsigned long, KERNEL_PGD_PTRS, KERNEL_PGD_BOUNDARY));
>         flush_tlb_all();
>  -#endif
>  +       low_mappings = 1;
>
>         err = do_boot_cpu(apicid, cpu);
>  -       if (err < 0) {
>  +
>  +       zap_low_mappings();
>  +       low_mappings = 0;
>  +#else
>  +       err = do_boot_cpu(apicid, cpu);
>  +#endif
>  +       if (err) {
>                 Dprintk("do_boot_cpu failed %d\n", err);
>  -               return err;
>  +               return -EIO;
>         }
>
>         /*
>  @@ -1259,9 +1272,6 @@ void __init native_smp_cpus_done(unsigne
>         setup_ioapic_dest();
>   #endif
>         check_nmi_watchdog();
>  -#ifdef CONFIG_X86_32
>  -       zap_low_mappings();
>  -#endif
>   }
>
>   #ifdef CONFIG_HOTPLUG_CPU
>  --- 2.6.26-rc2/arch/x86/mm/init_32.c    2008-05-03 21:54:41.000000000 +0100
>  +++ linux/arch/x86/mm/init_32.c 2008-05-12 14:07:06.000000000 +0100
>  @@ -438,8 +438,6 @@ void zap_low_mappings(void)
>   {
>         int i;
>
>  -       save_pg_dir();
>  -
>         /*
>          * Zap initial low-memory mappings.
>          *
>  @@ -663,16 +661,8 @@ void __init mem_init(void)
>                 test_wp_bit();
>
>         cpa_init();
>  -
>  -       /*
>  -        * Subtle. SMP is doing it's boot stuff late (because it has to
>  -        * fork idle threads) - but it also needs low mappings for the
>  -        * protected-mode entry to work. We zap these entries only after
>  -        * the WP-bit has been tested.
>  -        */
>  -#ifndef CONFIG_SMP
>  +       save_pg_dir();
>         zap_low_mappings();
>  -#endif
>   }
>
>   #ifdef CONFIG_MEMORY_HOTPLUG
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."