linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 -tip 1/4] x86, dumpstack: Correct stack dump info when frame pointer is available
@ 2011-03-08 11:44 Namhyung Kim
  2011-03-08 11:44 ` [PATCH v4 -tip 2/4] x86, dumpstack: Random printk changes Namhyung Kim
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Namhyung Kim @ 2011-03-08 11:44 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Frederic Weisbecker
  Cc: x86, linux-kernel, Soren Sandmann, Peter Zijlstra,
	Paul Mackerras, Arnaldo Carvalho de Melo

Current stack dump code scans entire stack and check each entry
contains a pointer to kernel code. If CONFIG_FRAME_POINTER=y it
could mark whether the pointer is valid or not based on value of
the frame pointer. Invalid entries could be preceded by '?' sign.

However this was not going to happen because scan start point was
always higher than the frame pointer so that they could not meet.

Commit 9c0729dc8062 ("x86: Eliminate bp argument from the stack
tracing routines") delayed bp acquisition point, so the bp was
read in lower frame, thus all of the entries were marked invalid.

This patch fixes this by reverting above commit while retaining
stack_frame() helper as suggested by Frederic Weisbecker.
End result looks like below:

before:
[    3.508329] Call Trace:
[    3.508551]  [<ffffffff814f35c9>] ? panic+0x91/0x199
[    3.508662]  [<ffffffff814f3739>] ? printk+0x68/0x6a
[    3.508770]  [<ffffffff81a981b2>] ? mount_block_root+0x257/0x26e
[    3.508876]  [<ffffffff81a9821f>] ? mount_root+0x56/0x5a
[    3.508975]  [<ffffffff81a98393>] ? prepare_namespace+0x170/0x1a9
[    3.509216]  [<ffffffff81a9772b>] ? kernel_init+0x1d2/0x1e2
[    3.509335]  [<ffffffff81003894>] ? kernel_thread_helper+0x4/0x10
[    3.509442]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
[    3.509542]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
[    3.509641]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10

after:
[    3.522991] Call Trace:
[    3.523351]  [<ffffffff814f35b9>] panic+0x91/0x199
[    3.523468]  [<ffffffff814f3729>] ? printk+0x68/0x6a
[    3.523576]  [<ffffffff81a981b2>] mount_block_root+0x257/0x26e
[    3.523681]  [<ffffffff81a9821f>] mount_root+0x56/0x5a
[    3.523780]  [<ffffffff81a98393>] prepare_namespace+0x170/0x1a9
[    3.523885]  [<ffffffff81a9772b>] kernel_init+0x1d2/0x1e2
[    3.523987]  [<ffffffff81003894>] kernel_thread_helper+0x4/0x10
[    3.524228]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
[    3.524345]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
[    3.524445]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Soren Sandmann <ssp@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
v4:
 * use 0 instead of regs->bp
 * separate out printk changes

v3:
 * apply comment from Frederic
 * add a couple of printk fixes

 arch/x86/include/asm/kdebug.h     |    2 +-
 arch/x86/include/asm/stacktrace.h |    6 +++---
 arch/x86/kernel/cpu/perf_event.c  |    2 +-
 arch/x86/kernel/dumpstack.c       |   14 ++++++++------
 arch/x86/kernel/dumpstack_32.c    |   15 ++++++++-------
 arch/x86/kernel/dumpstack_64.c    |   14 +++++++-------
 arch/x86/kernel/process.c         |    2 +-
 arch/x86/kernel/stacktrace.c      |    6 +++---
 8 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index ca242d35e873..e28ec43a0737 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -26,7 +26,7 @@ extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
 extern void show_registers(struct pt_regs *regs);
 extern void show_trace(struct task_struct *t, struct pt_regs *regs,
-		       unsigned long *sp);
+		       unsigned long *sp, unsigned long bp);
 extern void __show_regs(struct pt_regs *regs, int all);
 extern void show_regs(struct pt_regs *regs);
 extern unsigned long oops_begin(void);
diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 52b5c7ed3608..d7e89c83645d 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -47,7 +47,7 @@ struct stacktrace_ops {
 };
 
 void dump_trace(struct task_struct *tsk, struct pt_regs *regs,
-		unsigned long *stack,
+		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data);
 
 #ifdef CONFIG_X86_32
@@ -86,11 +86,11 @@ stack_frame(struct task_struct *task, struct pt_regs *regs)
 
 extern void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *stack, char *log_lvl);
+		   unsigned long *stack, unsigned long bp, char *log_lvl);
 
 extern void
 show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *sp, char *log_lvl);
+		   unsigned long *sp, unsigned long bp, char *log_lvl);
 
 extern unsigned int code_bytes;
 
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 9d977a2ea693..d19cdc2d602e 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1792,7 +1792,7 @@ perf_callchain_kernel(struct perf_callchain_entry *entry, struct pt_regs *regs)
 
 	perf_callchain_store(entry, regs->ip);
 
-	dump_trace(NULL, regs, NULL, &backtrace_ops, entry);
+	dump_trace(NULL, regs, NULL, 0, &backtrace_ops, entry);
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index df20723a6a1b..c330160b6da3 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -175,21 +175,21 @@ static const struct stacktrace_ops print_trace_ops = {
 
 void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		unsigned long *stack, char *log_lvl)
+		unsigned long *stack, unsigned long bp, char *log_lvl)
 {
 	printk("%sCall Trace:\n", log_lvl);
-	dump_trace(task, regs, stack, &print_trace_ops, log_lvl);
+	dump_trace(task, regs, stack, bp, &print_trace_ops, log_lvl);
 }
 
 void show_trace(struct task_struct *task, struct pt_regs *regs,
-		unsigned long *stack)
+		unsigned long *stack, unsigned long bp)
 {
-	show_trace_log_lvl(task, regs, stack, "");
+	show_trace_log_lvl(task, regs, stack, bp, "");
 }
 
 void show_stack(struct task_struct *task, unsigned long *sp)
 {
-	show_stack_log_lvl(task, NULL, sp, "");
+	show_stack_log_lvl(task, NULL, sp, 0, "");
 }
 
 /*
@@ -197,14 +197,16 @@ void show_stack(struct task_struct *task, unsigned long *sp)
  */
 void dump_stack(void)
 {
+	unsigned long bp;
 	unsigned long stack;
 
+	bp = stack_frame(current, NULL);
 	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
 		current->pid, current->comm, print_tainted(),
 		init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
 		init_utsname()->version);
-	show_trace(NULL, NULL, &stack);
+	show_trace(NULL, NULL, &stack, bp);
 }
 EXPORT_SYMBOL(dump_stack);
 
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 74cc1eda384b..3b97a80ce329 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -17,12 +17,11 @@
 #include <asm/stacktrace.h>
 
 
-void dump_trace(struct task_struct *task,
-		struct pt_regs *regs, unsigned long *stack,
+void dump_trace(struct task_struct *task, struct pt_regs *regs,
+		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)
 {
 	int graph = 0;
-	unsigned long bp;
 
 	if (!task)
 		task = current;
@@ -35,7 +34,9 @@ void dump_trace(struct task_struct *task,
 			stack = (unsigned long *)task->thread.sp;
 	}
 
-	bp = stack_frame(task, regs);
+	if (!bp)
+		bp = stack_frame(task, regs);
+
 	for (;;) {
 		struct thread_info *context;
 
@@ -55,7 +56,7 @@ EXPORT_SYMBOL(dump_trace);
 
 void
 show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *sp, char *log_lvl)
+		   unsigned long *sp, unsigned long bp, char *log_lvl)
 {
 	unsigned long *stack;
 	int i;
@@ -77,7 +78,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 		touch_nmi_watchdog();
 	}
 	printk(KERN_CONT "\n");
-	show_trace_log_lvl(task, regs, sp, log_lvl);
+	show_trace_log_lvl(task, regs, sp, bp, log_lvl);
 }
 
 
@@ -102,7 +103,7 @@ void show_registers(struct pt_regs *regs)
 		u8 *ip;
 
 		printk(KERN_EMERG "Stack:\n");
-		show_stack_log_lvl(NULL, regs, &regs->sp, KERN_EMERG);
+		show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
 
 		printk(KERN_EMERG "Code: ");
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index a6b6fcf7f0ae..e71c98d3c0d2 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -139,8 +139,8 @@ fixup_bp_irq_link(unsigned long bp, unsigned long *stack,
  * severe exception (double fault, nmi, stack fault, debug, mce) hardware stack
  */
 
-void dump_trace(struct task_struct *task,
-		struct pt_regs *regs, unsigned long *stack,
+void dump_trace(struct task_struct *task, struct pt_regs *regs,
+		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)
 {
 	const unsigned cpu = get_cpu();
@@ -150,7 +150,6 @@ void dump_trace(struct task_struct *task,
 	struct thread_info *tinfo;
 	int graph = 0;
 	unsigned long dummy;
-	unsigned long bp;
 
 	if (!task)
 		task = current;
@@ -161,7 +160,8 @@ void dump_trace(struct task_struct *task,
 			stack = (unsigned long *)task->thread.sp;
 	}
 
-	bp = stack_frame(task, regs);
+	if (!bp)
+		bp = stack_frame(task, regs);
 	/*
 	 * Print function call entries in all stacks, starting at the
 	 * current stack address. If the stacks consist of nested
@@ -225,7 +225,7 @@ EXPORT_SYMBOL(dump_trace);
 
 void
 show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *sp, char *log_lvl)
+		   unsigned long *sp, unsigned long bp, char *log_lvl)
 {
 	unsigned long *irq_stack_end;
 	unsigned long *irq_stack;
@@ -269,7 +269,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	preempt_enable();
 
 	printk(KERN_CONT "\n");
-	show_trace_log_lvl(task, regs, sp, log_lvl);
+	show_trace_log_lvl(task, regs, sp, bp, log_lvl);
 }
 
 void show_registers(struct pt_regs *regs)
@@ -298,7 +298,7 @@ void show_registers(struct pt_regs *regs)
 
 		printk(KERN_EMERG "Stack:\n");
 		show_stack_log_lvl(NULL, regs, (unsigned long *)sp,
-				   KERN_EMERG);
+				   0, KERN_EMERG);
 
 		printk(KERN_EMERG "Code: ");
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index ff4554198981..f4c6cc40168e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -86,7 +86,7 @@ void exit_thread(void)
 void show_regs(struct pt_regs *regs)
 {
 	show_registers(regs);
-	show_trace(NULL, regs, (unsigned long *)kernel_stack_pointer(regs));
+	show_trace(NULL, regs, (unsigned long *)kernel_stack_pointer(regs), 0);
 }
 
 void show_regs_common(void)
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 938c8e10a19a..6515733a289d 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -73,7 +73,7 @@ static const struct stacktrace_ops save_stack_ops_nosched = {
  */
 void save_stack_trace(struct stack_trace *trace)
 {
-	dump_trace(current, NULL, NULL, &save_stack_ops, trace);
+	dump_trace(current, NULL, NULL, 0, &save_stack_ops, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
@@ -81,14 +81,14 @@ EXPORT_SYMBOL_GPL(save_stack_trace);
 
 void save_stack_trace_regs(struct stack_trace *trace, struct pt_regs *regs)
 {
-	dump_trace(current, regs, NULL, &save_stack_ops, trace);
+	dump_trace(current, regs, NULL, 0, &save_stack_ops, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
 
 void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 {
-	dump_trace(tsk, NULL, NULL, &save_stack_ops_nosched, trace);
+	dump_trace(tsk, NULL, NULL, 0, &save_stack_ops_nosched, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 -tip 2/4] x86, dumpstack: Random printk changes
  2011-03-08 11:44 [PATCH v4 -tip 1/4] x86, dumpstack: Correct stack dump info when frame pointer is available Namhyung Kim
@ 2011-03-08 11:44 ` Namhyung Kim
  2011-03-10 22:25   ` [tip:x86/cleanups] x86, dumpstack: Cleanup printks tip-bot for Namhyung Kim
  2011-03-08 11:44 ` [PATCH v4 -tip 3/4] x86, dumpstack: Rename print_context_stack and friends Namhyung Kim
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2011-03-08 11:44 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Frederic Weisbecker
  Cc: x86, linux-kernel

