[PATCH 0/2] Improve Oprofile Callgraphs on i386/x86_64

[PATCH 0/2] Improve Oprofile Callgraphs on i386/x86_64

[jblunck]

Here are two patches that should improve Oprofile callgraphs inside the kernel
for i386/x86_64. I already send them to the oprofile-list yesterday but they
are still waiting for moderator approval.

Cheers,
Jan

[PATCH 1/2] oprofile: Make callgraph use dump_trace() on i386/x86_64

[jblunck]

[-- Attachment #1: patches.fixes/oprofile-backtrace-use-dump_trace.diff --]
[-- Type: text/plain, Size: 4212 bytes --]

This patch improves oprofile callgraphs for i386/x86_64. The old backtracing
code was unable to produce even kernel backtraces if the kernel wasn't
compiled with framepointers. The code now uses dump_trace().

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 arch/i386/oprofile/backtrace.c |  104 ++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 66 deletions(-)

--- a/arch/i386/oprofile/backtrace.c
+++ b/arch/i386/oprofile/backtrace.c
@@ -13,25 +13,45 @@
 #include <linux/mm.h>
 #include <asm/ptrace.h>
 #include <asm/uaccess.h>
+#include <asm/stacktrace.h>
 
-struct frame_head {
-	struct frame_head * ebp;
-	unsigned long ret;
-} __attribute__((packed));
+static void backtrace_warning_symbol(void *data, char *msg,
+				     unsigned long symbol)
+{
+	/* Ignore warnings */
+}
 
-static struct frame_head *
-dump_kernel_backtrace(struct frame_head * head)
+static void backtrace_warning(void *data, char *msg)
 {
-	oprofile_add_trace(head->ret);
+	/* Ignore warnings */
+}
 
-	/* frame pointers should strictly progress back up the stack
-	 * (towards higher addresses) */
-	if (head >= head->ebp)
-		return NULL;
+static int backtrace_stack(void *data, char *name)
+{
+	/* Yes, we want all stacks */
+	return 0;
+}
 
-	return head->ebp;
+static void backtrace_address(void *data, unsigned long addr)
+{
+	unsigned int *depth = data;
+
+	if ((*depth)--)
+		oprofile_add_trace(addr);
 }
 
+static struct stacktrace_ops backtrace_ops = {
+	.warning = backtrace_warning,
+	.warning_symbol = backtrace_warning_symbol,
+	.stack = backtrace_stack,
+	.address = backtrace_address,
+};
+
+struct frame_head {
+	struct frame_head * ebp;
+	unsigned long ret;
+} __attribute__((packed));
+
 static struct frame_head *
 dump_user_backtrace(struct frame_head * head)
 {
@@ -53,72 +73,24 @@ dump_user_backtrace(struct frame_head * 
 	return bufhead[0].ebp;
 }
 
-/*
- * |             | /\ Higher addresses
- * |             |
- * --------------- stack base (address of current_thread_info)
- * | thread info |
- * .             .
- * |    stack    |
- * --------------- saved regs->ebp value if valid (frame_head address)
- * .             .
- * --------------- saved regs->rsp value if x86_64
- * |             |
- * --------------- struct pt_regs * stored on stack if 32-bit
- * |             |
- * .             .
- * |             |
- * --------------- %esp
- * |             |
- * |             | \/ Lower addresses
- *
- * Thus, regs (or regs->rsp for x86_64) <-> stack base restricts the
- * valid(ish) ebp values. Note: (1) for x86_64, NMI and several other
- * exceptions use special stacks, maintained by the interrupt stack table
- * (IST). These stacks are set up in trap_init() in
- * arch/x86_64/kernel/traps.c. Thus, for x86_64, regs now does not point
- * to the kernel stack; instead, it points to some location on the NMI
- * stack. On the other hand, regs->rsp is the stack pointer saved when the
- * NMI occurred. (2) For 32-bit, regs->esp is not valid because the
- * processor does not save %esp on the kernel stack when interrupts occur
- * in the kernel mode.
- */
-#ifdef CONFIG_FRAME_POINTER
-static int valid_kernel_stack(struct frame_head * head, struct pt_regs * regs)
-{
-	unsigned long headaddr = (unsigned long)head;
-#ifdef CONFIG_X86_64
-	unsigned long stack = (unsigned long)regs->rsp;
-#else
-	unsigned long stack = (unsigned long)regs;
-#endif
-	unsigned long stack_base = (stack & ~(THREAD_SIZE - 1)) + THREAD_SIZE;
-
-	return headaddr > stack && headaddr < stack_base;
-}
-#else
-/* without fp, it's just junk */
-static int valid_kernel_stack(struct frame_head * head, struct pt_regs * regs)
-{
-	return 0;
-}
-#endif
-
-
 void
 x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_head *head;
+	unsigned long stack;
 
 #ifdef CONFIG_X86_64
 	head = (struct frame_head *)regs->rbp;
+	stack = regs->rsp;
 #else
 	head = (struct frame_head *)regs->ebp;
+	stack = regs->esp;
 #endif
 
 	if (!user_mode_vm(regs)) {
-		while (depth-- && valid_kernel_stack(head, regs))
-			head = dump_kernel_backtrace(head);
+		if (depth)
+			dump_trace(NULL, regs, (unsigned long *)stack,
+				   &backtrace_ops, &depth);
 		return;
 	}
 

-- 

Re: [PATCH 1/2] oprofile: Make callgraph use dump_trace() on i386/x86_64

[Andi Kleen]

On Friday 10 August 2007 15:35:29 jblunck@suse.de wrote:
> This patch improves oprofile callgraphs for i386/x86_64. The old backtracing
> code was unable to produce even kernel backtraces if the kernel wasn't
> compiled with framepointers. The code now uses dump_trace().

Hmm one issue i didn't notice before: with imprecise backtrace
the profiling could be a little random because even if the same
call chain is hit repeatedly the garbage left over stack entries also
reported could vary and then cause oprofile to put it into different
buckets. But there is probably not much that can be 
done about that.

-Andi

Re: [PATCH 1/2] oprofile: Make callgraph use dump_trace() on i386/x86_64

[Jan Blunck]

On Fri, Aug 10, Andi Kleen wrote:

> On Friday 10 August 2007 15:35:29 jblunck@suse.de wrote:
> > This patch improves oprofile callgraphs for i386/x86_64. The old backtracing
> > code was unable to produce even kernel backtraces if the kernel wasn't
> > compiled with framepointers. The code now uses dump_trace().
> 
> Hmm one issue i didn't notice before: with imprecise backtrace
> the profiling could be a little random because even if the same
> call chain is hit repeatedly the garbage left over stack entries also
> reported could vary and then cause oprofile to put it into different
> buckets. But there is probably not much that can be 
> done about that.

Yes, but before we didn't had any callgraphs for x86_64 since with
framepointers enabled the backtrace is having very strange results too.

I guess this is the best what we can achieve at the moment. Eventually when we
have fast, precise backtraces in the kernel the oprofile code benefits from
that automatically with this patch.

[PATCH 2/2] i386/x86_64: Introduce frame_pointer() and stack_pointer()

[jblunck]

[-- Attachment #1: patches.fixes/x86-introduce-stack_frame_pointer.diff --]
[-- Type: text/plain, Size: 1799 bytes --]

This patch defines frame_pointer() and stack_pointer() similar to the
already defined instruction_pointer(). Thus the oprofile code can be written
in a more readable fashion.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 arch/i386/oprofile/backtrace.c |   12 ++----------
 include/asm-i386/ptrace.h      |    2 ++
 include/asm-x86_64/ptrace.h    |    2 ++
 3 files changed, 6 insertions(+), 10 deletions(-)

--- a/arch/i386/oprofile/backtrace.c
+++ b/arch/i386/oprofile/backtrace.c
@@ -76,16 +76,8 @@ dump_user_backtrace(struct frame_head * 
 void
 x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
-	struct frame_head *head;
-	unsigned long stack;
-
-#ifdef CONFIG_X86_64
-	head = (struct frame_head *)regs->rbp;
-	stack = regs->rsp;
-#else
-	head = (struct frame_head *)regs->ebp;
-	stack = regs->esp;
-#endif
+	struct frame_head *head = (struct frame_head *)frame_pointer(regs);
+	unsigned long stack = stack_pointer(regs);
 
 	if (!user_mode_vm(regs)) {
 		if (depth)
--- a/include/asm-i386/ptrace.h
+++ b/include/asm-i386/ptrace.h
@@ -78,6 +78,8 @@ static inline int v8086_mode(struct pt_r
 }
 
 #define instruction_pointer(regs) ((regs)->eip)
+#define frame_pointer(regs) ((regs)->ebp)
+#define stack_pointer(regs) ((regs)->esp)
 #define regs_return_value(regs) ((regs)->eax)
 
 extern unsigned long profile_pc(struct pt_regs *regs);
--- a/include/asm-x86_64/ptrace.h
+++ b/include/asm-x86_64/ptrace.h
@@ -39,6 +39,8 @@ struct pt_regs {
 #define user_mode(regs) (!!((regs)->cs & 3))
 #define user_mode_vm(regs) user_mode(regs)
 #define instruction_pointer(regs) ((regs)->rip)
+#define frame_pointer(regs) ((regs)->rbp)
+#define stack_pointer(regs) ((regs)->rsp)
 #define regs_return_value(regs) ((regs)->rax)
 
 extern unsigned long profile_pc(struct pt_regs *regs);

--