latencytop support for powerpc?

latencytop support for powerpc?

[Chris Friesen]

Just wondering if anyone has looked at what it would take to support 
latency top on powerpc?

I've got a dual G5 and I'd like to be able to track causes of latency. 
Based on the s390 implementation it doesn't look all that complicated 
for someone who understands what's going on...

Chris

Re: latencytop support for powerpc?

[Arnd Bergmann]

On Thursday 10 July 2008, Chris Friesen wrote:
> Just wondering if anyone has looked at what it would take to support 
> latency top on powerpc?

Just guessing something like the patch below, not more that that, but
without the bugs ;-). Haven't tested at all, so it will probably
fall apart, but give it a try if you like.

Subject: powerpc: support for latencytop

Implement save_stack_trace_tsk on powerpc, so that we can run with
latencytop.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index ff5217a..b1fd369 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -53,6 +53,9 @@ config STACKTRACE_SUPPORT
 	bool
 	default y
 
+config HAVE_LATENCYTOP_SUPPORT
+	def_bool y
+
 config TRACE_IRQFLAGS_SUPPORT
 	bool
 	depends on PPC64
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 9861f17..2b8c14d 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -10,19 +10,18 @@
  *      2 of the License, or (at your option) any later version.
  */
 
+#include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 #include <asm/ptrace.h>
+#include <asm/processor.h>
 
 /*
  * Save stack-backtrace addresses into a stack_trace buffer.
  */
-void save_stack_trace(struct stack_trace *trace)
+static void save_context_stack(struct stack_trace *trace, unsigned long sp,
+			struct task_struct *tsk, int savesched)
 {
-	unsigned long sp;
-
-	asm("mr %0,1" : "=r" (sp));
-
 	for (;;) {
 		unsigned long *stack = (unsigned long *) sp;
 		unsigned long newsp, ip;
@@ -33,10 +32,12 @@ void save_stack_trace(struct stack_trace *trace)
 		newsp = stack[0];
 		ip = stack[STACK_FRAME_LR_SAVE];
 
-		if (!trace->skip)
-			trace->entries[trace->nr_entries++] = ip;
-		else
-			trace->skip--;
+		if (savesched || !in_sched_functions(ip)) {
+			if (!trace->skip)
+				trace->entries[trace->nr_entries++] = ip;
+			else
+				trace->skip--;
+		}
 
 		if (trace->nr_entries >= trace->max_entries)
 			return;
@@ -44,4 +45,19 @@ void save_stack_trace(struct stack_trace *trace)
 		sp = newsp;
 	}
 }
+
+void save_stack_trace(struct stack_trace *trace)
+{
+	unsigned long sp;
+
+	asm("mr %0,1" : "=r" (sp));
+
+	save_context_stack(trace, sp, current, 1);
+}
 EXPORT_SYMBOL_GPL(save_stack_trace);
+
+void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+{
+	save_context_stack(trace, tsk->thread.regs->gpr[1], tsk, 0);
+}
+EXPORT_SYMBOL_GPL(save_stack_trace_tsk);

[PATCH] powerpc: support for latencytop

[Arnd Bergmann]

Implement save_stack_trace_tsk on powerpc, so that we can run with
latencytop.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
On Thursday 10 July 2008, Arnd Bergmann wrote:
> Just guessing something like the patch below, not more that that, but
> without the bugs ;-).

Obvious bug #1: needs to use the task we want to dump, not 'current'.

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index ff5217a..b1fd369 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -53,6 +53,9 @@ config STACKTRACE_SUPPORT
 	bool
 	default y
 
+config HAVE_LATENCYTOP_SUPPORT
+	def_bool y
+
 config TRACE_IRQFLAGS_SUPPORT
 	bool
 	depends on PPC64
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 9861f17..6a4fb00 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -10,33 +10,34 @@
  *      2 of the License, or (at your option) any later version.
  */
 
+#include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 #include <asm/ptrace.h>
+#include <asm/processor.h>
 
 /*
  * Save stack-backtrace addresses into a stack_trace buffer.
  */
