tick_broadcast_mask is not restored after a CPU has been offline/onlined

Regression 3.4 Paul E. McKenney 2 participants 6 messages Jul 30, 2012 – Jul 31, 2012 1 message not in the archive ACPI

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20120730151559.772d4055@feng-i7

Paul E. McKenney

author

2012-07-30 13:42 UTC #1 lore raw

Quoted 5 lines · Feng Tang

Hi All,
When I debugged a suspend/resume bug, I found that tick_broadcast_mask is not restored for a CPU after it is offline/onlined since kernel 3.4, while it's fine for 3.3.

Could you please try 3.5?

Quoted 29 lines

Further check show it is caused by the commit 9505626d7bfe
ACPI: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()
The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.
This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().
Signed-off-by Paul E. McKenney <[email protected]>
drivers/acpi/processor_idle.c +1 -2
index 0e8e2de..9e57b06 100644
@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
         * to make the code that updates C-States be called once.
         */
 
-       if (smp_processor_id() == 0 &&
-                       cpuidle_get_driver() == &acpi_idle_driver) {
+       if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
                cpuidle_pause_and_lock();
                /* Protect against cpu-hotplug */

Quoted 6 lines

The root cause is acpi_processor_cst_has_changed() will also be called when cpu_up() is run on cpu 0 to boot up other cpu, this commit will prevent the following code be run for that cpu, which triggers some side effect like the broadcast_mask is not restored.
I raise this problem up and I don't if revert is a good solution here.

Indeed, that would re-introduce the splats from unprotected use of smp_processor_id(). :-(

Thanx, Paul

Feng Tang

2012-07-30 15:12 UTC #2 lore raw

Hi Paul,

Quoted 8 lines · On Mon, 30 Jul 2012 06:39:13 -0700 "Paul E. McKenney"…

Quoted 5 lines · Feng Tang
Hi All,
When I debugged a suspend/resume bug, I found that tick_broadcast_mask is not restored for a CPU after it is offline/onlined since kernel 3.4, while it's fine for 3.3.
Could you please try 3.5?

Yes, it's the same for 3.5

Thanks,
Feng

Paul E. McKenney

author

2012-07-30 17:09 UTC #3 lore raw

Quoted 8 lines · Feng Tang

Hi Paul,
Quoted 3 lines · On Mon, 30 Jul 2012 06:39:13 -0700 "Paul E. McKenney"…
Quoted 2 lines · Feng Tang
Hi All,

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
When I debugged a suspend/resume bug, I found that tick_broadcast_mask is not restored for a CPU after it is offline/onlined since kernel 3.4, while it's fine for 3.3.
Could you please try 3.5?
Yes, it's the same for 3.5

Thank you for checking, Feng.

Len, the comment above the change says:

/*
* FIXME: Design the ACPI notification to make it once per
* system instead of once per-cpu. This condition is a hack
* to make the code that updates C-States be called once. */

Is it time for this design-level change? Or is there something obvious that I missed when fixing the smp_processor_id() splat?

I could revert back, but use raw_smp_processor_id() rather than smp_processor_id(), but that feels like papering over a problem rather than fixing it.

Thoughts?

Thanx, Paul

Paul E. McKenney

author

2012-07-30 17:48 UTC #4 lore raw

Quoted 27 lines · Paul E. McKenney

Quoted 8 lines · Feng Tang
Hi Paul,
Quoted 3 lines · On Mon, 30 Jul 2012 06:39:13 -0700 "Paul E. McKenney"…
Quoted 2 lines · Feng Tang
Hi All,
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
When I debugged a suspend/resume bug, I found that tick_broadcast_mask is not restored for a CPU after it is offline/onlined since kernel 3.4, while it's fine for 3.3.
Could you please try 3.5?
Yes, it's the same for 3.5
Thank you for checking, Feng.
Len, the comment above the change says:
/*
* FIXME: Design the ACPI notification to make it once per
* system instead of once per-cpu. This condition is a hack
* to make the code that updates C-States be called once. */

Quoted 6 lines

Is it time for this design-level change? Or is there something obvious that I missed when fixing the smp_processor_id() splat?
I could revert back, but use raw_smp_processor_id() rather than smp_processor_id(), but that feels like papering over a problem rather than fixing it.

But should papering be appropriate, here is the patch.

Thanx, Paul

------------------------------------------------------------------------

ACPI: Repair fix to unprotected smp_processor_id()

Commit 9505626d (ACPI: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()) introduced a suspend/resume bug. This commit therefore introduces a bug-for-bug compatible fix for the original problem.

Reported-by Feng Tang <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

drivers/acpi/processor_idle.c +2 -1

index 47a8caa..19c151a 100644
@@ -1218,7 +1218,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
+	if (raw_smp_processor_id() == 0 &&
+	    cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */

Feng Tang

2012-07-31 03:23 UTC #5 lore raw

Hi Paul,

Quoted 31 lines · On Mon, 30 Jul 2012 10:42:18 -0700 "Paul E. McKenney"…

Quoted 30 lines · Paul E. McKenney
Quoted 8 lines · Feng Tang
Hi Paul,
Quoted 3 lines · On Mon, 30 Jul 2012 06:39:13 -0700 "Paul E. McKenney"…
Quoted 2 lines · Feng Tang
Hi All,
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
When I debugged a suspend/resume bug, I found that tick_broadcast_mask is not restored for a CPU after it is offline/onlined since kernel 3.4, while it's fine for 3.3.
Could you please try 3.5?
Yes, it's the same for 3.5
Thank you for checking, Feng.
Len, the comment above the change says:
/*
* FIXME: Design the ACPI notification to make it once per
* system instead of once per-cpu. This condition is a hack
* to make the code that updates C-States be called once. */
Is it time for this design-level change? Or is there something obvious that I missed when fixing the smp_processor_id() splat?

Quoted 7 lines

Quoted 3 lines
I could revert back, but use raw_smp_processor_id() rather than smp_processor_id(), but that feels like papering over a problem rather than fixing it.
But should papering be appropriate, here is the patch.
Thanx, Paul

Just found and have a patch to fix a typo in acpi processor_driver.c, which could also fix this tick_broadcast_mask issue.

Patch is in https://lkml.org/lkml/2012/7/30/483

So I think we don't need this "papering over" patch :)

Thanks,
Feng

Quoted 29 lines

------------------------------------------------------------------------
ACPI: Repair fix to unprotected smp_processor_id()
Commit 9505626d (ACPI: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()) introduced a suspend/resume bug. This commit therefore introduces a bug-for-bug compatible fix for the original problem.
Reported-by Feng Tang <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
drivers/acpi/processor_idle.c +2 -1
index 47a8caa..19c151a 100644
@@ -1218,7 +1218,8 @@ int acpi_processor_cst_has_changed(struct
 acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
+	if (raw_smp_processor_id() == 0 &&
+	    cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
/* Protect against cpu-hotplug */

Paul E. McKenney

author

2012-07-31 04:10 UTC #6 lore raw

Quoted 42 lines · Feng Tang

Hi Paul,
Quoted 37 lines · On Mon, 30 Jul 2012 10:42:18 -0700 "Paul E. McKenney"…
Quoted 27 lines · Paul E. McKenney
Quoted 8 lines · Feng Tang
Hi Paul,
Quoted 3 lines · On Mon, 30 Jul 2012 06:39:13 -0700 "Paul E. McKenney"…
Quoted 2 lines · Feng Tang
Hi All,
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
When I debugged a suspend/resume bug, I found that tick_broadcast_mask is not restored for a CPU after it is offline/onlined since kernel 3.4, while it's fine for 3.3.
Could you please try 3.5?
Yes, it's the same for 3.5
Thank you for checking, Feng.
Len, the comment above the change says:
/*
* FIXME: Design the ACPI notification to make it once per
* system instead of once per-cpu. This condition is a hack
* to make the code that updates C-States be called once. */
Quoted 6 lines
Is it time for this design-level change? Or is there something obvious that I missed when fixing the smp_processor_id() splat?
I could revert back, but use raw_smp_processor_id() rather than smp_processor_id(), but that feels like papering over a problem rather than fixing it.
But should papering be appropriate, here is the patch.

Quoted 8 lines

Quoted 1 line
Thanx, Paul
Just found and have a patch to fix a typo in acpi processor_driver.c, which could also fix this tick_broadcast_mask issue.
Patch is in https://lkml.org/lkml/2012/7/30/483
So I think we don't need this "papering over" patch :)

Very good, I have dropped it.

Thanx, Paul

Quoted 33 lines

Thanks,
Feng
Quoted 29 lines
------------------------------------------------------------------------
ACPI: Repair fix to unprotected smp_processor_id()
Commit 9505626d (ACPI: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()) introduced a suspend/resume bug. This commit therefore introduces a bug-for-bug compatible fix for the original problem.
Reported-by Feng Tang <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
drivers/acpi/processor_idle.c +2 -1
index 47a8caa..19c151a 100644
@@ -1218,7 +1218,8 @@ int acpi_processor_cst_has_changed(struct
 acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
+	if (raw_smp_processor_id() == 0 &&
+	    cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
/* Protect against cpu-hotplug */