[PATCH v2 0/3] powerpc: bug fixes in pseries_mach_cpu_die()

[PATCH v2 0/3] powerpc: bug fixes in pseries_mach_cpu_die()

[Vaidyanathan Srinivasan]

Hi Ben,

The following set of patches fixes kernel stack reset issue and also
potential race conditions.

This fix should be applied in 2.6.33 stable tree onwards.

Problem description:

(1) Repeated offline/online operation on pseries with extended cede
processor feature will run over the kernel stack and crash since the
stack was not reset on resume from cede processor.

(2) The 'if' conditions and code has been slightly rearranged to avoid
any possible race conditions where preferred_offline_state can change
while the cpu is still executing the condition checks in
pseries_mach_cpu_die().  The new code has the CPU_STATE_OFFLINE as
default and also improved readability.  Thanks to Anton Blanchard for
pointing this out.

(3) There were too many noisy KERN_INFO printks on offline/online
operation.  Removed the printk's for now.  Other debug methods or
traceevents can be introduced at a later point.

The patch has been tested on large POWER machine with both
cede_offline=off case and the default cede_offline=on case.

Please apply in powerpc.git tree and push upstream.

Previous version of the patch can be found at:

[PATCH] powerpc: reset kernel stack on cpu online from cede state
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-February/080515.html

Thanks,
Vaidy

---

Vaidyanathan Srinivasan (3):
      powerpc: reset kernel stack on cpu online from cede state
      powerpc: move checks in pseries_mach_cpu_die()
      powerpc: reduce printk from pseries_mach_cpu_die()


 arch/powerpc/kernel/head_64.S                   |   11 ++++++
 arch/powerpc/platforms/pseries/hotplug-cpu.c    |   42 ++++++++---------------
 arch/powerpc/platforms/pseries/offline_states.h |    2 +
 3 files changed, 27 insertions(+), 28 deletions(-)

[PATCH v2 1/3] powerpc: reset kernel stack on cpu online from cede state

[Vaidyanathan Srinivasan]

	Cpu hotplug (offline) without dlpar operation will place cpu
	in cede state and the extended_cede_processor() function will
	return when resumed.

	Kernel stack pointer needs to be reset before
	start_secondary() is called to continue the online operation.

	Added new function start_secondary_resume() to do the above
	steps.

Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Cc: Gautham R Shenoy <ego@in.ibm.com>
---
 arch/powerpc/kernel/head_64.S                   |   11 +++++++++++
 arch/powerpc/platforms/pseries/hotplug-cpu.c    |    9 ++++-----
 arch/powerpc/platforms/pseries/offline_states.h |    2 +-
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 9258074..567cd57 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -615,6 +615,17 @@ _GLOBAL(start_secondary_prolog)
 	std	r3,0(r1)		/* Zero the stack frame pointer	*/
 	bl	.start_secondary
 	b	.
+/*
+ * Reset stack pointer and call start_secondary
+ * to continue with online operation when woken up
+ * from cede in cpu offline.
+ */
+_GLOBAL(start_secondary_resume)
+	ld	r1,PACAKSAVE(r13)	/* Reload kernel stack pointer */
+	li	r3,0
+	std	r3,0(r1)		/* Zero the stack frame pointer	*/
+	bl	.start_secondary
+	b	.
 #endif
 
 /*
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 6ea4698..9be7af4 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -146,12 +146,11 @@ static void pseries_mach_cpu_die(void)
 		unregister_slb_shadow(hwcpu, __pa(get_slb_shadow()));
 
 		/*
-		 * NOTE: Calling start_secondary() here for now to
-		 * start new context.
-		 * However, need to do it cleanly by resetting the
-		 * stack pointer.
+		 * Call to start_secondary_resume() will not return.
+		 * Kernel stack will be reset and start_secondary()
+		 * will be called to continue the online operation.
 		 */
