crash when doing second suspend: BUG in arch/i386/kernel/nmi.c:174

Quoted 11 lines · Jeremy Fitzhardinge

I'm trying to get suspend/resume working properly on my Thinkpad X60. This is a dual-core machine, so its running in SMP mode.
Now that I have a set of patches to make AHCI resume properly, I'm getting a crash on the second suspend. I can't get an actual listing of the oops, but I have a set of screenshots if anyone needs more details.
The gist is that there's a BUG_ON failing at arch/i386/kernel/nmi.c:174 (BUG_ON(counter > NMI_MAX_COUNTER_BITS)), in release_evntsel_nmi. The backtrace is:

Quoted 15 lines

release_evntsel_nmi
stop_apci_nmi_watchdog
on_each_cpu
disable_lapic_nmi_watchdog
lapic_nmi_suspend
sysdev_suspend
device_power_down
suspend_enter
enter_state
state_store
subsys_attr_store
sysfs_write_file
vfs_write
sys_write
sysenter_past_esp

This BUG_ON was introduced by the patch x86_64-mm-add-performance-counter-reservation-framework-for-up-kernels.patch.

Andrew Morton

2006-06-05 07:48 UTC #4 lore raw

Quoted 28 lines · On Mon, 05 Jun 2006 00:37:22 -0700 Jeremy Fitzhardinge <[email protected]> wrote:

Quoted 11 lines · Jeremy Fitzhardinge
I'm trying to get suspend/resume working properly on my Thinkpad X60. This is a dual-core machine, so its running in SMP mode.
Now that I have a set of patches to make AHCI resume properly, I'm getting a crash on the second suspend. I can't get an actual listing of the oops, but I have a set of screenshots if anyone needs more details.
The gist is that there's a BUG_ON failing at arch/i386/kernel/nmi.c:174 (BUG_ON(counter > NMI_MAX_COUNTER_BITS)), in release_evntsel_nmi. The backtrace is:
Quoted 15 lines
release_evntsel_nmi
stop_apci_nmi_watchdog
on_each_cpu
disable_lapic_nmi_watchdog
lapic_nmi_suspend
sysdev_suspend
device_power_down
suspend_enter
enter_state
state_store
subsys_attr_store
sysfs_write_file
vfs_write
sys_write
sysenter_past_esp

Quoted 3 lines

This BUG_ON was introduced by the patch x86_64-mm-add-performance-counter-reservation-framework-for-up-kernels.patch.

http://bugzilla.kernel.org/show_bug.cgi?id=6647 has details.

Do you think the suspend breakage is related to that patch?

Miles also reports that every second suspend fails for him. Miles, does 'nmi_watchdog=0' make it better?

Jeremy Fitzhardinge

author

2006-06-05 07:59 UTC #5 lore raw

Quoted 4 lines · Andrew Morton

http://bugzilla.kernel.org/show_bug.cgi?id=6647 has details.
Do you think the suspend breakage is related to that patch?

Yes. I haven't really worked out what's going on in there, but it looks like it's losing track of what it has allocated and running out of timer MSRs. Possibly because the CPUs are reinitialized on resume: it "thaws the CPUs", which prints the same CPU information as at boot time - caps, bogomips, etc - so I presume it actually redoes those things. I wonder if this makes the performance counter reservation loose track of things?

Miles Lane

2006-06-05 08:35 UTC #6 lore raw

Quoted 4 lines · Andrew Morton

Do you think the suspend breakage is related to that patch?
Miles also reports that every second suspend fails for him. Miles, does 'nmi_watchdog=0' make it better?

I tried using that as an appended boot option, but it didn't change the behavior.

Miles

Shaohua Li

2006-06-06 07:05 UTC #7 lore raw

Quoted 11 lines · Miles Lane

Quoted 3 lines · Andrew Morton
Do you think the suspend breakage is related to that patch?
Miles also reports that every second suspend fails for him. Miles,
does
Quoted 1 line
'nmi_watchdog=0' make it better?
I tried using that as an appended boot option, but it didn't change the
behavior.

Does below patch help? The nmi suspend/resume doesn't look good to me. Only CPU0 uses the suspend/resume code path. Other CPUs run the CPU hotplug code path.

Signed-off-by Shaohua Li <[email protected]>

---

 linux-2.6.17-rc5-mm3-root/arch/i386/kernel/nmi.c       |   14 +++++++++-----
 linux-2.6.17-rc5-mm3-root/arch/i386/kernel/smpboot.c   |    3 ++-
 linux-2.6.17-rc5-mm3-root/arch/x86_64/kernel/nmi.c     |   14 +++++++++-----
 linux-2.6.17-rc5-mm3-root/arch/x86_64/kernel/smpboot.c |    2 ++
 linux-2.6.17-rc5-mm3-root/include/asm-i386/nmi.h       |    1 +
 linux-2.6.17-rc5-mm3-root/include/asm-x86_64/nmi.h     |    1 +
 6 files changed, 24 insertions(+), 11 deletions(-)

arch/i386/kernel/nmi.c renamed from arch/i386/kernel/nmi.c~nmi +9 -5

diff -puN arch/i386/kernel/nmi.c~nmi arch/i386/kernel/nmi.c
@@ -65,7 +65,6 @@ struct nmi_watchdog_ctlblk {
 static DEFINE_PER_CPU(struct nmi_watchdog_ctlblk, nmi_watchdog_ctlblk);
 
 /* local prototypes */
-static void stop_apic_nmi_watchdog(void *unused);
 static int unknown_nmi_panic_callback(struct pt_regs *regs, int cpu);
 
 extern void show_registers(struct pt_regs *regs);

@@ -377,15 +376,20 @@ static int nmi_pm_active; /* nmi_active 
 
 static int lapic_nmi_suspend(struct sys_device *dev, pm_message_t state)
 {
+	/* only CPU0 goes here, other CPUs should be offline */
 	nmi_pm_active = atomic_read(&nmi_active);
-	disable_lapic_nmi_watchdog();
+	stop_apic_nmi_watchdog(NULL);
+	BUG_ON(atomic_read(&nmi_active) != 0);
 	return 0;
 }
 
 static int lapic_nmi_resume(struct sys_device *dev)
 {
-	if (nmi_pm_active > 0)
-		enable_lapic_nmi_watchdog();
+	/* only CPU0 goes here, other CPUs should be offline */
+	if (nmi_pm_active > 0) {
+		setup_apic_nmi_watchdog(NULL);
+		touch_nmi_watchdog();
+	}
 	return 0;
 }

@@ -798,7 +802,7 @@ void setup_apic_nmi_watchdog (void *unus
 	atomic_inc(&nmi_active);
 }
 
