linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86, stacktrace: avoid recording save_stack_trace wrappers
@ 2017-09-07  7:57 Vlastimil Babka
  2017-09-07 13:07 ` Josh Poimboeuf
  2017-09-29  9:23 ` [PATCH v2] " Vlastimil Babka
  0 siblings, 2 replies; 5+ messages in thread
From: Vlastimil Babka @ 2017-09-07  7:57 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, Josh Poimboeuf, Miroslav Benes, linux-kernel, Vlastimil Babka

The save_stack_trace() and save_stack_trace_tsk() wrappers of
__save_stack_trace() add themselves to the call stack, and thus appear in the
recorded stacktraces. This is redundant and wasteful when we have limited space
to record the useful part of the backtrace with e.g. page_owner functionality.

Fix this by making sure __save_stack_trace() is noinline (which matches the
current gcc decision) and bumping the skip in the wrappers. This is similar
to what was done for arm in 3683f44c42e9 ("ARM: stacktrace: avoid listing
stacktrace functions in stacktrace") and is pending for arm64.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 arch/x86/kernel/stacktrace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 8dabd7bf1673..4b2fd6092739 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -30,7 +30,7 @@ static int save_stack_address(struct stack_trace *trace, unsigned long addr,
 	return 0;
 }
 
-static void __save_stack_trace(struct stack_trace *trace,
+static void noinline __save_stack_trace(struct stack_trace *trace,
 			       struct task_struct *task, struct pt_regs *regs,
 			       bool nosched)
 {
@@ -56,6 +56,7 @@ static void __save_stack_trace(struct stack_trace *trace,
  */
 void save_stack_trace(struct stack_trace *trace)
 {
+	trace->skip++;
 	__save_stack_trace(trace, current, NULL, false);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
@@ -70,6 +71,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 	if (!try_get_task_stack(tsk))
 		return;
 
+	trace->skip++;
 	__save_stack_trace(trace, tsk, NULL, true);
 
 	put_task_stack(tsk);
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] x86, stacktrace: avoid recording save_stack_trace wrappers
  2017-09-07  7:57 [PATCH] x86, stacktrace: avoid recording save_stack_trace wrappers Vlastimil Babka
@ 2017-09-07 13:07 ` Josh Poimboeuf
  2017-09-29  9:23 ` [PATCH v2] " Vlastimil Babka
  1 sibling, 0 replies; 5+ messages in thread
From: Josh Poimboeuf @ 2017-09-07 13:07 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, x86,
	Miroslav Benes, linux-kernel

On Thu, Sep 07, 2017 at 09:57:36AM +0200, Vlastimil Babka wrote:
> The save_stack_trace() and save_stack_trace_tsk() wrappers of
> __save_stack_trace() add themselves to the call stack, and thus appear in the
> recorded stacktraces. This is redundant and wasteful when we have limited space
> to record the useful part of the backtrace with e.g. page_owner functionality.
> 
> Fix this by making sure __save_stack_trace() is noinline (which matches the
> current gcc decision) and bumping the skip in the wrappers. This is similar
> to what was done for arm in 3683f44c42e9 ("ARM: stacktrace: avoid listing
> stacktrace functions in stacktrace") and is pending for arm64.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  arch/x86/kernel/stacktrace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> index 8dabd7bf1673..4b2fd6092739 100644
> --- a/arch/x86/kernel/stacktrace.c
> +++ b/arch/x86/kernel/stacktrace.c
> @@ -30,7 +30,7 @@ static int save_stack_address(struct stack_trace *trace, unsigned long addr,
>  	return 0;
>  }
>  
> -static void __save_stack_trace(struct stack_trace *trace,
> +static void noinline __save_stack_trace(struct stack_trace *trace,
>  			       struct task_struct *task, struct pt_regs *regs,
>  			       bool nosched)
>  {
> @@ -56,6 +56,7 @@ static void __save_stack_trace(struct stack_trace *trace,
>   */
>  void save_stack_trace(struct stack_trace *trace)
>  {
> +	trace->skip++;
>  	__save_stack_trace(trace, current, NULL, false);
>  }
>  EXPORT_SYMBOL_GPL(save_stack_trace);
> @@ -70,6 +71,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
>  	if (!try_get_task_stack(tsk))
>  		return;
>  
> +	trace->skip++;
>  	__save_stack_trace(trace, tsk, NULL, true);
>  
>  	put_task_stack(tsk);

save_stack_trace_tsk() is usually called for other tasks, in which case
these functions aren't on the stack.  So the skip should only be done
when task == current.

Also, I think __save_stack_trace_reliable() should be made
__always_inline (which matches its current GCC behavior) so that it
doesn't accidentally get this problem in the future.

-- 
Josh

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH v2] x86, stacktrace: avoid recording save_stack_trace wrappers
  2017-09-07  7:57 [PATCH] x86, stacktrace: avoid recording save_stack_trace wrappers Vlastimil Babka
  2017-09-07 13:07 ` Josh Poimboeuf
@ 2017-09-29  9:23 ` Vlastimil Babka
  2017-09-29 15:57   ` Josh Poimboeuf
  2017-09-29 20:15   ` [tip:x86/debug] x86/stacktrace: Avoid recording save_stack_trace() wrappers tip-bot for Vlastimil Babka
  1 sibling, 2 replies; 5+ messages in thread
From: Vlastimil Babka @ 2017-09-29  9:23 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H . Peter Anvin
  Cc: x86, Josh Poimboeuf, Miroslav Benes, linux-kernel, Vlastimil Babka

The save_stack_trace() and save_stack_trace_tsk() wrappers of
__save_stack_trace() add themselves to the call stack, and thus appear in the
recorded stacktraces. This is redundant and wasteful when we have limited space
to record the useful part of the backtrace with e.g. page_owner functionality.

Fix this by making sure __save_stack_trace() is noinline (which matches the
current gcc decision) and bumping the skip in the wrappers
(save_stack_trace_tsk() only when called for the current task). This is similar
to what was done for arm in 3683f44c42e9 ("ARM: stacktrace: avoid listing
stacktrace functions in stacktrace") and is pending for arm64.

Also make sure that __save_stack_trace_reliable() doesn't get this problem in
the future by marking it __always_inline (which matches current gcc decision),
per Josh Poimboeuf.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
v2: save_stack_trace_tsk(): skip only when tsk == current; make
    __save_stack_trace_reliable() __always_inline (both suggested by Josh)
    
 arch/x86/kernel/stacktrace.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 8dabd7bf1673..77835bc021c7 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -30,7 +30,7 @@ static int save_stack_address(struct stack_trace *trace, unsigned long addr,
 	return 0;
 }
 
-static void __save_stack_trace(struct stack_trace *trace,
+static void noinline __save_stack_trace(struct stack_trace *trace,
 			       struct task_struct *task, struct pt_regs *regs,
 			       bool nosched)
 {
@@ -56,6 +56,7 @@ static void __save_stack_trace(struct stack_trace *trace,
  */
 void save_stack_trace(struct stack_trace *trace)
 {
+	trace->skip++;
 	__save_stack_trace(trace, current, NULL, false);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
@@ -70,6 +71,8 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 	if (!try_get_task_stack(tsk))
 		return;
 
+	if (tsk == current)
+		trace->skip++;
 	__save_stack_trace(trace, tsk, NULL, true);
 
 	put_task_stack(tsk);
@@ -88,8 +91,9 @@ EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 	}							\
 })
 
-static int __save_stack_trace_reliable(struct stack_trace *trace,
-				       struct task_struct *task)
+static int __always_inline
+__save_stack_trace_reliable(struct stack_trace *trace,
+			    struct task_struct *task)
 {
 	struct unwind_state state;
 	struct pt_regs *regs;
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86, stacktrace: avoid recording save_stack_trace wrappers
  2017-09-29  9:23 ` [PATCH v2] " Vlastimil Babka
@ 2017-09-29 15:57   ` Josh Poimboeuf
  2017-09-29 20:15   ` [tip:x86/debug] x86/stacktrace: Avoid recording save_stack_trace() wrappers tip-bot for Vlastimil Babka
  1 sibling, 0 replies; 5+ messages in thread
From: Josh Poimboeuf @ 2017-09-29 15:57 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, x86,
	Miroslav Benes, linux-kernel

On Fri, Sep 29, 2017 at 11:23:35AM +0200, Vlastimil Babka wrote:
> The save_stack_trace() and save_stack_trace_tsk() wrappers of
> __save_stack_trace() add themselves to the call stack, and thus appear in the
> recorded stacktraces. This is redundant and wasteful when we have limited space
> to record the useful part of the backtrace with e.g. page_owner functionality.
> 
> Fix this by making sure __save_stack_trace() is noinline (which matches the
> current gcc decision) and bumping the skip in the wrappers
> (save_stack_trace_tsk() only when called for the current task). This is similar
> to what was done for arm in 3683f44c42e9 ("ARM: stacktrace: avoid listing
> stacktrace functions in stacktrace") and is pending for arm64.
> 
> Also make sure that __save_stack_trace_reliable() doesn't get this problem in
> the future by marking it __always_inline (which matches current gcc decision),
> per Josh Poimboeuf.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [tip:x86/debug] x86/stacktrace: Avoid recording save_stack_trace() wrappers
  2017-09-29  9:23 ` [PATCH v2] " Vlastimil Babka
  2017-09-29 15:57   ` Josh Poimboeuf
@ 2017-09-29 20:15   ` tip-bot for Vlastimil Babka
  1 sibling, 0 replies; 5+ messages in thread
From: tip-bot for Vlastimil Babka @ 2017-09-29 20:15 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jpoimboe, torvalds, vbabka, mingo, mbenes, linux-kernel, peterz,
	tglx, hpa

Commit-ID:  77072f09eab19326dd2424c8dad0a443341a228f
Gitweb:     https://git.kernel.org/tip/77072f09eab19326dd2424c8dad0a443341a228f
Author:     Vlastimil Babka <vbabka@suse.cz>
AuthorDate: Fri, 29 Sep 2017 11:23:35 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 29 Sep 2017 19:44:03 +0200

x86/stacktrace: Avoid recording save_stack_trace() wrappers

The save_stack_trace() and save_stack_trace_tsk() wrappers of
__save_stack_trace() add themselves to the call stack, and thus appear in the
recorded stacktraces. This is redundant and wasteful when we have limited space
to record the useful part of the backtrace with e.g. page_owner functionality.

Fix this by making sure __save_stack_trace() is noinline (which matches the
current gcc decision) and bumping the skip in the wrappers
(save_stack_trace_tsk() only when called for the current task). This is similar
to what was done for arm in 3683f44c42e9 ("ARM: stacktrace: avoid listing
stacktrace functions in stacktrace") and is pending for arm64.

Also make sure that __save_stack_trace_reliable() doesn't get this problem in
the future by marking it __always_inline (which matches current gcc decision),
per Josh Poimboeuf.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170929092335.2744-1-vbabka@suse.cz
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/stacktrace.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 8dabd7b..77835bc 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -30,7 +30,7 @@ static int save_stack_address(struct stack_trace *trace, unsigned long addr,
 	return 0;
 }
 
-static void __save_stack_trace(struct stack_trace *trace,
+static void noinline __save_stack_trace(struct stack_trace *trace,
 			       struct task_struct *task, struct pt_regs *regs,
 			       bool nosched)
 {
@@ -56,6 +56,7 @@ static void __save_stack_trace(struct stack_trace *trace,
  */
 void save_stack_trace(struct stack_trace *trace)
 {
+	trace->skip++;
 	__save_stack_trace(trace, current, NULL, false);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
@@ -70,6 +71,8 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 	if (!try_get_task_stack(tsk))
 		return;
 
+	if (tsk == current)
+		trace->skip++;
 	__save_stack_trace(trace, tsk, NULL, true);
 
 	put_task_stack(tsk);
@@ -88,8 +91,9 @@ EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 	}							\
 })
 
-static int __save_stack_trace_reliable(struct stack_trace *trace,
-				       struct task_struct *task)
+static int __always_inline
+__save_stack_trace_reliable(struct stack_trace *trace,
+			    struct task_struct *task)
 {
 	struct unwind_state state;
 	struct pt_regs *regs;

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-29 20:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-07  7:57 [PATCH] x86, stacktrace: avoid recording save_stack_trace wrappers Vlastimil Babka
2017-09-07 13:07 ` Josh Poimboeuf
2017-09-29  9:23 ` [PATCH v2] " Vlastimil Babka
2017-09-29 15:57   ` Josh Poimboeuf
2017-09-29 20:15   ` [tip:x86/debug] x86/stacktrace: Avoid recording save_stack_trace() wrappers tip-bot for Vlastimil Babka

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).