Use KERN_CONT level if messages are continued from previous one.
Also remove a pair of angle brackets on EOE for consistency.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 arch/x86/kernel/dumpstack_32.c |    8 ++++----
 arch/x86/kernel/dumpstack_64.c |   10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 3b97a80ce329..c99f9ed013d5 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -116,16 +116,16 @@ void show_registers(struct pt_regs *regs)
 		for (i = 0; i < code_len; i++, ip++) {
 			if (ip < (u8 *)PAGE_OFFSET ||
 					probe_kernel_address(ip, c)) {
-				printk(" Bad EIP value.");
+				printk(KERN_CONT " Bad EIP value.");
 				break;
 			}
 			if (ip == (u8 *)regs->ip)
-				printk("<%02x> ", c);
+				printk(KERN_CONT "<%02x> ", c);
 			else
-				printk("%02x ", c);
+				printk(KERN_CONT "%02x ", c);
 		}
 	}
-	printk("\n");
+	printk(KERN_CONT "\n");
 }
 
 int is_valid_bugaddr(unsigned long ip)
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index e71c98d3c0d2..50aa303f6611 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -180,7 +180,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 
 			bp = ops->walk_stack(tinfo, stack, bp, ops,
 					     data, estack_end, &graph);
-			ops->stack(data, "<EOE>");
+			ops->stack(data, "EOE");
 			/*
 			 * We link to the next stack via the
 			 * second-to-last pointer (index -2 to end) in the
@@ -311,16 +311,16 @@ void show_registers(struct pt_regs *regs)
 		for (i = 0; i < code_len; i++, ip++) {
 			if (ip < (u8 *)PAGE_OFFSET ||
 					probe_kernel_address(ip, c)) {
-				printk(" Bad RIP value.");
+				printk(KERN_CONT " Bad RIP value.");
 				break;
 			}
 			if (ip == (u8 *)regs->ip)
-				printk("<%02x> ", c);
+				printk(KERN_CONT "<%02x> ", c);
 			else
-				printk("%02x ", c);
+				printk(KERN_CONT "%02x ", c);
 		}
 	}
-	printk("\n");
+	printk(KERN_CONT "\n");
 }
 
 int is_valid_bugaddr(unsigned long ip)
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 -tip 3/4] x86, dumpstack: Rename print_context_stack and friends
  2011-03-08 11:44 [PATCH v4 -tip 1/4] x86, dumpstack: Correct stack dump info when frame pointer is available Namhyung Kim
  2011-03-08 11:44 ` [PATCH v4 -tip 2/4] x86, dumpstack: Random printk changes Namhyung Kim
@ 2011-03-08 11:44 ` Namhyung Kim
  2011-03-10 22:25   ` [tip:x86/cleanups] " tip-bot for Namhyung Kim
  2011-03-08 11:44 ` [PATCH v4 -tip 4/4] x86, dumpstack: Use frame pointer during stack trace Namhyung Kim
  2011-03-10 22:24 ` [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available tip-bot for Namhyung Kim
  3 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2011-03-08 11:44 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Frederic Weisbecker
  Cc: x86, linux-kernel, Peter Zijlstra, Paul Mackerras,
	Arnaldo Carvalho de Melo, Robert Richter

print_context_stack* and print_ftrace_graph_addr are misnomer.
They don't print anything by themselves and calls appropriate
callback routines. Actually save_stack_ops* use them just for
saving return addresses not for printing.

Rename them to walk_context_stack* will make more sense IMHO.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Robert Richter <robert.richter@amd.com>
---
 arch/x86/include/asm/stacktrace.h |    4 ++--
 arch/x86/kernel/cpu/perf_event.c  |    2 +-
 arch/x86/kernel/dumpstack.c       |   18 +++++++++---------
 arch/x86/kernel/stacktrace.c      |    4 ++--
 arch/x86/oprofile/backtrace.c     |    2 +-
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index d7e89c83645d..73fc8e2c4872 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -23,13 +23,13 @@ typedef unsigned long (*walk_stack_t)(struct thread_info *tinfo,
 				      int *graph);
 
 extern unsigned long
-print_context_stack(struct thread_info *tinfo,
+walk_context_stack(struct thread_info *tinfo,
 		    unsigned long *stack, unsigned long bp,
 		    const struct stacktrace_ops *ops, void *data,
 		    unsigned long *end, int *graph);
 
 extern unsigned long
-print_context_stack_bp(struct thread_info *tinfo,
+walk_context_stack_bp(struct thread_info *tinfo,
 		       unsigned long *stack, unsigned long bp,
 		       const struct stacktrace_ops *ops, void *data,
 		       unsigned long *end, int *graph);
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index d19cdc2d602e..9237e83ccaa1 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1779,7 +1779,7 @@ static const struct stacktrace_ops backtrace_ops = {
 	.warning_symbol		= backtrace_warning_symbol,
 	.stack			= backtrace_stack,
 	.address		= backtrace_address,
-	.walk_stack		= print_context_stack_bp,
+	.walk_stack		= walk_context_stack_bp,
 };
 
 void
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index c330160b6da3..38b74a5e59aba 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -33,7 +33,7 @@ void printk_address(unsigned long address, int reliable)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 static void
-print_ftrace_graph_addr(unsigned long addr, void *data,
+walk_ftrace_graph_addr(unsigned long addr, void *data,
 			const struct stacktrace_ops *ops,
 			struct thread_info *tinfo, int *graph)
 {
@@ -56,7 +56,7 @@ print_ftrace_graph_addr(unsigned long addr, void *data,
 }
 #else
 static inline void
-print_ftrace_graph_addr(unsigned long addr, void *data,
+walk_ftrace_graph_addr(unsigned long addr, void *data,
 			const struct stacktrace_ops *ops,
 			struct thread_info *tinfo, int *graph)
 { }
@@ -83,7 +83,7 @@ static inline int valid_stack_ptr(struct thread_info *tinfo,
 }
 
 unsigned long
-print_context_stack(struct thread_info *tinfo,
+walk_context_stack(struct thread_info *tinfo,
 		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data,
 		unsigned long *end, int *graph)
@@ -102,16 +102,16 @@ print_context_stack(struct thread_info *tinfo,
 			} else {
 				ops->address(data, addr, 0);
 			}
-			print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
+			walk_ftrace_graph_addr(addr, data, ops, tinfo, graph);
 		}
 		stack++;
 	}
 	return bp;
 }
-EXPORT_SYMBOL_GPL(print_context_stack);
+EXPORT_SYMBOL_GPL(walk_context_stack);
 
 unsigned long
-print_context_stack_bp(struct thread_info *tinfo,
+walk_context_stack_bp(struct thread_info *tinfo,
 		       unsigned long *stack, unsigned long bp,
 		       const struct stacktrace_ops *ops, void *data,
 		       unsigned long *end, int *graph)
@@ -128,12 +128,12 @@ print_context_stack_bp(struct thread_info *tinfo,
 		ops->address(data, addr, 1);
 		frame = frame->next_frame;
 		ret_addr = &frame->return_address;
-		print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
+		walk_ftrace_graph_addr(addr, data, ops, tinfo, graph);
 	}
 
 	return (unsigned long)frame;
 }
-EXPORT_SYMBOL_GPL(print_context_stack_bp);
+EXPORT_SYMBOL_GPL(walk_context_stack_bp);
 
 
 static void
@@ -170,7 +170,7 @@ static const struct stacktrace_ops print_trace_ops = {
 	.warning_symbol		= print_trace_warning_symbol,
 	.stack			= print_trace_stack,
 	.address		= print_trace_address,
-	.walk_stack		= print_context_stack,
+	.walk_stack		= walk_context_stack,
 };
 
 void
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 6515733a289d..2e44ee13637f 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -57,7 +57,7 @@ static const struct stacktrace_ops save_stack_ops = {
 	.warning_symbol	= save_stack_warning_symbol,
 	.stack		= save_stack_stack,
 	.address	= save_stack_address,
-	.walk_stack	= print_context_stack,
+	.walk_stack	= walk_context_stack,
 };
 
 static const struct stacktrace_ops save_stack_ops_nosched = {
@@ -65,7 +65,7 @@ static const struct stacktrace_ops save_stack_ops_nosched = {
 	.warning_symbol	= save_stack_warning_symbol,
 	.stack		= save_stack_stack,
 	.address	= save_stack_address_nosched,
-	.walk_stack	= print_context_stack,
+	.walk_stack	= walk_context_stack,
 };
 
 /*
diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index 72cbec14d783..f4b9fbb8de2b 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -46,7 +46,7 @@ static struct stacktrace_ops backtrace_ops = {
 	.warning_symbol	= backtrace_warning_symbol,
 	.stack		= backtrace_stack,
 	.address	= backtrace_address,
-	.walk_stack	= print_context_stack,
+	.walk_stack	= walk_context_stack,
 };
 
 #ifdef CONFIG_COMPAT
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 -tip 4/4] x86, dumpstack: Use frame pointer during stack trace
  2011-03-08 11:44 [PATCH v4 -tip 1/4] x86, dumpstack: Correct stack dump info when frame pointer is available Namhyung Kim
  2011-03-08 11:44 ` [PATCH v4 -tip 2/4] x86, dumpstack: Random printk changes Namhyung Kim
  2011-03-08 11:44 ` [PATCH v4 -tip 3/4] x86, dumpstack: Rename print_context_stack and friends Namhyung Kim
@ 2011-03-08 11:44 ` Namhyung Kim
  2011-03-10 22:26   ` [tip:x86/cleanups] " tip-bot for Namhyung Kim
  2011-03-10 22:24 ` [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available tip-bot for Namhyung Kim
  3 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2011-03-08 11:44 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Frederic Weisbecker
  Cc: x86, linux-kernel

We now have CONFIG_FRAME_POINTER=y by default, it would be better
use the frame pointer for the stack backtrace rather than scanning
whole stack blindly.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 arch/x86/kernel/dumpstack.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 38b74a5e59aba..56db27d36858 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -170,7 +170,11 @@ static const struct stacktrace_ops print_trace_ops = {
 	.warning_symbol		= print_trace_warning_symbol,
 	.stack			= print_trace_stack,
 	.address		= print_trace_address,
+#ifdef CONFIG_FRAME_POINTER
+	.walk_stack		= walk_context_stack_bp,
+#else
 	.walk_stack		= walk_context_stack,
+#endif
 };
 
 void
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available
  2011-03-08 11:44 [PATCH v4 -tip 1/4] x86, dumpstack: Correct stack dump info when frame pointer is available Namhyung Kim
                   ` (2 preceding siblings ...)
  2011-03-08 11:44 ` [PATCH v4 -tip 4/4] x86, dumpstack: Use frame pointer during stack trace Namhyung Kim
@ 2011-03-10 22:24 ` tip-bot for Namhyung Kim
  2011-03-10 22:46   ` Frederic Weisbecker
  2011-03-11 19:54   ` Thomas Gleixner
  3 siblings, 2 replies; 14+ messages in thread
From: tip-bot for Namhyung Kim @ 2011-03-10 22:24 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, acme, paulus, hpa, mingo, a.p.zijlstra, namhyung,
	ssp, fweisbec, tglx

Commit-ID:  074206a787e4dfdc4c290789ab6604c7f9e691ca
Gitweb:     http://git.kernel.org/tip/074206a787e4dfdc4c290789ab6604c7f9e691ca
Author:     Namhyung Kim <namhyung@gmail.com>
AuthorDate: Tue, 8 Mar 2011 20:44:19 +0900
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Mar 2011 23:20:30 +0100

x86, dumpstack: Correct stack dump info when frame pointer is available

Current stack dump code scans entire stack and checks each entry for a
pointer to kernel code. If CONFIG_FRAME_POINTER=y it could mark
whether the pointer is valid or not based on value of the frame
pointer. Invalid entries could be preceded by '?' sign.

However this was not going to happen because scan start point was
always higher than the frame pointer so that they could not meet.

Commit 9c0729dc8062 ("x86: Eliminate bp argument from the stack
tracing routines") delayed bp acquisition point, so the bp was
read in lower frame, thus all of the entries were marked invalid.

This patch fixes this by reverting above commit while retaining
stack_frame() helper as suggested by Frederic Weisbecker.
End result looks like below:

before:
[    3.508329] Call Trace:
[    3.508551]  [<ffffffff814f35c9>] ? panic+0x91/0x199
[    3.508662]  [<ffffffff814f3739>] ? printk+0x68/0x6a
[    3.508770]  [<ffffffff81a981b2>] ? mount_block_root+0x257/0x26e
[    3.508876]  [<ffffffff81a9821f>] ? mount_root+0x56/0x5a
[    3.508975]  [<ffffffff81a98393>] ? prepare_namespace+0x170/0x1a9
[    3.509216]  [<ffffffff81a9772b>] ? kernel_init+0x1d2/0x1e2
[    3.509335]  [<ffffffff81003894>] ? kernel_thread_helper+0x4/0x10
[    3.509442]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
[    3.509542]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
[    3.509641]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10

after:
[    3.522991] Call Trace:
[    3.523351]  [<ffffffff814f35b9>] panic+0x91/0x199
[    3.523468]  [<ffffffff814f3729>] ? printk+0x68/0x6a
[    3.523576]  [<ffffffff81a981b2>] mount_block_root+0x257/0x26e
[    3.523681]  [<ffffffff81a9821f>] mount_root+0x56/0x5a
[    3.523780]  [<ffffffff81a98393>] prepare_namespace+0x170/0x1a9
[    3.523885]  [<ffffffff81a9772b>] kernel_init+0x1d2/0x1e2
[    3.523987]  [<ffffffff81003894>] kernel_thread_helper+0x4/0x10
[    3.524228]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
[    3.524345]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
[    3.524445]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Soren Sandmann <ssp@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1299584662-24421-1-git-send-email-namhyung@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/kdebug.h     |    2 +-
 arch/x86/include/asm/stacktrace.h |    6 +++---
 arch/x86/kernel/cpu/perf_event.c  |    2 +-
 arch/x86/kernel/dumpstack.c       |   14 ++++++++------
 arch/x86/kernel/dumpstack_32.c    |   15 ++++++++-------
 arch/x86/kernel/dumpstack_64.c    |   14 +++++++-------
 arch/x86/kernel/process.c         |    2 +-
 arch/x86/kernel/stacktrace.c      |    6 +++---
 8 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index ca242d3..e28ec43 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -27,7 +27,7 @@ extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
 extern void show_registers(struct pt_regs *regs);
 extern void show_trace(struct task_struct *t, struct pt_regs *regs,
-		       unsigned long *sp);
+		       unsigned long *sp, unsigned long bp);
 extern void __show_regs(struct pt_regs *regs, int all);
 extern void show_regs(struct pt_regs *regs);
 extern unsigned long oops_begin(void);
diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 52b5c7e..d7e89c8 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -47,7 +47,7 @@ struct stacktrace_ops {
 };
 
 void dump_trace(struct task_struct *tsk, struct pt_regs *regs,
-		unsigned long *stack,
+		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data);
 
 #ifdef CONFIG_X86_32
@@ -86,11 +86,11 @@ stack_frame(struct task_struct *task, struct pt_regs *regs)
 
 extern void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *stack, char *log_lvl);
+		   unsigned long *stack, unsigned long bp, char *log_lvl);
 
 extern void
 show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *sp, char *log_lvl);
+		   unsigned long *sp, unsigned long bp, char *log_lvl);
 
 extern unsigned int code_bytes;
 
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 9d977a2..d19cdc2 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1710,7 +1710,7 @@ perf_callchain_kernel(struct perf_callchain_entry *entry, struct pt_regs *regs)
 
 	perf_callchain_store(entry, regs->ip);
 
-	dump_trace(NULL, regs, NULL, &backtrace_ops, entry);
+	dump_trace(NULL, regs, NULL, 0, &backtrace_ops, entry);
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index df20723..c330160 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -175,21 +175,21 @@ static const struct stacktrace_ops print_trace_ops = {
 
 void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		unsigned long *stack, char *log_lvl)
+		unsigned long *stack, unsigned long bp, char *log_lvl)
 {
 	printk("%sCall Trace:\n", log_lvl);
-	dump_trace(task, regs, stack, &print_trace_ops, log_lvl);
+	dump_trace(task, regs, stack, bp, &print_trace_ops, log_lvl);
 }
 
 void show_trace(struct task_struct *task, struct pt_regs *regs,
-		unsigned long *stack)
+		unsigned long *stack, unsigned long bp)
 {
-	show_trace_log_lvl(task, regs, stack, "");
+	show_trace_log_lvl(task, regs, stack, bp, "");
 }
 
 void show_stack(struct task_struct *task, unsigned long *sp)
 {
-	show_stack_log_lvl(task, NULL, sp, "");
+	show_stack_log_lvl(task, NULL, sp, 0, "");
 }
 
 /*
@@ -197,14 +197,16 @@ void show_stack(struct task_struct *task, unsigned long *sp)
  */
 void dump_stack(void)
 {
+	unsigned long bp;
 	unsigned long stack;
 
+	bp = stack_frame(current, NULL);
 	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
 		current->pid, current->comm, print_tainted(),
 		init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
 		init_utsname()->version);
