latencytop support for powerpc?

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

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.

index ff5217a..b1fd369 100644
@@ -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

index 9861f17..2b8c14d 100644
@@ -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);

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

---

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

index ff5217a..b1fd369 100644
@@ -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

index 9861f17..6a4fb00 100644
@@ -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);

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

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

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.

@@ -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);

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.

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