Mailing List Archive

* 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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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!!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/