-	show_trace(NULL, NULL, &stack);
+	show_trace(NULL, NULL, &stack, bp);
 }
 EXPORT_SYMBOL(dump_stack);
 
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 74cc1ed..3b97a80 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -17,12 +17,11 @@
 #include <asm/stacktrace.h>
 
 
-void dump_trace(struct task_struct *task,
-		struct pt_regs *regs, unsigned long *stack,
+void dump_trace(struct task_struct *task, struct pt_regs *regs,
+		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)
 {
 	int graph = 0;
-	unsigned long bp;
 
 	if (!task)
 		task = current;
@@ -35,7 +34,9 @@ void dump_trace(struct task_struct *task,
 			stack = (unsigned long *)task->thread.sp;
 	}
 
-	bp = stack_frame(task, regs);
+	if (!bp)
+		bp = stack_frame(task, regs);
+
 	for (;;) {
 		struct thread_info *context;
 
@@ -55,7 +56,7 @@ EXPORT_SYMBOL(dump_trace);
 
 void
 show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *sp, char *log_lvl)
+		   unsigned long *sp, unsigned long bp, char *log_lvl)
 {
 	unsigned long *stack;
 	int i;
@@ -77,7 +78,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 		touch_nmi_watchdog();
 	}
 	printk(KERN_CONT "\n");
-	show_trace_log_lvl(task, regs, sp, log_lvl);
+	show_trace_log_lvl(task, regs, sp, bp, log_lvl);
 }
 
 