-		start_secondary();
+		start_secondary_resume();
 
 	} else if (get_preferred_offline_state(cpu) == CPU_STATE_OFFLINE) {
 
diff --git a/arch/powerpc/platforms/pseries/offline_states.h b/arch/powerpc/platforms/pseries/offline_states.h
index 22574e0..da07256 100644
--- a/arch/powerpc/platforms/pseries/offline_states.h
+++ b/arch/powerpc/platforms/pseries/offline_states.h
@@ -14,5 +14,5 @@ extern void set_cpu_current_state(int cpu, enum cpu_state_vals state);
 extern enum cpu_state_vals get_preferred_offline_state(int cpu);
 extern void set_preferred_offline_state(int cpu, enum cpu_state_vals state);
 extern void set_default_offline_state(int cpu);
-extern int start_secondary(void);
+extern void start_secondary_resume(void);
 #endif

[PATCH v2 2/3] powerpc: move checks in pseries_mach_cpu_die()

[Vaidyanathan Srinivasan]

	Rearrange condition checks for better code readability and
	prevention of possible race conditions when
	preferred_offline_state can potentially change during the
	execution of pseries_mach_cpu_die().  The patch will make
	pseries_mach_cpu_die() put cpu in one of the consistent states
	and not hit the run over BUG()

Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Cc: Gautham R Shenoy <ego@in.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |   30 +++++++++++++-------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 9be7af4..39ecb40 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -140,25 +140,25 @@ static void pseries_mach_cpu_die(void)
 		if (!get_lppaca()->shared_proc)
 			get_lppaca()->donate_dedicated_cpu = 0;
 		get_lppaca()->idle = 0;
-	}
 
-	if (get_preferred_offline_state(cpu) == CPU_STATE_ONLINE) {
-		unregister_slb_shadow(hwcpu, __pa(get_slb_shadow()));
+		if (get_preferred_offline_state(cpu) == CPU_STATE_ONLINE) {
+			unregister_slb_shadow(hwcpu, __pa(get_slb_shadow()));
 
-		/*
-		 * Call to start_secondary_resume() will not return.
-		 * Kernel stack will be reset and start_secondary()
-		 * will be called to continue the online operation.
-		 */
-		start_secondary_resume();
+			/*
+			 * Call to start_secondary_resume() will not return.
+			 * Kernel stack will be reset and start_secondary()
+			 * will be called to continue the online operation.
+			 */
+			start_secondary_resume();
+		}
+	}
 
-	} else if (get_preferred_offline_state(cpu) == CPU_STATE_OFFLINE) {
+	/* Requested state is CPU_STATE_OFFLINE at this point */
+	WARN_ON(get_preferred_offline_state(cpu) != CPU_STATE_OFFLINE);
 
-		set_cpu_current_state(cpu, CPU_STATE_OFFLINE);
-		unregister_slb_shadow(hard_smp_processor_id(),
-					__pa(get_slb_shadow()));
-		rtas_stop_self();
-	}
+	set_cpu_current_state(cpu, CPU_STATE_OFFLINE);
+	unregister_slb_shadow(hwcpu, __pa(get_slb_shadow()));
+	rtas_stop_self();
 
 	/* Should never get here... */
 	BUG();

[PATCH v2 3/3] powerpc: reduce printk from pseries_mach_cpu_die()

[Vaidyanathan Srinivasan]

	Remove debug printks in pseries_mach_cpu_die().  These are
	noisy at runtime.  Traceevents can be added to instrument this
	section of code.

	The following KERN_INFO printks are removed:

	cpu 62 (hwid 62) returned from cede.
	Decrementer value = b2802fff Timebase value = 2fa8f95035f4a
	cpu 62 (hwid 62) got prodded to go online
	cpu 58 (hwid 58) ceding for offline with hint 2

Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Cc: Gautham R Shenoy <ego@in.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 39ecb40..b842378 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -122,21 +122,10 @@ static void pseries_mach_cpu_die(void)
 		if (!get_lppaca()->shared_proc)
 			get_lppaca()->donate_dedicated_cpu = 1;
 
-		printk(KERN_INFO
-			"cpu %u (hwid %u) ceding for offline with hint %d\n",
-			cpu, hwcpu, cede_latency_hint);
 		while (get_preferred_offline_state(cpu) == CPU_STATE_INACTIVE) {
 			extended_cede_processor(cede_latency_hint);
-			printk(KERN_INFO "cpu %u (hwid %u) returned from cede.\n",
-				cpu, hwcpu);
-			printk(KERN_INFO
-			"Decrementer value = %x Timebase value = %llx\n",
-			get_dec(), get_tb());
 		}
 
-		printk(KERN_INFO "cpu %u (hwid %u) got prodded to go online\n",
-			cpu, hwcpu);
-
 		if (!get_lppaca()->shared_proc)
 			get_lppaca()->donate_dedicated_cpu = 0;
 		get_lppaca()->idle = 0;