-static void stop_apic_nmi_watchdog(void *unused)
+void stop_apic_nmi_watchdog(void *unused)
 {
 	/* only support LOCAL and IO APICs for now */
 	if ((nmi_watchdog != NMI_LOCAL_APIC) &&

arch/i386/kernel/smpboot.c renamed from arch/i386/kernel/smpboot.c~nmi +2 -1

diff -puN arch/i386/kernel/smpboot.c~nmi arch/i386/kernel/smpboot.c
@@ -1368,7 +1368,8 @@ int __cpu_disable(void)
 	 */
 	if (cpu == 0)
 		return -EBUSY;
-
+	if (nmi_watchdog == NMI_LOCAL_APIC)
+		stop_apic_nmi_watchdog(NULL);
 	clear_local_APIC();
 	/* Allow any queued timer interrupts to get serviced */
 	local_irq_enable();

include/asm-i386/nmi.h renamed from include/asm-i386/nmi.h~nmi +1 -0

diff -puN include/asm-i386/nmi.h~nmi include/asm-i386/nmi.h
@@ -23,6 +23,7 @@ extern int reserve_evntsel_nmi(unsigned 
 extern void release_evntsel_nmi(unsigned int);
 
 extern void setup_apic_nmi_watchdog (void *);
+extern void stop_apic_nmi_watchdog (void *);
 extern void disable_timer_nmi_watchdog(void);
 extern void enable_timer_nmi_watchdog(void);
 extern int nmi_watchdog_tick (struct pt_regs * regs, unsigned reason);

arch/x86_64/kernel/nmi.c renamed from arch/x86_64/kernel/nmi.c~nmi +9 -5

diff -puN arch/x86_64/kernel/nmi.c~nmi arch/x86_64/kernel/nmi.c
@@ -65,7 +65,6 @@ struct nmi_watchdog_ctlblk {
 static DEFINE_PER_CPU(struct nmi_watchdog_ctlblk, nmi_watchdog_ctlblk);
 
 /* local prototypes */
-static void stop_apic_nmi_watchdog(void *unused);
 static int unknown_nmi_panic_callback(struct pt_regs *regs, int cpu);
 
 /* converts an msr to an appropriate reservation bit */

@@ -363,15 +362,20 @@ static int nmi_pm_active; /* nmi_active 
 
 static int lapic_nmi_suspend(struct sys_device *dev, pm_message_t state)
 {
+	/* only CPU0 goes here, other CPUs should be offline */
 	nmi_pm_active = atomic_read(&nmi_active);
-	disable_lapic_nmi_watchdog();
+	stop_apic_nmi_watchdog(NULL);
+	BUG_ON(atomic_read(&nmi_active) != 0);
 	return 0;
 }
 
 static int lapic_nmi_resume(struct sys_device *dev)
 {
-	if (nmi_pm_active > 0)
-		enable_lapic_nmi_watchdog();
+	/* only CPU0 goes here, other CPUs should be offline */
+	if (nmi_pm_active > 0) {
+		setup_apic_nmi_watchdog(NULL);
+		touch_nmi_watchdog();
+	}
 	return 0;
 }

@@ -709,7 +713,7 @@ void setup_apic_nmi_watchdog(void *unuse
 	atomic_inc(&nmi_active);
 }
 
-static void stop_apic_nmi_watchdog(void *unused)
+void stop_apic_nmi_watchdog(void *unused)
 {
 	/* only support LOCAL and IO APICs for now */
 	if ((nmi_watchdog != NMI_LOCAL_APIC) &&

include/asm-x86_64/nmi.h renamed from include/asm-x86_64/nmi.h~nmi +1 -0

diff -puN include/asm-x86_64/nmi.h~nmi include/asm-x86_64/nmi.h
@@ -54,6 +54,7 @@ extern int reserve_evntsel_nmi(unsigned 
 extern void release_evntsel_nmi(unsigned int);
 
 extern void setup_apic_nmi_watchdog (void *);
+extern void stop_apic_nmi_watchdog (void *);
 extern void disable_timer_nmi_watchdog(void);
 extern void enable_timer_nmi_watchdog(void);
 extern int nmi_watchdog_tick (struct pt_regs * regs, unsigned reason);

arch/x86_64/kernel/smpboot.c renamed from arch/x86_64/kernel/smpboot.c~nmi +2 -0

diff -puN arch/x86_64/kernel/smpboot.c~nmi arch/x86_64/kernel/smpboot.c
@@ -1232,6 +1232,8 @@ int __cpu_disable(void)
 	if (cpu == 0)
 		return -EBUSY;
 
+	if (nmi_watchdog == NMI_LOCAL_APIC)
+		stop_apic_nmi_watchdog(NULL);
 	clear_local_APIC();
 
 	/*

Don Zickus

2006-06-06 14:13 UTC #8 lore raw

Quoted 7 lines · Shaohua Li

Quoted 6 lines · Miles Lane
Quoted 3 lines · Andrew Morton
Do you think the suspend breakage is related to that patch?
Miles also reports that every second suspend fails for him. Miles,
does

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
'nmi_watchdog=0' make it better?
I tried using that as an appended boot option, but it didn't change the
behavior.
Does below patch help? The nmi suspend/resume doesn't look good to me. Only CPU0 uses the suspend/resume code path. Other CPUs run the CPU hotplug code path.

This patch makes sense. I was unaware that the suspend/resume case was only for CPU0.

However, I am still concerned about one thing though. After looking through the bugzilla attachments, the reason why Jeremy's machine is crashing on the second suspend is because one of the watchdog timers is turning on after the resume.

Because he is using a i386 machine, the nmi watchdog is disabled by default. This is evident by his attachment ('cat /proc/interrupts | grep NMI was zero). The machine suspends then resumes. Running the same 'cat' command again, we see NMI output. My guess is during the second suspend, the NMI watchdog code sees one watchdog is enabled but tries to disable all of them (this is a bug and I will provide that patch). Upon disabling the 'disabled' watchdog, the BUG_ON is hit and the machine goes down.

My concern is _why_ one of the watchdog timers was re-enable during resume. It makes sense that CPU0 was still disabled, but apparently the hotplug case for CPU1 didn't realize that CPU1 was previously disabled or that the default is to remain off. I need to look at the code for this. Any inputs from those that know the hotplug code better than I?

Cheers,
Don

Andi Kleen

2006-06-06 14:18 UTC #9 lore raw

Quoted 3 lines

Because he is using a i386 machine, the nmi watchdog is disabled by default.

I changed that - it's now on by default on i386 too.

-Andi

Jeremy Fitzhardinge

author

2006-06-06 16:24 UTC #40 lore raw

In reply to Shaohua Li

Quoted 4 lines · Shaohua Li

Does below patch help? The nmi suspend/resume doesn't look good to me. Only CPU0 uses the suspend/resume code path. Other CPUs run the CPU hotplug code path.

Unfortunately this just oopses immediately on the first suspend. The stack trace is unclear (and I'm just going from memory at the moment), but it looked like it got an invalid op. I'll try to get a clearer idea of the crash later today.

Don Zickus

2006-06-06 16:46 UTC #41 lore raw

Quoted 11 lines · Jeremy Fitzhardinge

Quoted 4 lines · Shaohua Li
Does below patch help? The nmi suspend/resume doesn't look good to me. Only CPU0 uses the suspend/resume code path. Other CPUs run the CPU hotplug code path.
Unfortunately this just oopses immediately on the first suspend. The stack trace is unclear (and I'm just going from memory at the moment), but it looked like it got an invalid op. I'll try to get a clearer idea of the crash later today.
J

No this makes sense. The code right now just blindly tries to disable the watchdog without checking to see that it is already disabled. The oops you are seeing is a result of that. I'll have a patch to fix all that a little later.

Cheers,
Don

Don Zickus

2006-06-06 21:41 UTC #10 lore raw

In reply to Andi Kleen

Quoted 7 lines · Andi Kleen

Quoted 3 lines
Because he is using a i386 machine, the nmi watchdog is disabled by default.
I changed that - it's now on by default on i386 too.
-Andi

I am trying to create a patch for this problem and it just dawned on me, how does one store the previous state in a suspend/resume path if the code hotplugs all the cpus first? CPU0 is easy because an explicit suspend/resume path is called, but it seems to be called last after all the other cpus have been removed. How do I save the state?

Is there a recommened way of doing this? Or can I assume that __cpu_disable/enable is only called by the suspend/resume subsystem?

Thanks.

Cheers,
Don

Andrew Morton

2006-06-06 22:15 UTC #11 lore raw

Quoted 7 lines · On Tue, 6 Jun 2006 17:45:53 -0400 Don Zickus <[email protected]> wrote:

Quoted 6 lines · Andi Kleen
Quoted 3 lines
Because he is using a i386 machine, the nmi watchdog is disabled by default.
I changed that - it's now on by default on i386 too.

Quoted 7 lines

Quoted 1 line
-Andi
I am trying to create a patch for this problem and it just dawned on me, how does one store the previous state in a suspend/resume path if the code hotplugs all the cpus first? CPU0 is easy because an explicit suspend/resume path is called, but it seems to be called last after all the other cpus have been removed. How do I save the state?

I'm really struggling to understand this question. If you're referring to some per-cpu state then a CPU hotplug handler would be appropriate?

Don Zickus

2006-06-06 23:00 UTC #12 lore raw

Quoted 12 lines · Andrew Morton

Quoted 9 lines · On Tue, 6 Jun 2006 17:45:53 -0400 Don Zickus <[email protected]> wrote:
Quoted 7 lines · Andi Kleen
Quoted 3 lines
Because he is using a i386 machine, the nmi watchdog is disabled by default.
I changed that - it's now on by default on i386 too.
-Andi

Quoted 8 lines

Quoted 5 lines
I am trying to create a patch for this problem and it just dawned on me, how does one store the previous state in a suspend/resume path if the code hotplugs all the cpus first? CPU0 is easy because an explicit suspend/resume path is called, but it seems to be called last after all the other cpus have been removed. How do I save the state?
I'm really struggling to understand this question. If you're referring to some per-cpu state then a CPU hotplug handler would be appropriate?

Sorry. I got ahead of myself. My concern is how the suspend/resume code works with device drivers on an SMP system. My initial impression was that the subsystem registers with the suspend/resume layer and upon such actions those registered functions are called.

Inside those functions I saved the previous state of the watchdog timer. However, I learned today that my understanding was incorrect. Instead first the _hotplug_ code is called for every cpu _except_ cpu0. The _suspend/resume_ functions are only called in the context of _cpu0_.

This breaks the design I have because upon resuming the watchdog timers automatically start on all cpus (except cpu0 because I saved the previous state through the handlers), regardless of what the previous state was.

So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.

I didn't want to register a hotplug handler because a hotplug event is really different than a suspend event (I want to _save_ info during a suspend event). The documentation I was reading seemed to suggest that hotplug/suspend/smp was a work-in-progress.

Is the typical approach to just hack in an extra parameter to the start/stop functions of the nmi_watchdog letting the function know it is coming through the suspend/resume path?

Any tips, code, other docs would be helpful.

Cheers,
Don

Andrew Morton

2006-06-06 23:22 UTC #13 lore raw

Quoted 45 lines · On Tue, 6 Jun 2006 19:05:04 -0400 Don Zickus <[email protected]> wrote:

Quoted 12 lines · Andrew Morton
Quoted 9 lines · On Tue, 6 Jun 2006 17:45:53 -0400 Don Zickus <[email protected]> wrote:
Quoted 7 lines · Andi Kleen
Quoted 3 lines
Because he is using a i386 machine, the nmi watchdog is disabled by default.
I changed that - it's now on by default on i386 too.
-Andi
Quoted 8 lines
Quoted 5 lines
I am trying to create a patch for this problem and it just dawned on me, how does one store the previous state in a suspend/resume path if the code hotplugs all the cpus first? CPU0 is easy because an explicit suspend/resume path is called, but it seems to be called last after all the other cpus have been removed. How do I save the state?
I'm really struggling to understand this question. If you're referring to some per-cpu state then a CPU hotplug handler would be appropriate?
Sorry. I got ahead of myself. My concern is how the suspend/resume code works with device drivers on an SMP system. My initial impression was that the subsystem registers with the suspend/resume layer and upon such actions those registered functions are called.
Inside those functions I saved the previous state of the watchdog timer. However, I learned today that my understanding was incorrect. Instead first the _hotplug_ code is called for every cpu _except_ cpu0. The _suspend/resume_ functions are only called in the context of _cpu0_.
This breaks the design I have because upon resuming the watchdog timers automatically start on all cpus (except cpu0 because I saved the previous state through the handlers), regardless of what the previous state was.
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
I didn't want to register a hotplug handler because a hotplug event is really different than a suspend event (I want to _save_ info during a suspend event). The documentation I was reading seemed to suggest that hotplug/suspend/smp was a work-in-progress.

Quoted 6 lines

Is the typical approach to just hack in an extra parameter to the start/stop functions of the nmi_watchdog letting the function know it is coming through the suspend/resume path?
Any tips, code, other docs would be helpful.

OK... My understanding of how it works is that the cpu hotplug handlers are called early in the suspend process to take the CPUs down. Once all the APs are shut down, CPU0 will then proceed to handle the devices.

So if you want to save and restore per-cpu NMI state then doing it in the CPU hot-add and hot-remove handlers is appropriate. It will affect the behaviour of _real_ CPU hot-add and hot-remove as well. But in what appears to be a correct fashion.

All the above applies to suspend-to-disk. I don't know if suspend-to-RAM shuts down the APs.

Jeremy Fitzhardinge

author

2006-06-06 23:27 UTC #14 lore raw

Quoted 3 lines · Andrew Morton

All the above applies to suspend-to-disk. I don't know if suspend-to-RAM shuts down the APs.

I'm using suspend-to-mem and it looks like its unplugging/replugging all the CPUs.

The part of the question I don't quite understand is why this is considered per-CPU state? Surely NMI-watchdog is a system-wide thing? Or does this also tie into other uses of the performance registers which may be set per-CPU?

Andi Kleen

2006-06-06 23:34 UTC #15 lore raw

Quoted 3 lines

Or does this also tie into other uses of the performance registers which may be set per-CPU?

That's it. The registration is to properly share performance registers between oprofile, nmi watchdog and other users.

-Andi

Andi Kleen

2006-06-06 23:34 UTC #31 lore raw

Quoted 8 lines · Don Zickus

Inside those functions I saved the previous state of the watchdog timer. However, I learned today that my understanding was incorrect. Instead first the _hotplug_ code is called for every cpu _except_ cpu0. The _suspend/resume_ functions are only called in the context of _cpu0_.
This breaks the design I have because upon resuming the watchdog timers automatically start on all cpus (except cpu0 because I saved the previous state through the handlers), regardless of what the previous state was.

This means the design was incorrect for CPU hotplug (which needs to work anyways). suspend is just the most popular user of CPU hotplug.

Quoted 3 lines

So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.

Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.

Quoted 4 lines

I didn't want to register a hotplug handler because a hotplug event is really different than a suspend event (I want to _save_ info during a suspend event). The documentation I was reading seemed to suggest that hotplug/suspend/smp was a work-in-progress.

You need to disable the nmi watchdog on CPU hotunplug too, it's no good to keep the NMI running.

-Andi

Nigel Cunningham

2006-06-06 23:37 UTC #18 lore raw

Hi guys.

Back on board after the big shift :)

Quoted 58 lines · Andrew Morton

On Tue, 6 Jun 2006 19:05:04 -0400
Quoted 45 lines · Don Zickus
Quoted 18 lines · Andrew Morton
On Tue, 6 Jun 2006 17:45:53 -0400
Quoted 6 lines · Don Zickus
Quoted 5 lines · Andi Kleen
Quoted 2 lines
Because he is using a i386 machine, the nmi watchdog is disabled by default.
I changed that - it's now on by default on i386 too.
Quoted 8 lines
Quoted 1 line
-Andi
I am trying to create a patch for this problem and it just dawned on me, how does one store the previous state in a suspend/resume path if the code hotplugs all the cpus first? CPU0 is easy because an explicit suspend/resume path is called, but it seems to be called last after all the other cpus have been removed. How do I save the state?
Quoted 2 lines
I'm really struggling to understand this question. If you're referring to some per-cpu state then a CPU hotplug handler would be appropriate?
Sorry. I got ahead of myself. My concern is how the suspend/resume code works with device drivers on an SMP system. My initial impression was that the subsystem registers with the suspend/resume layer and upon such actions those registered functions are called.
Inside those functions I saved the previous state of the watchdog timer. However, I learned today that my understanding was incorrect. Instead first the _hotplug_ code is called for every cpu _except_ cpu0. The _suspend/resume_ functions are only called in the context of _cpu0_.
This breaks the design I have because upon resuming the watchdog timers automatically start on all cpus (except cpu0 because I saved the previous state through the handlers), regardless of what the previous state was.
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
I didn't want to register a hotplug handler because a hotplug event is really different than a suspend event (I want to _save_ info during a suspend event). The documentation I was reading seemed to suggest that hotplug/suspend/smp was a work-in-progress.
Quoted 5 lines
Is the typical approach to just hack in an extra parameter to the start/stop functions of the nmi_watchdog letting the function know it is coming through the suspend/resume path?
Any tips, code, other docs would be helpful.
OK... My understanding of how it works is that the cpu hotplug handlers are called early in the suspend process to take the CPUs down. Once all the APs are shut down, CPU0 will then proceed to handle the devices.

Quoted 7 lines

So if you want to save and restore per-cpu NMI state then doing it in the CPU hot-add and hot-remove handlers is appropriate. It will affect the behaviour of _real_ CPU hot-add and hot-remove as well. But in what appears to be a correct fashion.
All the above applies to suspend-to-disk. I don't know if suspend-to-RAM shuts down the APs.

I'm not sure about suspend to ram either, but I can confirm the rest:

* Hotplugging handlers should manage the state of secondary cpus, really disabling NMIs on unplug and only enabling NMIs if the boot processor has them enabled (assuming consistent behaviour across cpus is desired). At a hotplug event, both the hardware state and values of variables may not match the state at the end of a previous hotplug event (we may have powered off, or may have switched from a boot kernel to a suspended-to-disk one), so even if you know the hotplug event is synthesised, you may still need to treat it as uninitialised.
* Driver suspend and resume calls should only handle cpu0, and should not touch other processors. The same semantics regarding hardware state and values of variables apply here.

If you _really_ need to store in a variable what the hardware state was at the last suspend call, it is possible to use _nosave variables, but this isn't done or recommended at the moment. (I think it's a good idea, but that's just my opinion). The value of a _nosave variable will persist across the atomic restore of a suspend to disk kernel context, and since a drivers_suspend call is made before doing the atomic restore, you'll never get uninitialised values. I'm not sure what it would be helpful for, but there may be a case.

Hope this helps.

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Don Zickus

2006-06-06 23:38 UTC #16 lore raw

Quoted 8 lines · Jeremy Fitzhardinge

Quoted 3 lines · Andrew Morton
All the above applies to suspend-to-disk. I don't know if suspend-to-RAM shuts down the APs.
I'm using suspend-to-mem and it looks like its unplugging/replugging all the CPUs.

Quoted 6 lines

The part of the question I don't quite understand is why this is considered per-CPU state? Surely NMI-watchdog is a system-wide thing? Or does this also tie into other uses of the performance registers which may be set per-CPU?
J

The nmi watchdog is enable/disabled on a per-cpu basis. The fact that a single switch turns all of them on/off is just convienance. Adding in code to turn them on/off on a per-cpu basis just requires a simple user interface. It has been talked about before to deal with NUMA systems.

Cheers,
Don

Don Zickus

2006-06-06 23:51 UTC #32 lore raw

In reply to Andi Kleen

Quoted 6 lines

Quoted 3 lines
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.

So the assumption is treat all the cpus the same either all on or all off, no mixed mode (some cpus on, some cpus off). I guess I was trying to hard to work on the per-cpu level.

Quoted 8 lines

Quoted 5 lines
I didn't want to register a hotplug handler because a hotplug event is really different than a suspend event (I want to _save_ info during a suspend event). The documentation I was reading seemed to suggest that hotplug/suspend/smp was a work-in-progress.
You need to disable the nmi watchdog on CPU hotunplug too, it's no good to keep the NMI running.

Don't you want to make sure those CPUs are actually sleeping. :^D

-Don

Quoted 3 lines

-Andi

Nigel Cunningham

2006-06-07 00:04 UTC #33 lore raw

Hi.

Quoted 11 lines · Don Zickus

Quoted 7 lines
Quoted 4 lines
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.
So the assumption is treat all the cpus the same either all on or all off, no mixed mode (some cpus on, some cpus off). I guess I was trying to hard to work on the per-cpu level.

This sounds wrong to me. Shouldn't the the effect of hotunplugging a cpu be to put the driver in a state equivalent to if that cpu simply didn't exist? Unplugging shouldn't assume we're going to subsequently have either a driver suspend, or a replug.

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Jeremy Fitzhardinge

author

2006-06-07 00:06 UTC #19 lore raw

Quoted 2 lines · Nigel Cunningham

* Driver suspend and resume calls should only handle cpu0, and should not touch other processors. The same semantics regarding hardware state and values of variables apply here.

Isn't the trouble that in this case, the devices themselves are the CPUs, and so the CPUs themselves need to operate on their own state?

Or perhaps, to look at it another way, suspend/resume is just a special case of:

1. unplug cpus 1-N
2. [something]
3. re-plug cpus 1-N

where [something] in this case is "suspend cpu0".

But the problem is that there's nothing which keeps track of whether the re-plugged cpus 1-N are the "same" as the unplugged 1-N, and so nothing can apply the same per-cpu settings to them. In the suspend/resume case they clearly are, but in the general remove/add case, do you really want the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities? (Do we support heterogeneous CPUs anyway?)

Andi Kleen

2006-06-07 00:10 UTC #39 lore raw

Quoted 11 lines · Don Zickus

Quoted 7 lines
Quoted 4 lines
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.
So the assumption is treat all the cpus the same either all on or all off, no mixed mode (some cpus on, some cpus off). I guess I was trying to hard to work on the per-cpu level.

No, mixed should work of course.

Quoted 10 lines

Quoted 8 lines
Quoted 5 lines
I didn't want to register a hotplug handler because a hotplug event is really different than a suspend event (I want to _save_ info during a suspend event). The documentation I was reading seemed to suggest that hotplug/suspend/smp was a work-in-progress.
You need to disable the nmi watchdog on CPU hotunplug too, it's no good to keep the NMI running.
Don't you want to make sure those CPUs are actually sleeping. :^D

That is what i meant - they can't sleep if the NMI keeps running. Ok it would run only for a short time for local APIC, but longer for io apic

-Andi

Nigel Cunningham

2006-06-07 00:12 UTC #20 lore raw

Hi.

Quoted 15 lines · Jeremy Fitzhardinge

Quoted 3 lines · Nigel Cunningham
* Driver suspend and resume calls should only handle cpu0, and should not touch other processors. The same semantics regarding hardware state and values of variables apply here.
Isn't the trouble that in this case, the devices themselves are the CPUs, and so the CPUs themselves need to operate on their own state?
Or perhaps, to look at it another way, suspend/resume is just a special case of:
1. unplug cpus 1-N
2. [something]
3. re-plug cpus 1-N

Quoted 6 lines

where [something] in this case is "suspend cpu0".
But the problem is that there's nothing which keeps track of whether the re-plugged cpus 1-N are the "same" as the unplugged 1-N, and so nothing can apply the same per-cpu settings to them. In the suspend/resume case they clearly are, but in the general remove/add case, do you really want

It's probably safter to say "In the suspend/resume case, they may well be." It's not inconceivable that a system could be suspended, a faulty cpu replaced with another, and the system resumed. Hotplugging ought to handle that nicely.

Quoted 3 lines

the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities? (Do we support heterogeneous CPUs anyway?)

Indeed. I'm also not sure that there's necessarily a guarantee that cpus will be hotplugged in the same order. Perhaps those with more knowledge can clarify there.

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Andrew Morton

2006-06-07 00:24 UTC #21 lore raw

Quoted 7 lines · On Wed, 7 Jun 2006 10:13:49 +1000 Nigel Cunningham <[email protected]>…

Quoted 3 lines
the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities? (Do we support heterogeneous CPUs anyway?)
Indeed. I'm also not sure that there's necessarily a guarantee that cpus will be hotplugged in the same order. Perhaps those with more knowledge can clarify there.

It all depends on what we mean by "per-cpu state". If we were to remember that "CPU 7 needs 0x1234 in register 44" then that would be wrong. But remembering some high-level functional thing like "CPU 7 needs to run the NMI watchdog" is fine. The CPU bringup code can work out whether that is possible, and how to do it.

Jeremy Fitzhardinge

author

2006-06-07 00:26 UTC #26 lore raw

Quoted 2 lines · Nigel Cunningham

It's probably safter to say "In the suspend/resume case, they may well be." It's not inconceivable that a system could be suspended, a faulty cpu replaced with another, and the system resumed. Hotplugging ought to handle that nicely.

I think, in general, changing the hardware configuration of the system while its suspend is not supported. But perhaps someone who actually knows about this PM stuff has a more authoritative view...

Jeremy Fitzhardinge

author

2006-06-07 00:29 UTC #22 lore raw

Quoted 7 lines · Andrew Morton

It all depends on what we mean by "per-cpu state". If we were to remember that "CPU 7 needs 0x1234 in register 44" then that would be wrong. But remembering some high-level functional thing like "CPU 7 needs to run the NMI watchdog" is fine. The CPU bringup code can work out whether that is possible, and how to do it.

But all the performance counter stuff is very model-specific. I don't think there's any abstraction which would allow us to say "CPU 7 is measuring branch misprediction stalls in pipeline 2" in any way other than "needs 0x1234 in register 44".

Nigel Cunningham

2006-06-07 00:30 UTC #23 lore raw

Hi.

Quoted 12 lines · you

On Wed, 7 Jun 2006 10:13:49 +1000
Quoted 8 lines · Nigel Cunningham
Quoted 4 lines
the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities? (Do we support heterogeneous CPUs anyway?)
Indeed. I'm also not sure that there's necessarily a guarantee that cpus will be hotplugged in the same order. Perhaps those with more knowledge can clarify there.

Quoted 5 lines

It all depends on what we mean by "per-cpu state". If we were to remember that "CPU 7 needs 0x1234 in register 44" then that would be wrong. But remembering some high-level functional thing like "CPU 7 needs to run the NMI watchdog" is fine. The CPU bringup code can work out whether that is possible, and how to do it.

Does that imply that there's no danger of cpus being hotplugged in a different order (so that cpu7 becomes cpu5, for example)?

I guess I'm missing an understanding of why one cpu would need a different configuration to the rest. If it's related to the cpu number, then it shouldn't matter if a different cpu gets the number, should it? If it's related to the node that the cpu is on, perhaps the hotplugging code for the driver should be checking for the reason ("Am I on the node with the... ?") rather than the cpu number?

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Nigel Cunningham

2006-06-07 00:32 UTC #27 lore raw

Hi.

Quoted 9 lines · Jeremy Fitzhardinge

Quoted 4 lines · Nigel Cunningham
It's probably safter to say "In the suspend/resume case, they may well be." It's not inconceivable that a system could be suspended, a faulty cpu replaced with another, and the system resumed. Hotplugging ought to handle that nicely.
I think, in general, changing the hardware configuration of the system while its suspend is not supported. But perhaps someone who actually knows about this PM stuff has a more authoritative view...

In general, you're right because we don't have perfect hardware hotplugging yet. But cpu hotplugging is one area we do have, so it should work. (I ought to be one of those people, because I'm the author of the Suspend2 patches :) ... not that I'm claiming complete knowledge of all things related to suspending! )

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Andi Kleen

2006-06-07 00:33 UTC #24 lore raw

Quoted 12 lines · Andrew Morton

On Wed, 7 Jun 2006 10:13:49 +1000
Quoted 8 lines · Nigel Cunningham
Quoted 4 lines
the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities? (Do we support heterogeneous CPUs anyway?)
Indeed. I'm also not sure that there's necessarily a guarantee that cpus will be hotplugged in the same order. Perhaps those with more knowledge can clarify there.

Quoted 5 lines

It all depends on what we mean by "per-cpu state". If we were to remember that "CPU 7 needs 0x1234 in register 44" then that would be wrong. But remembering some high-level functional thing like "CPU 7 needs to run the NMI watchdog" is fine. The CPU bringup code can work out whether that is possible, and how to do it.

Actually the nmi watchdog state should be global, not per CPU. We want it to either work for the whole system or be completely disabled.

What is per CPU are the performance counter allocations, but these can be forgotten over CPU unplug/replug.

(ok this means oprofile might need to be restarted after suspend/resume, but I guess that's reasonable)

-Andi

Don Zickus

2006-06-07 00:37 UTC #34 lore raw

Quoted 11 lines · Nigel Cunningham

Hi.
Quoted 8 lines · Don Zickus
Quoted 7 lines
Quoted 4 lines
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.

Quoted 8 lines

Quoted 3 lines
So the assumption is treat all the cpus the same either all on or all off, no mixed mode (some cpus on, some cpus off). I guess I was trying to hard to work on the per-cpu level.
This sounds wrong to me. Shouldn't the the effect of hotunplugging a cpu be to put the driver in a state equivalent to if that cpu simply didn't exist? Unplugging shouldn't assume we're going to subsequently have either a driver suspend, or a replug.

This is my biggest problem or maybe my complete lack of understanding, is that I don't know how to determine what state I am in during a hotplug event, either a cpu removal or a suspend. Therefore I feel like I have to store some persistant data around _just_ in case this is a suspend event. Also at the opposite end, how to separate a cpu insert vs. a cpu resume. The different being initialize to a global state vs. initialize to a last known state.

I thought it would make more sense if a few more states were to the hotplug event list. For example, in addition to CPU_ONLINE and CPU_DEAD, there could also be something like CPU_SUSPEND, CPU_FREEZE, CPU_RESUME, and CPU_THAW.

Anyway, I am probably complicating the matter. I'll whip something up and post it for review.

Cheers,
Don

Quoted 9 lines

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Nigel Cunningham

2006-06-07 00:39 UTC #25 lore raw

In reply to Andi Kleen

Hi.

Quoted 13 lines · Andi Kleen

Quoted 12 lines · Andrew Morton
On Wed, 7 Jun 2006 10:13:49 +1000
Quoted 8 lines · Nigel Cunningham
Quoted 4 lines
the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities? (Do we support heterogeneous CPUs anyway?)
Indeed. I'm also not sure that there's necessarily a guarantee that cpus will be hotplugged in the same order. Perhaps those with more knowledge can clarify there.

Quoted 8 lines

Quoted 5 lines
It all depends on what we mean by "per-cpu state". If we were to remember that "CPU 7 needs 0x1234 in register 44" then that would be wrong. But remembering some high-level functional thing like "CPU 7 needs to run the NMI watchdog" is fine. The CPU bringup code can work out whether that is possible, and how to do it.
Actually the nmi watchdog state should be global, not per CPU. We want it to either work for the whole system or be completely disabled.

Ok. Now I get and fully agree with what you said earlier ("Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.").

Quoted 5 lines

What is per CPU are the performance counter allocations, but these can be forgotten over CPU unplug/replug.
(ok this means oprofile might need to be restarted after suspend/resume, but I guess that's reasonable)

Don't know enough in that area to say anything :>

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Nigel Cunningham

2006-06-07 00:49 UTC #35 lore raw

Hi.

Quoted 29 lines · Don Zickus

Quoted 11 lines · Nigel Cunningham
Hi.
Quoted 8 lines · Don Zickus
Quoted 7 lines
Quoted 4 lines
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.
Quoted 8 lines
Quoted 3 lines
So the assumption is treat all the cpus the same either all on or all off, no mixed mode (some cpus on, some cpus off). I guess I was trying to hard to work on the per-cpu level.
This sounds wrong to me. Shouldn't the the effect of hotunplugging a cpu be to put the driver in a state equivalent to if that cpu simply didn't exist? Unplugging shouldn't assume we're going to subsequently have either a driver suspend, or a replug.
This is my biggest problem or maybe my complete lack of understanding, is that I don't know how to determine what state I am in during a hotplug event, either a cpu removal or a suspend. Therefore I feel like I have to store some persistant data around _just_ in case this is a suspend event. Also at the opposite end, how to separate a cpu insert vs. a cpu resume. The different being initialize to a global state vs. initialize to a last known state.

Quoted 7 lines

I thought it would make more sense if a few more states were to the hotplug event list. For example, in addition to CPU_ONLINE and CPU_DEAD, there could also be something like CPU_SUSPEND, CPU_FREEZE, CPU_RESUME, and CPU_THAW.
Anyway, I am probably complicating the matter. I'll whip something up and post it for review.

Act like...

Unplug: It's going away, never to return.
Plug: It's just appeared from nowhere, is completely uninitialised and may be a different item to anything that happened to look the same before.
Suspend: It's going to be put into a low (possibly no-) power state. It's going to come back, and when it does, you want to be able to put it back in the state it's in prior to this call.
Resume: You want to restore the state you saved in memory when given the suspend call earlier.

Regarding _FREEZE, there is work in progress to add this. I haven't been following the conversation really closely recently, but my understanding is that you should expect it to be similar to suspend, except that you can guarantee that power will not be lost. All activity should be stopped so that you get a consistent state which you can restore in the resume call. Every suspend or freeze must be followed by a resume.

I'll add the linux-pm list to the cc, just in case I've gotten something wrong or the other guys want to comment and have missed this thread.

Hope this helps.

Regards,

Nigel

Signature · 4 lines

Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

Don Zickus

2006-06-07 02:45 UTC #42 lore raw

Makes the start/stop paths of nmi watchdog more robust to handle the suspend/resume cases more gracefully.

Signed-off-by Don Zickus <[email protected]>

---

Quoted 11 lines · Jeremy Fitzhardinge

Quoted 4 lines · Shaohua Li
Does below patch help? The nmi suspend/resume doesn't look good to me. Only CPU0 uses the suspend/resume code path. Other CPUs run the CPU hotplug code path.
Unfortunately this just oopses immediately on the first suspend. The stack trace is unclear (and I'm just going from memory at the moment), but it looked like it got an invalid op. I'll try to get a clearer idea of the crash later today.
J

Can you apply this patch on top of Shaohua's. This should fix all your suspend problems.

Inside the patch is a little hack to handle the scenario when we come out of resume we do _not_ want the nmi watchdog enabled (to match the case entering suspend).

Compiled but not tested, as I don't have easy access to my test machines right now. Mainly posted for Andrew to pick up for rc6-mm1.

Cheers,
Don

arch/i386/kernel/nmi.c +16 -2

Index: linux-don/arch/i386/kernel/nmi.c
===================================================================
@@ -745,6 +745,7 @@ static void stop_intel_arch_watchdog(voi
 
 void setup_apic_nmi_watchdog (void *unused)
 {
+	struct nmi_watchdog_ctlblk *wd = &__get_cpu_var(nmi_watchdog_ctlblk);
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * The NMI watchdog uses spinlocks (notifier chains, etc.),

@@ -761,6 +762,14 @@ void setup_apic_nmi_watchdog (void *unus
 	    (nmi_watchdog != NMI_IO_APIC))
 	    	return;
 
+	if (wd->enabled == 1)
+		return;
+
+	/* cheap hack to support suspend/resume */
+	/* if cpu0 is not active neither should the other cpus */
+	if ((smp_processor_id() != 0) && (atomic_read(&nmi_active) <= 0))
+		return;
+
 	if (nmi_watchdog == NMI_LOCAL_APIC) {
 		switch (boot_cpu_data.x86_vendor) {
 		case X86_VENDOR_AMD:

@@ -798,17 +807,22 @@ void setup_apic_nmi_watchdog (void *unus
 			return;
 		}
 	}
-	__get_cpu_var(nmi_watchdog_ctlblk.enabled) = 1;
+	wd->enabled = 1;
 	atomic_inc(&nmi_active);
 }
 
 void stop_apic_nmi_watchdog(void *unused)
 {
+	struct nmi_watchdog_ctlblk *wd = &__get_cpu_var(nmi_watchdog_ctlblk);
+
 	/* only support LOCAL and IO APICs for now */
 	if ((nmi_watchdog != NMI_LOCAL_APIC) &&
 	    (nmi_watchdog != NMI_IO_APIC))
 	    	return;
 
+	if (wd->enabled == 0)
+		return;
+
 	if (nmi_watchdog == NMI_LOCAL_APIC) {
 		switch (boot_cpu_data.x86_vendor) {
 		case X86_VENDOR_AMD:

@@ -836,7 +850,7 @@ void stop_apic_nmi_watchdog(void *unused
 			return;
 		}
 	}
-	__get_cpu_var(nmi_watchdog_ctlblk.enabled) = 0;
+	wd->enabled = 0;
 	atomic_dec(&nmi_active);
 }

arch/x86_64/kernel/nmi.c +16 -2

Index: linux-don/arch/x86_64/kernel/nmi.c
===================================================================
@@ -672,6 +672,7 @@ static void stop_intel_arch_watchdog(voi
 
 void setup_apic_nmi_watchdog(void *unused)
 {
+	struct nmi_watchdog_ctlblk *wd = &__get_cpu_var(nmi_watchdog_ctlblk);
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * The NMI watchdog uses spinlocks (notifier chains, etc.),

@@ -688,6 +689,14 @@ void setup_apic_nmi_watchdog(void *unuse
 	    (nmi_watchdog != NMI_IO_APIC))
 	    	return;
 
+	if (wd->enabled == 1)
+		return;
+
+	/* cheap hack to support suspend/resume */
+	/* if cpu0 is not active neither should the other cpus */
+	if ((smp_processor_id() != 0) && (atomic_read(&nmi_active) <= 0))
+		return;
+
 	if (nmi_watchdog == NMI_LOCAL_APIC) {
 		switch (boot_cpu_data.x86_vendor) {
 		case X86_VENDOR_AMD:

@@ -709,17 +718,22 @@ void setup_apic_nmi_watchdog(void *unuse
 			return;
 		}
 	}
-	__get_cpu_var(nmi_watchdog_ctlblk.enabled) = 1;
+	wd->enabled = 1;
 	atomic_inc(&nmi_active);
 }
 
 void stop_apic_nmi_watchdog(void *unused)
 {
+	struct nmi_watchdog_ctlblk *wd = &__get_cpu_var(nmi_watchdog_ctlblk);
+
 	/* only support LOCAL and IO APICs for now */
 	if ((nmi_watchdog != NMI_LOCAL_APIC) &&
 	    (nmi_watchdog != NMI_IO_APIC))
 	    	return;
 
+	if (wd->enabled == 0)
+		return;
+
 	if (nmi_watchdog == NMI_LOCAL_APIC) {
 		switch (boot_cpu_data.x86_vendor) {
 		case X86_VENDOR_AMD:

@@ -738,7 +752,7 @@ void stop_apic_nmi_watchdog(void *unused
 			return;
 		}
 	}
-	__get_cpu_var(nmi_watchdog_ctlblk.enabled) = 0;
+	wd->enabled = 0;
 	atomic_dec(&nmi_active);
 }

David Brownell

2006-06-07 03:29 UTC #36 lore raw

Quoted 3 lines · Nigel Cunningham

Suspend: It's going to be put into a low (possibly no-) power state. It's going to come back, and when it does, you want to be able to put it back in the state it's in prior to this call.

Not exactly. Suspended devices can in general can resume() into a RESET state in which case software reinit is appropriate ... or they can come back in the state that the suspend() left them in, modulo changes that may come from hot-unplugging hardware connected to that device. (Which may be a wakeup event, depending on system configuration.)

CPU suspend might have additional rules (just like for any pm-smart class of drivers), but those are the generic rules. Not that I think many platforms treat CPUs quite the same as other hardware! :)

I don't think the PM events -- suspend()/resume() -- should ever be entangled with hotplug events. The former apply to devices which are known; the latter are how they become known (or get forgotten).

Quoted 1 line

Every suspend or freeze must be followed by a resume.

Freeze is an optional nuance; it's basically OK to treat every suspend() as an "enter low power mode" suspend request, regardless of the event signified by its parameter. The canonical/main example of when it might _not_ do that is avoiding disk drive spindown on freeze durin swsusp.

It's a bit problematic just now to handle hot-unplug during suspend(), so the best advice just now is to make sure that if that's physically possible (like ejecting a PCMCIA/Cardbus adapter) then driver resume() checks whether the device is present, just like it checks for power-lost/reset.

- Dave

Jeremy Fitzhardinge

author

2006-06-07 04:25 UTC #28 lore raw

Quoted 1 line · Nigel Cunningham

In general, you're right because we don't have perfect hardware hotplugging yet. But cpu hotplugging is one area we do have, so it should work.

Well, it seems to me the general problem is generating the proper hotplug events. If you actually pull, say, a USB device, the usb subsystem will tell you about it as it happens. But if you can suspend the machine and then arbitrarily rearrange the hardware, then on resume you'd have to go over the current hardware state and compare it to the pre-suspend state and generate all those events. Or I guess you could just generate unplug events for everything at suspend and re-plug anything you find on resume. Sounds pretty heavyweight though.

Rafael J. Wysocki

2006-06-07 09:55 UTC #37 lore raw

Hi,

Quoted 19 lines · Don Zickus

Quoted 9 lines · Nigel Cunningham
Quoted 8 lines · Don Zickus
Quoted 7 lines
Quoted 4 lines
So my question is/was what is the proper way to handle processor level subsystems during the suspend/resume path on an SMP system. I really don't understand the hotplug path nor the suspend/resume path very well.
Make it work properly for CPU hotplug for individual CPU and then in suspend you take care of "global" state and the last CPU.
Quoted 8 lines
Quoted 3 lines
So the assumption is treat all the cpus the same either all on or all off, no mixed mode (some cpus on, some cpus off). I guess I was trying to hard to work on the per-cpu level.
This sounds wrong to me. Shouldn't the the effect of hotunplugging a cpu be to put the driver in a state equivalent to if that cpu simply didn't exist? Unplugging shouldn't assume we're going to subsequently have either a driver suspend, or a replug.

Quoted 7 lines

This is my biggest problem or maybe my complete lack of understanding, is that I don't know how to determine what state I am in during a hotplug event, either a cpu removal or a suspend. Therefore I feel like I have to store some persistant data around _just_ in case this is a suspend event. Also at the opposite end, how to separate a cpu insert vs. a cpu resume. The different being initialize to a global state vs. initialize to a last known state.

The original idea was to treat the nonboot CPUs as though they were removed.

Greetings,
Rafael

Andi Kleen

2006-06-07 16:33 UTC #43 lore raw

Quoted 2 lines · Don Zickus

Makes the start/stop paths of nmi watchdog more robust to handle the suspend/resume cases more gracefully.

Can someone with the problem please confirm the patch actually helps.

Thanks,

-Andi

Jeremy Fitzhardinge

author

2006-06-07 17:07 UTC #44 lore raw

Quoted 3 lines · Don Zickus

Makes the start/stop paths of nmi watchdog more robust to handle the suspend/resume cases more gracefully.

This solves the original symptom, but I'm seeing something else now. After the second resume, there's a noticable pause after it brings cpu 1 online. After the third resume it's a longer pause, and after the 4th it just hangs there. The system is up enough to respond to sysreq, but nothing in usermode seems to be actually running. I'll try and get a better understanding of what I'm seeing later today.

Don Zickus

2006-06-07 17:45 UTC #45 lore raw

Quoted 4 lines · Jeremy Fitzhardinge

Quoted 3 lines · Don Zickus
Makes the start/stop paths of nmi watchdog more robust to handle the suspend/resume cases more gracefully.

Quoted 8 lines

This solves the original symptom, but I'm seeing something else now. After the second resume, there's a noticable pause after it brings cpu 1 online. After the third resume it's a longer pause, and after the 4th it just hangs there. The system is up enough to respond to sysreq, but nothing in usermode seems to be actually running. I'll try and get a better understanding of what I'm seeing later today.
J

Can you do me a quick favor and 'cat /proc/interrupts |grep NMI' before each of your suspends. I want to double check a piece of code. Your bugzilla postings showed your system starting with no nmi watchdog running but after the resume the watchdog started running on cpu1. I am hoping I fixed that issue too.

Thanks.

Cheers,
Don

Jeremy Fitzhardinge

author

2006-06-07 21:31 UTC #46 lore raw

Quoted 6 lines · Don Zickus

Can you do me a quick favor and 'cat /proc/interrupts |grep NMI' before each of your suspends. I want to double check a piece of code. Your bugzilla postings showed your system starting with no nmi watchdog running but after the resume the watchdog started running on cpu1. I am hoping I fixed that issue too.

Yes, there are no NMIs on either CPU after resume.

Pavel Machek

2006-06-08 20:11 UTC #30 lore raw

Hi!

Quoted 10 lines

Quoted 6 lines
Is the typical approach to just hack in an extra parameter to the start/stop functions of the nmi_watchdog letting the function know it is coming through the suspend/resume path?
Any tips, code, other docs would be helpful.
OK... My understanding of how it works is that the cpu hotplug handlers are called early in the suspend process to take the CPUs down. Once all the APs are shut down, CPU0 will then proceed to handle the devices.

Yep.

Quoted 2 lines

All the above applies to suspend-to-disk. I don't know if suspend-to-RAM shuts down the APs.

It applies to suspend-to-ram, too.
Pavel

Signature · 1 line

Thanks for all the (sleeping) penguins.

Pavel Machek

2006-06-08 20:11 UTC #17 lore raw

Hi!

Quoted 12 lines

Quoted 11 lines
Quoted 3 lines
All the above applies to suspend-to-disk. I don't know if suspend-to-RAM shuts down the APs.
I'm using suspend-to-mem and it looks like its unplugging/replugging all the CPUs.
The part of the question I don't quite understand is why this is considered per-CPU state? Surely NMI-watchdog is a system-wide thing? Or does this also tie into other uses of the performance registers which may be set per-CPU?

Quoted 4 lines

The nmi watchdog is enable/disabled on a per-cpu basis. The fact that a single switch turns all of them on/off is just convienance. Adding in code to turn them on/off on a per-cpu basis just requires a simple user interface. It has been talked about before to deal with NUMA systems.

Does it make sense to run watchdog on cpu 1 but not on cpu 0? If user plugs cpu 2, should it get watchdog or not? If I unplug cpu 1 and plug it back, should it run watchdog or not?

I believe it should be per-system thing.
Pavel

Signature · 1 line

Thanks for all the (sleeping) penguins.

Pavel Machek

2006-06-08 20:13 UTC #29 lore raw

Hi!

Quoted 10 lines

But the problem is that there's nothing which keeps track of whether the re-plugged cpus 1-N are the "same" as the unplugged 1-N, and so nothing can apply the same per-cpu settings to them. In the suspend/resume case they clearly are, but in the general remove/add case, do you really want the new CPU to get the same state as the old one just because it ends up with the same logical CPU number? Perhaps, but what if it doesn't even have the same capabilities?

Quoted 2 lines

(Do we support heterogeneous
CPUs anyway?)

It works for some people, but it certainly falls into unsupported category.

Signature · 1 line

Thanks for all the (sleeping) penguins.

Pavel Machek

2006-06-08 20:28 UTC #38 lore raw