[PATCH] x86: fix app crashes after SMP resume

[PATCH] x86: fix app crashes after SMP resume

[Hugh Dickins]

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.

(In passing, fix error code from native_cpu_up: do_boot_cpu returns a
variety of diagnostic values, Dprintk what it says but convert to -EIO.
And save_pg_dir separately before zap_low_mappings: doesn't matter now,
but zapping twice in succession wiped out resume's swsusp_pg_dir.)

That worked well on the duo and one quad, but wouldn't boot 3rd or 4th
cpu on P4 Xeon, oopsing just after unlock_ipi_call_lock.  The TLB flush
IPI now being sent reveals a long-standing bug: the booting cpu has its
APIC readied in smp_callin at the top of start_secondary, but isn't put
into the cpu_online_map until just before that unlock_ipi_call_lock.

So native_smp_call_function_mask to online cpus would send_IPI_allbutself,
including the cpu just coming up, though it has been excluded from the
count to wait for: by the time it handles the IPI, the call data on
native_smp_call_function_mask's stack may well have been overwritten.

So fall back to send_IPI_mask while cpu_online_map does not match
cpu_callout_map: perhaps there's a better APICological fix to be
made at the start_secondary end, but I wouldn't know that.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
I've often wondered: should git reject any commit with "bad" in the id?

 arch/x86/kernel/smp.c     |    3 ++-
 arch/x86/kernel/smpboot.c |   24 +++++++++++++++++-------
 arch/x86/mm/init_32.c     |   12 +-----------
 3 files changed, 20 insertions(+), 19 deletions(-)

--- 2.6.26-rc2/arch/x86/kernel/smp.c	2008-05-03 21:54:40.000000000 +0100
+++ linux/arch/x86/kernel/smp.c	2008-05-13 11:31:26.000000000 +0100
@@ -231,7 +231,8 @@ native_smp_call_function_mask(cpumask_t 
 	wmb();
 
 	/* Send a message to other CPUs */
-	if (cpus_equal(mask, allbutself))
+	if (cpus_equal(mask, allbutself) &&
+	    cpus_equal(cpu_online_map, cpu_callout_map))
 		send_IPI_allbutself(CALL_FUNCTION_VECTOR);
 	else
 		send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
--- 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
+
 	/* 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

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

[Ingo Molnar]

* 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.
> 
> (In passing, fix error code from native_cpu_up: do_boot_cpu returns a 
> variety of diagnostic values, Dprintk what it says but convert to 
> -EIO. And save_pg_dir separately before zap_low_mappings: doesn't 
> matter now, but zapping twice in succession wiped out resume's 
> swsusp_pg_dir.)
> 
> That worked well on the duo and one quad, but wouldn't boot 3rd or 4th 
> cpu on P4 Xeon, oopsing just after unlock_ipi_call_lock.  The TLB 
> flush IPI now being sent reveals a long-standing bug: the booting cpu 
> has its APIC readied in smp_callin at the top of start_secondary, but 
> isn't put into the cpu_online_map until just before that 
> unlock_ipi_call_lock.
> 
> So native_smp_call_function_mask to online cpus would 
> send_IPI_allbutself, including the cpu just coming up, though it has 
> been excluded from the count to wait for: by the time it handles the 
> IPI, the call data on native_smp_call_function_mask's stack may well 
> have been overwritten.
> 
> So fall back to send_IPI_mask while cpu_online_map does not match 
> cpu_callout_map: perhaps there's a better APICological fix to be made 
> at the start_secondary end, but I wouldn't know that.

applied, thanks Hugh! You've once again proven that you are worth your 
weight in gold and more. Patch looks very clean and simplifies the code.

> I've often wondered: should git reject any commit with "bad" in the 
> id?

hehe :)

ob'note: would be nice to have the suspend+resume self-test/debug patch 
below upstream. It programs the RTC to a 5 second sleep and resumes the 
computer, which then self-wakeups afterwards.

	Ingo

-------------------------------->
Subject: suspend+resume self-test
From: David Brownell <david-b@pacbell.net>

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/power/Kconfig |   10 +++
 kernel/power/main.c  |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+)

Index: linux/kernel/power/Kconfig
===================================================================
--- linux.orig/kernel/power/Kconfig
+++ linux/kernel/power/Kconfig
@@ -94,6 +94,16 @@ config SUSPEND
 	  powered and thus its contents are preserved, such as the
 	  suspend-to-RAM state (e.g. the ACPI S3 state).
 
+config PM_TEST_SUSPEND
+	bool "Test suspend/resume and wakealarm during bootup"
+	depends on SUSPEND && PM_DEBUG && RTC_LIB=y
+	---help---
+	This option will suspend your machine during bootup, and make
+	it wake up a few seconds later using the RTC's wakeup alarm.
+
+	You probably want to have your system's RTC driver statically
+	linked, ensuring that it's available when this test runs.
+
 config SUSPEND_FREEZER
 	bool "Enable freezer for suspend to RAM/standby" \
 		if ARCH_WANTS_FREEZER_CONTROL || BROKEN
Index: linux/kernel/power/main.c
===================================================================
--- linux.orig/kernel/power/main.c
+++ linux/kernel/power/main.c
@@ -132,6 +132,52 @@ static inline int suspend_test(int level
 
 #ifdef CONFIG_SUSPEND
 
+#ifdef CONFIG_PM_TEST_SUSPEND
+
+/*
+ * We test the system suspend code by setting an RTC wakealarm a short
+ * time in the future, then suspending.  Suspending the devices won't
+ * normally take long ... some systems only need a few milliseconds.
+ *
+ * The time it takes is system-specific though, so when we test this
+ * during system bootup we allow a LOT of time.
+ */
+#define TEST_SUSPEND_SECONDS	5
+
+static unsigned long suspend_test_start_time;
+
+static void suspend_test_start(void)
+{
+	/* FIXME Use better timebase than "jiffies", ideally a clocksource.
+	 * What we want is a hardware counter that will work correctly even
+	 * during the irqs-are-off stages of the suspend/resume cycle...
+	 */
+	suspend_test_start_time = jiffies;
+}
+
+static void suspend_test_finish(const char *label)
+{
+	long nj = jiffies - suspend_test_start_time;
+	unsigned msec;
+
+	msec = jiffies_to_msecs((nj >= 0) ? nj : -nj);
+	pr_info("PM: %s took %d.%03d seconds\n", label,
+			msec / 1000, msec % 1000);
+	WARN_ON_ONCE(msec > ((TEST_SUSPEND_SECONDS+5) * 1000));
+}
+
+#else
+
+static void suspend_test_start(void)
+{
+}
+
+static void suspend_test_finish(const char *label)
+{
+}
+
+#endif
+
 /* This is just an arbitrary number */
 #define FREE_PAGE_NUMBER (100)
 
@@ -264,11 +310,14 @@ int suspend_devices_and_enter(suspend_st
 			goto Close;
 	}
 	suspend_console();
+
+	suspend_test_start();
 	error = device_suspend(PMSG_SUSPEND);
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to suspend\n");
 		goto Resume_console;
 	}
+	suspend_test_finish("suspend devices");
 
 	if (suspend_test(TEST_DEVICES))
 		goto Resume_devices;
@@ -291,7 +340,9 @@ int suspend_devices_and_enter(suspend_st
 	if (suspend_ops->finish)
 		suspend_ops->finish();
  Resume_devices:
+	suspend_test_start();
 	device_resume();
+	suspend_test_finish("resume devices");
  Resume_console:
 	resume_console();
  Close:
@@ -515,3 +566,115 @@ static int __init pm_init(void)
 }
 
 core_initcall(pm_init);
+
+
+#ifdef CONFIG_PM_TEST_SUSPEND
+
+#include <linux/rtc.h>
+
+/*
+ * To test system suspend, we need a hands-off mechanism to resume the
+ * system.  RTCs with wakeup alarms are the the most common mechanism
+ * that's self-contained.
+ */
+
+static void __init test_wakealarm(struct rtc_device *rtc, suspend_state_t state)
+{
+	static char	err_readtime [] __initdata =
+		KERN_ERR "PM: can't read %s time, err %d\n";
+	static char	err_wakealarm [] __initdata =
+		KERN_ERR "PM: can't set %s wakealarm, err %d\n";
+	static char	err_suspend [] __initdata =
+		KERN_ERR "PM: suspend test failed, error %d\n";
+	static char	info_test [] __initdata =
+		KERN_INFO "PM: test RTC wakeup from '%s' suspend\n";
+
+	unsigned long		now;
+	struct rtc_wkalrm	alm;
+	int			status;
+
+	/* this may fail if the RTC hasn't been initialized */
+	status = rtc_read_time(rtc, &alm.time);
+	if (status < 0) {
+		printk(err_readtime, rtc->dev.bus_id, status);
+		return;
+	}
+	rtc_tm_to_time(&alm.time, &now);
+
+	memset(&alm, 0, sizeof alm);
+	rtc_time_to_tm(now + TEST_SUSPEND_SECONDS, &alm.time);
+	alm.enabled = true;
+
+	status = rtc_set_alarm(rtc, &alm);
+	if (status < 0) {
+		printk(err_wakealarm, rtc->dev.bus_id, status);
+		return;
+	}
+
+	if (state == PM_SUSPEND_MEM) {
+		printk(info_test, pm_states[state]);
+		status = pm_suspend(state);
+		if (status == -ENODEV)
+			state = PM_SUSPEND_STANDBY;
+	}
+	if (state == PM_SUSPEND_STANDBY) {
+		printk(info_test, pm_states[state]);
+		status = pm_suspend(state);
+	}
+	if (status < 0)
+		printk(err_suspend, status);
+}
+
+static int __init has_wakealarm(struct device *dev, void *name_ptr)
+{
+	struct rtc_device *candidate = to_rtc_device(dev);
+
+	if (!candidate->ops->set_alarm)
+		return 0;
+	if (!device_may_wakeup(candidate->dev.parent))
+		return 0;
+
+	*(char **)name_ptr = dev->bus_id;
+	return 1;
+}
+
+/*
+ * We normally test Suspend-to-RAM, with standby as a backup when
+ * the system doesn't support that state.  But we also need to be
+ * able to disable the powerup test, and tell it to ignore STR since
+ * the RTC may not work then.
+ */
+static suspend_state_t test_state __initdata = PM_SUSPEND_MEM;
+
+static int __init setup_test_suspend(char *value)
+{
+	/* FIXME accept "standby", etc */
+	test_state = PM_SUSPEND_ON;
+	return 0;
+}
+__setup("test_suspend", setup_test_suspend);
+
+static int __init test_suspend(void)
+{
+	static char	warn_no_rtc[] __initdata =
+		KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";
+
+	char			*pony = NULL;
+	struct rtc_device	*rtc = NULL;
+
+	class_find_device(rtc_class, &pony, has_wakealarm);
+	if (pony)
+		rtc = rtc_class_open(pony);
+
+	if (rtc) {
+		if (test_state != PM_SUSPEND_ON)
+			test_wakealarm(rtc, test_state);
+		rtc_class_close(rtc);
+	} else
+		printk(warn_no_rtc);
+
+	return 0;
+}
+late_initcall(test_suspend);
+
+#endif /* CONFIG_PM_TEST_SUSPEND */

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

[Pavel Machek]

Hi!

> hehe :)
> 
> ob'note: would be nice to have the suspend+resume self-test/debug patch 
> below upstream. It programs the RTC to a 5 second sleep and resumes the 
> computer, which then self-wakeups afterwards.