-void save_stack_trace(struct stack_trace *trace)
+static void save_context_stack(struct stack_trace *trace, unsigned long sp,
+			struct task_struct *tsk, int savesched)
 {
-	unsigned long sp;
-
-	asm("mr %0,1" : "=r" (sp));
-
 	for (;;) {
 		unsigned long *stack = (unsigned long *) sp;
 		unsigned long newsp, ip;
 
-		if (!validate_sp(sp, current, STACK_FRAME_OVERHEAD))
+		if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
 			return;
 
 		newsp = stack[0];
 		ip = stack[STACK_FRAME_LR_SAVE];
 
-		if (!trace->skip)
-			trace->entries[trace->nr_entries++] = ip;
-		else
-			trace->skip--;
+		if (savesched || !in_sched_functions(ip)) {
+			if (!trace->skip)
+				trace->entries[trace->nr_entries++] = ip;
+			else
+				trace->skip--;
+		}
 
 		if (trace->nr_entries >= trace->max_entries)
 			return;
@@ -44,4 +45,19 @@ void save_stack_trace(struct stack_trace *trace)
 		sp = newsp;
 	}
 }
+
+void save_stack_trace(struct stack_trace *trace)
+{
+	unsigned long sp;
+
+	asm("mr %0,1" : "=r" (sp));
+
+	save_context_stack(trace, sp, current, 1);
+}
 EXPORT_SYMBOL_GPL(save_stack_trace);
+
+void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+{
+	save_context_stack(trace, tsk->thread.regs->gpr[1], tsk, 0);
+}
+EXPORT_SYMBOL_GPL(save_stack_trace_tsk);

Re: [PATCH] powerpc: support for latencytop

[Nathan Lynch]

Arnd Bergmann wrote:
> Implement save_stack_trace_tsk on powerpc, so that we can run with
> latencytop.

So I tried latencytop with linux-next and got the following oops, but
I didn't really look into it yet.

Unable to handle kernel paging request for data at address 0x00000008
Faulting instruction address: 0xc00000000002d114
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=32 pSeries
Modules linked in:
NIP: c00000000002d114 LR: c0000000005d3c68 CTR: 0000000000000000
REGS: c0000000e9137580 TRAP: 0300   Not tainted  (2.6.26-next-20080716-08333-g49de20b)
MSR: 8000000000009032 <EE,ME,IR,DR>  CR: 24000040  XER: 20000018
DAR: 0000000000000008, DSISR: 0000000040000000
TASK = c0000000e9138000[9] 'migration/2' THREAD: c0000000e9134000 CPU: 2
GPR00: c0000000005d3c68 c0000000e9137800 c000000000a54fc8 c0000000e91378f0 
GPR04: c0000000e91378f0 c0000000e91e2750 0000000000000000 0000000000000002 
GPR08: c0000000011154e0 0000000000000000 0000000000000400 c000000000a876c0 
GPR12: 0000000044000042 c000000000a90800 00000000002146f4 0000000000214744 
GPR16: 4000000002040000 0000000002903b58 c0000000e90bff10 00000000ffffffff 
GPR20: c0000000010aec30 c000000000a95dc8 0000000000000010 0000000000000003 
GPR24: 0000000000000001 c0000000011154a0 c0000000e91e2750 c000000000eedf68 
GPR28: c0000000e9137908 0000000000000003 c0000000009c0c30 c0000000e9137800 
NIP [c00000000002d114] .save_stack_trace_tsk+0x24/0x40
LR [c0000000005d3c68] .account_scheduler_latency+0xa4/0x2dc
Call Trace:
[c0000000e9137800] [c0000000e9137850] 0xc0000000e9137850 (unreliable)
[c0000000e9137880] [c0000000005d3c68] .account_scheduler_latency+0xa4/0x2dc
[c0000000e91379b0] [c0000000000794e8] .enqueue_task_fair+0x1e0/0x24c
[c0000000e9137a60] [c000000000074be4] .enqueue_task+0x80/0xb0
[c0000000e9137af0] [c000000000074c50] .activate_task+0x3c/0x64
[c0000000e9137b80] [c00000000007a5c8] .try_to_wake_up+0x2a0/0x384
[c0000000e9137c50] [c00000000007a6d0] .default_wake_function+0x24/0x38
[c0000000e9137cd0] [c00000000007438c] .__wake_up_common+0x74/0xec
[c0000000e9137d90] [c0000000000765c0] .complete+0x5c/0x8c
[c0000000e9137e30] [c00000000007eaa4] .migration_thread+0x2a4/0x33c
[c0000000e9137f00] [c0000000000a1da4] .kthread+0x84/0xd4
[c0000000e9137f90] [c000000000029ce4] .kernel_thread+0x4c/0x68
Instruction dump:
7c0803a6 7d808120 4e800020 7c0802a6 fbe1fff0 7c651b78 7c832378 38c00000 
f8010010 f821ff81 e9250478 7c3f0b78 <e8890008> 4bfffef5 e8210000 e8010010 
---[ end trace e35e4a6e751f309e ]---