@@ -102,7 +103,7 @@ void show_registers(struct pt_regs *regs)
 		u8 *ip;
 
 		printk(KERN_EMERG "Stack:\n");
-		show_stack_log_lvl(NULL, regs, &regs->sp, KERN_EMERG);
+		show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);
 
 		printk(KERN_EMERG "Code: ");
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index a6b6fcf..e71c98d 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -139,8 +139,8 @@ fixup_bp_irq_link(unsigned long bp, unsigned long *stack,
  * severe exception (double fault, nmi, stack fault, debug, mce) hardware stack
  */
 
-void dump_trace(struct task_struct *task,
-		struct pt_regs *regs, unsigned long *stack,
+void dump_trace(struct task_struct *task, struct pt_regs *regs,
+		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)
 {
 	const unsigned cpu = get_cpu();
@@ -150,7 +150,6 @@ void dump_trace(struct task_struct *task,
 	struct thread_info *tinfo;
 	int graph = 0;
 	unsigned long dummy;
-	unsigned long bp;
 
 	if (!task)
 		task = current;
@@ -161,7 +160,8 @@ void dump_trace(struct task_struct *task,
 			stack = (unsigned long *)task->thread.sp;
 	}
 
-	bp = stack_frame(task, regs);
+	if (!bp)
+		bp = stack_frame(task, regs);
 	/*
 	 * Print function call entries in all stacks, starting at the
 	 * current stack address. If the stacks consist of nested
@@ -225,7 +225,7 @@ EXPORT_SYMBOL(dump_trace);
 
 void
 show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
-		   unsigned long *sp, char *log_lvl)
+		   unsigned long *sp, unsigned long bp, char *log_lvl)
 {
 	unsigned long *irq_stack_end;
 	unsigned long *irq_stack;
@@ -269,7 +269,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	preempt_enable();
 
 	printk(KERN_CONT "\n");
-	show_trace_log_lvl(task, regs, sp, log_lvl);
+	show_trace_log_lvl(task, regs, sp, bp, log_lvl);
 }
 
 void show_registers(struct pt_regs *regs)
@@ -298,7 +298,7 @@ void show_registers(struct pt_regs *regs)
 
 		printk(KERN_EMERG "Stack:\n");
 		show_stack_log_lvl(NULL, regs, (unsigned long *)sp,
-				   KERN_EMERG);
+				   0, KERN_EMERG);
 
 		printk(KERN_EMERG "Code: ");
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index ff45541..f4c6cc4 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -87,7 +87,7 @@ void exit_thread(void)
 void show_regs(struct pt_regs *regs)
 {
 	show_registers(regs);
-	show_trace(NULL, regs, (unsigned long *)kernel_stack_pointer(regs));
+	show_trace(NULL, regs, (unsigned long *)kernel_stack_pointer(regs), 0);
 }
 
 void show_regs_common(void)
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 938c8e1..6515733 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -73,7 +73,7 @@ static const struct stacktrace_ops save_stack_ops_nosched = {
  */
 void save_stack_trace(struct stack_trace *trace)
 {
-	dump_trace(current, NULL, NULL, &save_stack_ops, trace);
+	dump_trace(current, NULL, NULL, 0, &save_stack_ops, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
@@ -81,14 +81,14 @@ EXPORT_SYMBOL_GPL(save_stack_trace);
 
 void save_stack_trace_regs(struct stack_trace *trace, struct pt_regs *regs)
 {
-	dump_trace(current, regs, NULL, &save_stack_ops, trace);
+	dump_trace(current, regs, NULL, 0, &save_stack_ops, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
 
 void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 {
-	dump_trace(tsk, NULL, NULL, &save_stack_ops_nosched, trace);
+	dump_trace(tsk, NULL, NULL, 0, &save_stack_ops_nosched, trace);
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [tip:x86/cleanups] x86, dumpstack: Cleanup printks
  2011-03-08 11:44 ` [PATCH v4 -tip 2/4] x86, dumpstack: Random printk changes Namhyung Kim
@ 2011-03-10 22:25   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Namhyung Kim @ 2011-03-10 22:25 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, fweisbec, tglx, namhyung

Commit-ID:  2b8689e03cd48bb832b933ce5b6335f30e02c6f9
Gitweb:     http://git.kernel.org/tip/2b8689e03cd48bb832b933ce5b6335f30e02c6f9
Author:     Namhyung Kim <namhyung@gmail.com>
AuthorDate: Tue, 8 Mar 2011 20:44:20 +0900
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Mar 2011 23:20:30 +0100

x86, dumpstack: Cleanup printks

Use KERN_CONT level if messages are continued from previous one.
Also remove a pair of angle brackets on EOE for consistency.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1299584662-24421-2-git-send-email-namhyung@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/dumpstack_32.c |    8 ++++----
 arch/x86/kernel/dumpstack_64.c |   10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 3b97a80..c99f9ed 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -116,16 +116,16 @@ void show_registers(struct pt_regs *regs)
 		for (i = 0; i < code_len; i++, ip++) {
 			if (ip < (u8 *)PAGE_OFFSET ||
 					probe_kernel_address(ip, c)) {
-				printk(" Bad EIP value.");
+				printk(KERN_CONT " Bad EIP value.");
 				break;
 			}
 			if (ip == (u8 *)regs->ip)
-				printk("<%02x> ", c);
+				printk(KERN_CONT "<%02x> ", c);
 			else
-				printk("%02x ", c);
+				printk(KERN_CONT "%02x ", c);
 		}
 	}
-	printk("\n");
+	printk(KERN_CONT "\n");
 }
 
 int is_valid_bugaddr(unsigned long ip)
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index e71c98d..50aa303 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -180,7 +180,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 
 			bp = ops->walk_stack(tinfo, stack, bp, ops,
 					     data, estack_end, &graph);
-			ops->stack(data, "<EOE>");
+			ops->stack(data, "EOE");
 			/*
 			 * We link to the next stack via the
 			 * second-to-last pointer (index -2 to end) in the
@@ -311,16 +311,16 @@ void show_registers(struct pt_regs *regs)
 		for (i = 0; i < code_len; i++, ip++) {
 			if (ip < (u8 *)PAGE_OFFSET ||
 					probe_kernel_address(ip, c)) {
-				printk(" Bad RIP value.");
+				printk(KERN_CONT " Bad RIP value.");
 				break;
 			}
 			if (ip == (u8 *)regs->ip)
-				printk("<%02x> ", c);
+				printk(KERN_CONT "<%02x> ", c);
 			else
-				printk("%02x ", c);
+				printk(KERN_CONT "%02x ", c);
 		}
 	}
-	printk("\n");
+	printk(KERN_CONT "\n");
 }
 
 int is_valid_bugaddr(unsigned long ip)

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [tip:x86/cleanups] x86, dumpstack: Rename print_context_stack and friends
  2011-03-08 11:44 ` [PATCH v4 -tip 3/4] x86, dumpstack: Rename print_context_stack and friends Namhyung Kim
@ 2011-03-10 22:25   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Namhyung Kim @ 2011-03-10 22:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, paulus, hpa, mingo, a.p.zijlstra, acme, namhyung,
	robert.richter, fweisbec, tglx

Commit-ID:  27f30f3e462f84e0a7c561d80e08d428e566db5e
Gitweb:     http://git.kernel.org/tip/27f30f3e462f84e0a7c561d80e08d428e566db5e
Author:     Namhyung Kim <namhyung@gmail.com>
AuthorDate: Tue, 8 Mar 2011 20:44:21 +0900
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Mar 2011 23:20:30 +0100

x86, dumpstack: Rename print_context_stack and friends

print_context_stack* and print_ftrace_graph_addr are misnomers. They
don't print anything by themselves and call appropriate callback
routines. Actually save_stack_ops* use them just for saving return
addresses not for printing.

Rename them to walk_context_stack* will make more sense IMHO.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1299584662-24421-3-git-send-email-namhyung@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/stacktrace.h |    4 ++--
 arch/x86/kernel/cpu/perf_event.c  |    2 +-
 arch/x86/kernel/dumpstack.c       |   18 +++++++++---------
 arch/x86/kernel/stacktrace.c      |    4 ++--
 arch/x86/oprofile/backtrace.c     |    2 +-
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index d7e89c8..73fc8e2 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -23,13 +23,13 @@ typedef unsigned long (*walk_stack_t)(struct thread_info *tinfo,
 				      int *graph);
 
 extern unsigned long
-print_context_stack(struct thread_info *tinfo,
+walk_context_stack(struct thread_info *tinfo,
 		    unsigned long *stack, unsigned long bp,
 		    const struct stacktrace_ops *ops, void *data,
 		    unsigned long *end, int *graph);
 
 extern unsigned long
-print_context_stack_bp(struct thread_info *tinfo,
+walk_context_stack_bp(struct thread_info *tinfo,
 		       unsigned long *stack, unsigned long bp,
 		       const struct stacktrace_ops *ops, void *data,
 		       unsigned long *end, int *graph);
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index d19cdc2..9237e83 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1697,7 +1697,7 @@ static const struct stacktrace_ops backtrace_ops = {
 	.warning_symbol		= backtrace_warning_symbol,
 	.stack			= backtrace_stack,
 	.address		= backtrace_address,
-	.walk_stack		= print_context_stack_bp,
+	.walk_stack		= walk_context_stack_bp,
 };
 
 void
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index c330160..38b74a5e 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -33,7 +33,7 @@ void printk_address(unsigned long address, int reliable)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 static void
-print_ftrace_graph_addr(unsigned long addr, void *data,
+walk_ftrace_graph_addr(unsigned long addr, void *data,
 			const struct stacktrace_ops *ops,
 			struct thread_info *tinfo, int *graph)
 {
@@ -56,7 +56,7 @@ print_ftrace_graph_addr(unsigned long addr, void *data,
 }
 #else
 static inline void
-print_ftrace_graph_addr(unsigned long addr, void *data,
+walk_ftrace_graph_addr(unsigned long addr, void *data,
 			const struct stacktrace_ops *ops,
 			struct thread_info *tinfo, int *graph)
 { }
@@ -83,7 +83,7 @@ static inline int valid_stack_ptr(struct thread_info *tinfo,
 }
 
 unsigned long
-print_context_stack(struct thread_info *tinfo,
+walk_context_stack(struct thread_info *tinfo,
 		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data,
 		unsigned long *end, int *graph)
@@ -102,16 +102,16 @@ print_context_stack(struct thread_info *tinfo,
 			} else {
 				ops->address(data, addr, 0);
 			}
-			print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
+			walk_ftrace_graph_addr(addr, data, ops, tinfo, graph);
 		}
 		stack++;
 	}
 	return bp;
 }
-EXPORT_SYMBOL_GPL(print_context_stack);
+EXPORT_SYMBOL_GPL(walk_context_stack);
 
 unsigned long
-print_context_stack_bp(struct thread_info *tinfo,
+walk_context_stack_bp(struct thread_info *tinfo,
 		       unsigned long *stack, unsigned long bp,
 		       const struct stacktrace_ops *ops, void *data,
 		       unsigned long *end, int *graph)
@@ -128,12 +128,12 @@ print_context_stack_bp(struct thread_info *tinfo,
 		ops->address(data, addr, 1);
 		frame = frame->next_frame;
 		ret_addr = &frame->return_address;
-		print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
+		walk_ftrace_graph_addr(addr, data, ops, tinfo, graph);
 	}
 
 	return (unsigned long)frame;
 }
-EXPORT_SYMBOL_GPL(print_context_stack_bp);
+EXPORT_SYMBOL_GPL(walk_context_stack_bp);
 
 
 static void
@@ -170,7 +170,7 @@ static const struct stacktrace_ops print_trace_ops = {
 	.warning_symbol		= print_trace_warning_symbol,
 	.stack			= print_trace_stack,
 	.address		= print_trace_address,
-	.walk_stack		= print_context_stack,
+	.walk_stack		= walk_context_stack,
 };
 
 void
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 6515733..2e44ee1 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -57,7 +57,7 @@ static const struct stacktrace_ops save_stack_ops = {
 	.warning_symbol	= save_stack_warning_symbol,
 	.stack		= save_stack_stack,
 	.address	= save_stack_address,
-	.walk_stack	= print_context_stack,
+	.walk_stack	= walk_context_stack,
 };
 
 static const struct stacktrace_ops save_stack_ops_nosched = {
@@ -65,7 +65,7 @@ static const struct stacktrace_ops save_stack_ops_nosched = {
 	.warning_symbol	= save_stack_warning_symbol,
 	.stack		= save_stack_stack,
 	.address	= save_stack_address_nosched,
-	.walk_stack	= print_context_stack,
+	.walk_stack	= walk_context_stack,
 };
 
 /*
diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index 72cbec1..f4b9fbb 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -46,7 +46,7 @@ static struct stacktrace_ops backtrace_ops = {
 	.warning_symbol	= backtrace_warning_symbol,
 	.stack		= backtrace_stack,
 	.address	= backtrace_address,
-	.walk_stack	= print_context_stack,
+	.walk_stack	= walk_context_stack,
 };
 
 #ifdef CONFIG_COMPAT

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [tip:x86/cleanups] x86, dumpstack: Use frame pointer during stack trace
  2011-03-08 11:44 ` [PATCH v4 -tip 4/4] x86, dumpstack: Use frame pointer during stack trace Namhyung Kim
@ 2011-03-10 22:26   ` tip-bot for Namhyung Kim
  2011-03-10 23:02     ` Frederic Weisbecker
  0 siblings, 1 reply; 14+ messages in thread
From: tip-bot for Namhyung Kim @ 2011-03-10 22:26 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, fweisbec, tglx, namhyung

Commit-ID:  2f8058ae197236f9d5641850ce27f67d8f3e0b39
Gitweb:     http://git.kernel.org/tip/2f8058ae197236f9d5641850ce27f67d8f3e0b39
Author:     Namhyung Kim <namhyung@gmail.com>
AuthorDate: Tue, 8 Mar 2011 20:44:22 +0900
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Mar 2011 23:20:30 +0100

x86, dumpstack: Use frame pointer during stack trace

If CONFIG_FRAME_POINTER is set then use the frame pointer for the
stack backtrace rather than scanning whole stack blindly.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1299584662-24421-4-git-send-email-namhyung@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/dumpstack.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 38b74a5e..56db27d 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -170,7 +170,11 @@ static const struct stacktrace_ops print_trace_ops = {
 	.warning_symbol		= print_trace_warning_symbol,
 	.stack			= print_trace_stack,
 	.address		= print_trace_address,
+#ifdef CONFIG_FRAME_POINTER
+	.walk_stack		= walk_context_stack_bp,
+#else
 	.walk_stack		= walk_context_stack,
+#endif
 };
 
 void

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available
  2011-03-10 22:24 ` [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available tip-bot for Namhyung Kim
@ 2011-03-10 22:46   ` Frederic Weisbecker
  2011-03-11 19:54   ` Thomas Gleixner
  1 sibling, 0 replies; 14+ messages in thread
From: Frederic Weisbecker @ 2011-03-10 22:46 UTC (permalink / raw)
  To: mingo, hpa, paulus, acme, linux-kernel, a.p.zijlstra, tglx,
	namhyung, ssp
  Cc: linux-tip-commits

On Thu, Mar 10, 2011 at 10:24:53PM +0000, tip-bot for Namhyung Kim wrote:
> Commit-ID:  074206a787e4dfdc4c290789ab6604c7f9e691ca
> Gitweb:     http://git.kernel.org/tip/074206a787e4dfdc4c290789ab6604c7f9e691ca
> Author:     Namhyung Kim <namhyung@gmail.com>
> AuthorDate: Tue, 8 Mar 2011 20:44:19 +0900
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
> 
> x86, dumpstack: Correct stack dump info when frame pointer is available
> 
> Current stack dump code scans entire stack and checks each entry for a
> pointer to kernel code. If CONFIG_FRAME_POINTER=y it could mark
> whether the pointer is valid or not based on value of the frame
> pointer. Invalid entries could be preceded by '?' sign.
> 
> However this was not going to happen because scan start point was
> always higher than the frame pointer so that they could not meet.
> 
> Commit 9c0729dc8062 ("x86: Eliminate bp argument from the stack
> tracing routines") delayed bp acquisition point, so the bp was
> read in lower frame, thus all of the entries were marked invalid.
> 
> This patch fixes this by reverting above commit while retaining
> stack_frame() helper as suggested by Frederic Weisbecker.
> End result looks like below:
> 
> before:
> [    3.508329] Call Trace:
> [    3.508551]  [<ffffffff814f35c9>] ? panic+0x91/0x199
> [    3.508662]  [<ffffffff814f3739>] ? printk+0x68/0x6a
> [    3.508770]  [<ffffffff81a981b2>] ? mount_block_root+0x257/0x26e
> [    3.508876]  [<ffffffff81a9821f>] ? mount_root+0x56/0x5a
> [    3.508975]  [<ffffffff81a98393>] ? prepare_namespace+0x170/0x1a9
> [    3.509216]  [<ffffffff81a9772b>] ? kernel_init+0x1d2/0x1e2
> [    3.509335]  [<ffffffff81003894>] ? kernel_thread_helper+0x4/0x10
> [    3.509442]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
> [    3.509542]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
> [    3.509641]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10
> 
> after:
> [    3.522991] Call Trace:
> [    3.523351]  [<ffffffff814f35b9>] panic+0x91/0x199
> [    3.523468]  [<ffffffff814f3729>] ? printk+0x68/0x6a
> [    3.523576]  [<ffffffff81a981b2>] mount_block_root+0x257/0x26e
> [    3.523681]  [<ffffffff81a9821f>] mount_root+0x56/0x5a
> [    3.523780]  [<ffffffff81a98393>] prepare_namespace+0x170/0x1a9
> [    3.523885]  [<ffffffff81a9772b>] kernel_init+0x1d2/0x1e2
> [    3.523987]  [<ffffffff81003894>] kernel_thread_helper+0x4/0x10
> [    3.524228]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
> [    3.524345]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
> [    3.524445]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> Cc: Soren Sandmann <ssp@redhat.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> LKML-Reference: <1299584662-24421-1-git-send-email-namhyung@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---

Nice fix, thanks!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [tip:x86/cleanups] x86, dumpstack: Use frame pointer during stack trace
  2011-03-10 22:26   ` [tip:x86/cleanups] " tip-bot for Namhyung Kim
@ 2011-03-10 23:02     ` Frederic Weisbecker
  2011-03-23 13:08       ` Namhyung Kim
  0 siblings, 1 reply; 14+ messages in thread
From: Frederic Weisbecker @ 2011-03-10 23:02 UTC (permalink / raw)
  To: mingo, hpa, linux-kernel, tglx, namhyung; +Cc: linux-tip-commits

On Thu, Mar 10, 2011 at 10:26:07PM +0000, tip-bot for Namhyung Kim wrote:
> Commit-ID:  2f8058ae197236f9d5641850ce27f67d8f3e0b39
> Gitweb:     http://git.kernel.org/tip/2f8058ae197236f9d5641850ce27f67d8f3e0b39
> Author:     Namhyung Kim <namhyung@gmail.com>
> AuthorDate: Tue, 8 Mar 2011 20:44:22 +0900
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
> 
> x86, dumpstack: Use frame pointer during stack trace
> 
> If CONFIG_FRAME_POINTER is set then use the frame pointer for the
> stack backtrace rather than scanning whole stack blindly.

We don't do it blindly, we actually check the reliability with the
frame pointer.

I'm not sure this patch is a good idea. stack dumps need to stay very
robust and not exclusively rely on the frame pointer to be correct.
At least walking blindly the stack provides a best effort dump as a last
resort.


> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> LKML-Reference: <1299584662-24421-4-git-send-email-namhyung@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/dumpstack.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 38b74a5e..56db27d 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -170,7 +170,11 @@ static const struct stacktrace_ops print_trace_ops = {
>  	.warning_symbol		= print_trace_warning_symbol,
>  	.stack			= print_trace_stack,
>  	.address		= print_trace_address,
> +#ifdef CONFIG_FRAME_POINTER
> +	.walk_stack		= walk_context_stack_bp,
> +#else
>  	.walk_stack		= walk_context_stack,
> +#endif
>  };

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available
  2011-03-10 22:24 ` [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available tip-bot for Namhyung Kim
  2011-03-10 22:46   ` Frederic Weisbecker
@ 2011-03-11 19:54   ` Thomas Gleixner
  1 sibling, 0 replies; 14+ messages in thread
From: Thomas Gleixner @ 2011-03-11 19:54 UTC (permalink / raw)
  To: mingo, hpa, paulus, acme, linux-kernel, fweisbec, a.p.zijlstra,
	namhyung, ssp
  Cc: linux-tip-commits

[-- Attachment #1: Type: TEXT/PLAIN, Size: 584 bytes --]

On Thu, 10 Mar 2011, tip-bot for Namhyung Kim wrote:

> Commit-ID:  074206a787e4dfdc4c290789ab6604c7f9e691ca
> Gitweb:     http://git.kernel.org/tip/074206a787e4dfdc4c290789ab6604c7f9e691ca
> Author:     Namhyung Kim <namhyung@gmail.com>
> AuthorDate: Tue, 8 Mar 2011 20:44:19 +0900
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
> 
> x86, dumpstack: Correct stack dump info when frame pointer is available

Dropped it due to:

arch/x86/oprofile/backtrace.c:130:8: error: too few arguments to function ‘dump_trace’

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [tip:x86/cleanups] x86, dumpstack: Use frame pointer during stack trace
  2011-03-10 23:02     ` Frederic Weisbecker
@ 2011-03-23 13:08       ` Namhyung Kim
  2011-03-23 14:08         ` Frederic Weisbecker
  0 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2011-03-23 13:08 UTC (permalink / raw)
  To: Frederic Weisbecker; +Cc: mingo, hpa, linux-kernel, tglx

2011-03-11 (금), 00:02 +0100, Frederic Weisbecker:
> On Thu, Mar 10, 2011 at 10:26:07PM +0000, tip-bot for Namhyung Kim wrote:
> > Commit-ID:  2f8058ae197236f9d5641850ce27f67d8f3e0b39
> > Gitweb:     http://git.kernel.org/tip/2f8058ae197236f9d5641850ce27f67d8f3e0b39
> > Author:     Namhyung Kim <namhyung@gmail.com>
> > AuthorDate: Tue, 8 Mar 2011 20:44:22 +0900
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
> > 
> > x86, dumpstack: Use frame pointer during stack trace
> > 
> > If CONFIG_FRAME_POINTER is set then use the frame pointer for the
> > stack backtrace rather than scanning whole stack blindly.
> 
> We don't do it blindly, we actually check the reliability with the
> frame pointer.
> 
> I'm not sure this patch is a good idea. stack dumps need to stay very
> robust and not exclusively rely on the frame pointer to be correct.
> At least walking blindly the stack provides a best effort dump as a last
> resort.
> 

Sounds reasonable. How about adding a boot param to control it then?


diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 809d027de28f..0d7efd90c588 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -177,6 +177,16 @@ static const struct stacktrace_ops print_trace_ops
#endif
 };
 
+static int __init nofptrace_setup(char *s)
+{
+	struct stacktrace_ops *ops;
+
+	ops = (struct stacktrace_ops *) &print_trace_ops;
+	ops->walk_stack = walk_context_stack;
+	return 0;
+}
+early_param("nofptrace", nofptrace_setup);
+
 void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 		unsigned long *stack, unsigned long bp, char *log_lvl)



-- 
Regards,
Namhyung Kim



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [tip:x86/cleanups] x86, dumpstack: Use frame pointer during stack trace
  2011-03-23 13:08       ` Namhyung Kim
@ 2011-03-23 14:08         ` Frederic Weisbecker
  2011-03-23 14:36           ` Ingo Molnar
  0 siblings, 1 reply; 14+ messages in thread
From: Frederic Weisbecker @ 2011-03-23 14:08 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: mingo, hpa, linux-kernel, tglx

2011/3/23 Namhyung Kim <namhyung@gmail.com>:
> 2011-03-11 (금), 00:02 +0100, Frederic Weisbecker:
>> On Thu, Mar 10, 2011 at 10:26:07PM +0000, tip-bot for Namhyung Kim wrote:
>> > Commit-ID:  2f8058ae197236f9d5641850ce27f67d8f3e0b39
>> > Gitweb:     http://git.kernel.org/tip/2f8058ae197236f9d5641850ce27f67d8f3e0b39
>> > Author:     Namhyung Kim <namhyung@gmail.com>
>> > AuthorDate: Tue, 8 Mar 2011 20:44:22 +0900
>> > Committer:  Thomas Gleixner <tglx@linutronix.de>
>> > CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
>> >
>> > x86, dumpstack: Use frame pointer during stack trace
>> >
>> > If CONFIG_FRAME_POINTER is set then use the frame pointer for the
>> > stack backtrace rather than scanning whole stack blindly.
>>
>> We don't do it blindly, we actually check the reliability with the
>> frame pointer.
>>
>> I'm not sure this patch is a good idea. stack dumps need to stay very
>> robust and not exclusively rely on the frame pointer to be correct.
>> At least walking blindly the stack provides a best effort dump as a last
>> resort.
>>
>
> Sounds reasonable. How about adding a boot param to control it then?

Hmm, but I'm not sure what it would be useful for. Even if one is sure that his
crash will have the needed reliable addresses already, having
unreliable ones too in
the report are not a problem. Are they?

Besides, how can we read a stack dump report posted by someone and be sure
he did not miss an important part because he had this kernel parameter enabled
and his frame pointer broken, or our frame based stack dump walking broken as
it has been many times, like your fixed in your patchset.

One win I could find though is that it can help a stack dump to fit in
the screen....

>
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 809d027de28f..0d7efd90c588 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -177,6 +177,16 @@ static const struct stacktrace_ops print_trace_ops
> #endif
>  };
>
> +static int __init nofptrace_setup(char *s)
> +{
> +       struct stacktrace_ops *ops;
> +
> +       ops = (struct stacktrace_ops *) &print_trace_ops;
> +       ops->walk_stack = walk_context_stack;
> +       return 0;
> +}
> +early_param("nofptrace", nofptrace_setup);
> +
>  void
>  show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
>                unsigned long *stack, unsigned long bp, char *log_lvl)
>
>
>
> --
> Regards,
> Namhyung Kim
>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [tip:x86/cleanups] x86, dumpstack: Use frame pointer during stack trace
  2011-03-23 14:08         ` Frederic Weisbecker
@ 2011-03-23 14:36           ` Ingo Molnar
  0 siblings, 0 replies; 14+ messages in thread
From: Ingo Molnar @ 2011-03-23 14:36 UTC (permalink / raw)
  To: Frederic Weisbecker
  Cc: Namhyung Kim, mingo, hpa, linux-kernel, tglx, Linus Torvalds,
	Andrew Morton


* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> 2011/3/23 Namhyung Kim <namhyung@gmail.com>:
> > 2011-03-11 (금), 00:02 +0100, Frederic Weisbecker:
> >> On Thu, Mar 10, 2011 at 10:26:07PM +0000, tip-bot for Namhyung Kim wrote:
> >> > Commit-ID:  2f8058ae197236f9d5641850ce27f67d8f3e0b39
> >> > Gitweb:     http://git.kernel.org/tip/2f8058ae197236f9d5641850ce27f67d8f3e0b39
> >> > Author:     Namhyung Kim <namhyung@gmail.com>
> >> > AuthorDate: Tue, 8 Mar 2011 20:44:22 +0900
> >> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> >> > CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
> >> >
> >> > x86, dumpstack: Use frame pointer during stack trace
> >> >
> >> > If CONFIG_FRAME_POINTER is set then use the frame pointer for the
> >> > stack backtrace rather than scanning whole stack blindly.
> >>
> >> We don't do it blindly, we actually check the reliability with the
> >> frame pointer.
> >>
> >> I'm not sure this patch is a good idea. stack dumps need to stay very
> >> robust and not exclusively rely on the frame pointer to be correct.
> >> At least walking blindly the stack provides a best effort dump as a last
> >> resort.
> >>
> >
> > Sounds reasonable. How about adding a boot param to control it then?
> 
> Hmm, but I'm not sure what it would be useful for. Even if one is sure that 
> his crash will have the needed reliable addresses already, having unreliable 
> ones too in the report are not a problem. Are they?

Agreed, there's no point in such a boot parameter really.

The principles for printing backtraces are the following:

 - Robustness comes first. We do not ever want to crash or miss important
   information because the frame pointer chain broke in some rarely used
   assembler code.

 - Information. Backtraces like:

     [    3.522991] Call Trace:
     [    3.523351]  [<ffffffff814f35b9>] panic+0x91/0x199
     [    3.523468]  [<ffffffff814f3729>] ? printk+0x68/0x6a
     [    3.523576]  [<ffffffff81a981b2>] mount_block_root+0x257/0x26e
     [    3.523681]  [<ffffffff81a9821f>] mount_root+0x56/0x5a
     [    3.523780]  [<ffffffff81a98393>] prepare_namespace+0x170/0x1a9
     [    3.523885]  [<ffffffff81a9772b>] kernel_init+0x1d2/0x1e2
     [    3.523987]  [<ffffffff81003894>] kernel_thread_helper+0x4/0x10
     [    3.524228]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
     [    3.524345]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
     [    3.524445]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10

   are immensely useful, because firstly we see the 'real' backtrace:

     [    3.523351]  [<ffffffff814f35b9>] panic+0x91/0x199
     [    3.523576]  [<ffffffff81a981b2>] mount_block_root+0x257/0x26e
     [    3.523681]  [<ffffffff81a9821f>] mount_root+0x56/0x5a
     [    3.523780]  [<ffffffff81a98393>] prepare_namespace+0x170/0x1a9
     [    3.523885]  [<ffffffff81a9772b>] kernel_init+0x1d2/0x1e2
     [    3.523987]  [<ffffffff81003894>] kernel_thread_helper+0x4/0x10

   Secondly, we also see the 'residual trace' of what is on the kernel stack:

     [    3.523468]  [<ffffffff814f3729>] ? printk+0x68/0x6a
     [    3.524228]  [<ffffffff814f6880>] ? restore_args+0x0/0x30
     [    3.524345]  [<ffffffff81a97559>] ? kernel_init+0x0/0x1e2
     [    3.524445]  [<ffffffff81003890>] ? kernel_thread_helper+0x0/0x10

   Which is a poor man's kernel trace really. Here it tells us that a printk
   executed shortly before the backtrace was generated. Info like this can
   be useful when rare crashes are analyzed.

So hiding that information is not really productive. If then we could think about
making the visual output more expressive. For example:

 Call Trace:
  [<ffffffff814f35b9>] panic()                   # 0x091/0x199
  [<ffffffff814f3729>]                           # ? printk+0x68/0x6a
  [<ffffffff81a981b2>] mount_block_root()        # 0x257/0x26e
  [<ffffffff81a9821f>] mount_root()              # 0x056/0x05a
  [<ffffffff81a98393>] prepare_namespace()       # 0x170/0x1a9
  [<ffffffff81a9772b>] kernel_init()             # 0x1d2/0x1e2
  [<ffffffff81003894>] kernel_thread_helper()    # 0x004/0x010
  [<ffffffff814f6880>]                           # ? restore_args+0x0/0x30
  [<ffffffff81a97559>]                           # ? kernel_init+0x0/0x1e2
  [<ffffffff81003890>]                           # ? kernel_thread_helper+0x0/0x10

Would be a 'human readable' variant that tells us the real backtrace 'at a 
glance' - perhaps in a bit clearer fashion - while still keeping the 'residual' 
entries included as well.

The downside is that tools that parse backtraces out of the syslog might get 
confused, so that aspect has to be investigated as well.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2011-03-23 14:36 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-08 11:44 [PATCH v4 -tip 1/4] x86, dumpstack: Correct stack dump info when frame pointer is available Namhyung Kim
2011-03-08 11:44 ` [PATCH v4 -tip 2/4] x86, dumpstack: Random printk changes Namhyung Kim
2011-03-10 22:25   ` [tip:x86/cleanups] x86, dumpstack: Cleanup printks tip-bot for Namhyung Kim
2011-03-08 11:44 ` [PATCH v4 -tip 3/4] x86, dumpstack: Rename print_context_stack and friends Namhyung Kim
2011-03-10 22:25   ` [tip:x86/cleanups] " tip-bot for Namhyung Kim
2011-03-08 11:44 ` [PATCH v4 -tip 4/4] x86, dumpstack: Use frame pointer during stack trace Namhyung Kim
2011-03-10 22:26   ` [tip:x86/cleanups] " tip-bot for Namhyung Kim
2011-03-10 23:02     ` Frederic Weisbecker
2011-03-23 13:08       ` Namhyung Kim
2011-03-23 14:08         ` Frederic Weisbecker
2011-03-23 14:36           ` Ingo Molnar
2011-03-10 22:24 ` [tip:x86/cleanups] x86, dumpstack: Correct stack dump info when frame pointer is available tip-bot for Namhyung Kim
2011-03-10 22:46   ` Frederic Weisbecker
2011-03-11 19:54   ` Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).