The version with pony, yes! :-)

> Subject: suspend+resume self-test
> From: David Brownell <david-b@pacbell.net>
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

ACK. Perhaps you should send it to Andrew for a merge? Or maybe Rafael
wants to add it to his patch queue?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Glauber Costa]

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."

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

[Hugh Dickins]

On Tue, 13 May 2008, Glauber Costa wrote:
> On Tue, May 13, 2008 at 10:26 AM, Hugh Dickins <hugh@veritas.com> wrote:
> >
> >  +#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.

You're right, something like that (but avoiding the __flush_tlb_all
on x86_64) would have been nicer; never mind, now it's going forward,
I'll leave it as is.

A bigger improvement would be to cut out all that swapper_pg_dir
to and fro, needing a global TLB flush on all cpus for each cpu.
Just have an alternate pg_dir (often already there as swsusp_pg_dir)
to point cr3 at for the bootup (or maybe it needs to be vice versa).

But that's harder to get right, and involves wider changes and much
more testing than I could afford for the bugfix.  Plus I expect it's
on your radar if not already done.

Hugh

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

[Carlos R. Mafra]

On Tue 13.May'08 at 14:26:57 +0100, Hugh Dickins 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.
> [...] 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---

I've tested it twice already in my P4 desktop (where the problem was
very frequent after "echo standby > /sys/power/state") and the
patch really fixed it!!

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

[Hugh Dickins]

On Tue, 13 May 2008, Carlos R. Mafra wrote:
> On Tue 13.May'08 at 14:26:57 +0100, Hugh Dickins 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.
> > [...] 
> > Signed-off-by: Hugh Dickins <hugh@veritas.com>
> > ---
> 
> I've tested it twice already in my P4 desktop (where the problem was
> very frequent after "echo standby > /sys/power/state") and the
> patch really fixed it!!

Great, thanks a lot for sharing the problem and reporting back, Carlos.

Hugh

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

[Theodore Tso]

Hugh,

Many, many thanks!!  This fixed the my suspend/resume problem on my
X61s as well (where the X server would stop accepting keyboard and
mouse input, and subsequent to restart the X server after remote login
would fail).

      	   	     	  	       		      - Ted