[PATCH] powerpc: fix support for latencytop

[Arnd Bergmann]

We need to pass the kernel stack pointer instead of the user space
stack pointer in save_stack_trace_tsk().

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

On Wednesday 16 July 2008, Nathan Lynch wrote:
> Arnd Bergmann wrote:
> > Implement save_stack_trace_tsk on powerpc, so that we can run with
> > latencytop.
> 
> So I tried latencytop with linux-next and got the following oops, but
> I didn't really look into it yet.

Oh, I didn't even realize that benh had merged that patch of mine.
As I wrote in the description, it was entirely untested. You found
another obvious bug: The code was passing the user space stack
pointer instead of the kernel stack pointer.

Again, this patch is entirely untested, and I would not be at all
surprised to find other trivial bugs.

--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -59,6 +59,6 @@ EXPORT_SYMBOL_GPL(save_stack_trace);
 
 void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 {
-	save_context_stack(trace, tsk->thread.regs->gpr[1], tsk, 0);
+	save_context_stack(trace, tsk->thread.ksp, tsk, 0);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_tsk);

Re: [PATCH] powerpc: fix support for latencytop

[Benjamin Herrenschmidt]

On Thu, 2008-07-17 at 00:12 +0200, Arnd Bergmann wrote:
> We need to pass the kernel stack pointer instead of the user space
> stack pointer in save_stack_trace_tsk().
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> On Wednesday 16 July 2008, Nathan Lynch wrote:
> > Arnd Bergmann wrote:
> > > Implement save_stack_trace_tsk on powerpc, so that we can run with
> > > latencytop.
> > 
> > So I tried latencytop with linux-next and got the following oops, but
> > I didn't really look into it yet.
> 
> Oh, I didn't even realize that benh had merged that patch of mine.
> As I wrote in the description, it was entirely untested. You found
> another obvious bug: The code was passing the user space stack
> pointer instead of the kernel stack pointer.
> 
> Again, this patch is entirely untested, and I would not be at all
> surprised to find other trivial bugs.

I missed that part of your description and it didn't seem to break the
kernel stack trace so ... :-) It's a nice feature to have and bugs can
be fixed before release.

Now we should probably look at making the stacktrace code a bit more
robust vs. wild pointers anyway.

> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -59,6 +59,6 @@ EXPORT_SYMBOL_GPL(save_stack_trace);
>  
>  void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
>  {
> -	save_context_stack(trace, tsk->thread.regs->gpr[1], tsk, 0);
> +	save_context_stack(trace, tsk->thread.ksp, tsk, 0);
>  }
>  EXPORT_SYMBOL_GPL(save_stack_trace_tsk);

Re: [PATCH] powerpc: fix support for latencytop

[Nathan Lynch]

Arnd Bergmann wrote:
> We need to pass the kernel stack pointer instead of the user space
> stack pointer in save_stack_trace_tsk().

Thanks, this fixes it, and latencytop even seems to work. :)

Re: [PATCH] powerpc: fix support for latencytop

[Chris Friesen]

Nathan Lynch wrote:
> Arnd Bergmann wrote:
> 
>>We need to pass the kernel stack pointer instead of the user space
>>stack pointer in save_stack_trace_tsk().
> 
> 
> Thanks, this fixes it, and latencytop even seems to work. :)

Sweet.  One week from mention to working in -next...not bad.

My thanks to Arnd and Nathan for implementing and testing it out.

Chris