Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1554562 > unrolled thread
| Started by | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| First post | 2017-01-09 19:10 +0100 |
| Last post | 2017-01-12 11:20 +0100 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 3/4] x86/unwind: include __schedule() in stack traces Josh Poimboeuf <jpoimboe@redhat.com> - 2017-01-09 19:10 +0100
Re: [PATCH 3/4] x86/unwind: include __schedule() in stack traces Miroslav Benes <mbenes@suse.cz> - 2017-01-10 11:20 +0100
Re: [PATCH 3/4] x86/unwind: include __schedule() in stack traces Josh Poimboeuf <jpoimboe@redhat.com> - 2017-01-10 18:30 +0100
Re: [PATCH 3/4] x86/unwind: include __schedule() in stack traces Miroslav Benes <mbenes@suse.cz> - 2017-01-10 21:10 +0100
[tip:x86/urgent] x86/unwind: Include __schedule() in stack traces tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2017-01-12 11:20 +0100
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-01-09 19:10 +0100 |
| Subject | [PATCH 3/4] x86/unwind: include __schedule() in stack traces |
| Message-ID | <sXMyl-7wx-13@gated-at.bofh.it> |
In the following commit:
0100301bfdf5 ("sched/x86: Rewrite the switch_to() code")
... the layout of the 'inactive_task_frame' struct was designed to have
a frame pointer header embedded in it, so that the unwinder could use
the 'bp' and 'ret_addr' fields to report __schedule() on the stack (or
ret_from_fork() for newly forked tasks which haven't actually run yet).
Finish the job by changing get_frame_pointer() to return a pointer to
inactive_task_frame's 'bp' field rather than 'bp' itself. This allows
the unwinder to start one frame higher on the stack, so that it properly
reports __schedule().
Reported-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/include/asm/stacktrace.h | 5 +----
arch/x86/include/asm/switch_to.h | 10 +++++++++-
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 20ce3db..2e41c50 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -52,16 +52,13 @@ static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
static inline unsigned long *
get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
{
- struct inactive_task_frame *frame;
-
if (regs)
return (unsigned long *)regs->bp;
if (task == current)
return __builtin_frame_address(0);
- frame = (struct inactive_task_frame *)task->thread.sp;
- return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
+ return &((struct inactive_task_frame *)task->thread.sp)->bp;
}
#else
static inline unsigned long *
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 5cb436a..fcc5cd3 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -36,7 +36,10 @@ static inline void prepare_switch_to(struct task_struct *prev,
asmlinkage void ret_from_fork(void);
-/* data that is pointed to by thread.sp */
+/*
+ * This is the structure pointed to by thread.sp for an inactive task. The
+ * order of the fields must match the code in __switch_to_asm().
+ */
struct inactive_task_frame {
#ifdef CONFIG_X86_64
unsigned long r15;
@@ -48,6 +51,11 @@ struct inactive_task_frame {
unsigned long di;
#endif
unsigned long bx;
+
+ /*
+ * These two fields must be together. They form a stack frame header,
+ * needed by get_frame_pointer().
+ */
unsigned long bp;
unsigned long ret_addr;
};
--
2.7.4
[toc] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2017-01-10 11:20 +0100 |
| Message-ID | <sY1H4-8v8-11@gated-at.bofh.it> |
| In reply to | #1554562 |
On Mon, 9 Jan 2017, Josh Poimboeuf wrote:
> In the following commit:
>
> 0100301bfdf5 ("sched/x86: Rewrite the switch_to() code")
>
> ... the layout of the 'inactive_task_frame' struct was designed to have
> a frame pointer header embedded in it, so that the unwinder could use
> the 'bp' and 'ret_addr' fields to report __schedule() on the stack (or
> ret_from_fork() for newly forked tasks which haven't actually run yet).
>
> Finish the job by changing get_frame_pointer() to return a pointer to
> inactive_task_frame's 'bp' field rather than 'bp' itself. This allows
> the unwinder to start one frame higher on the stack, so that it properly
> reports __schedule().
>
> Reported-by: Miroslav Benes <mbenes@suse.cz>
You can also add my
Tested-by: Miroslav Benes <mbenes@suse.cz>
One ignorant question below.
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/include/asm/stacktrace.h | 5 +----
> arch/x86/include/asm/switch_to.h | 10 +++++++++-
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> index 20ce3db..2e41c50 100644
> --- a/arch/x86/include/asm/stacktrace.h
> +++ b/arch/x86/include/asm/stacktrace.h
> @@ -52,16 +52,13 @@ static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
> static inline unsigned long *
> get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
> {
> - struct inactive_task_frame *frame;
> -
> if (regs)
> return (unsigned long *)regs->bp;
>
> if (task == current)
> return __builtin_frame_address(0);
>
> - frame = (struct inactive_task_frame *)task->thread.sp;
> - return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
> + return &((struct inactive_task_frame *)task->thread.sp)->bp;
You effectively remove one of the changes from the previous patch -
READ_ONCE_NOCHECK. Is it intentional?
Regards,
Miroslav
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-01-10 18:30 +0100 |
| Message-ID | <sY8pc-4aE-1@gated-at.bofh.it> |
| In reply to | #1555107 |
On Tue, Jan 10, 2017 at 11:14:51AM +0100, Miroslav Benes wrote:
> On Mon, 9 Jan 2017, Josh Poimboeuf wrote:
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> > arch/x86/include/asm/stacktrace.h | 5 +----
> > arch/x86/include/asm/switch_to.h | 10 +++++++++-
> > 2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> > index 20ce3db..2e41c50 100644
> > --- a/arch/x86/include/asm/stacktrace.h
> > +++ b/arch/x86/include/asm/stacktrace.h
> > @@ -52,16 +52,13 @@ static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
> > static inline unsigned long *
> > get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
> > {
> > - struct inactive_task_frame *frame;
> > -
> > if (regs)
> > return (unsigned long *)regs->bp;
> >
> > if (task == current)
> > return __builtin_frame_address(0);
> >
> > - frame = (struct inactive_task_frame *)task->thread.sp;
> > - return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
> > + return &((struct inactive_task_frame *)task->thread.sp)->bp;
>
> You effectively remove one of the changes from the previous patch -
> READ_ONCE_NOCHECK. Is it intentional?
Yes, notice that it's no longer reading the value of bp on the stack.
It's instead getting a pointer to it. Since there's no longer a stack
access, READ_ONCE_NOCHECK is no longer needed.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2017-01-10 21:10 +0100 |
| Message-ID | <sYaU2-5Ln-25@gated-at.bofh.it> |
| In reply to | #1555729 |
On Tue, 10 Jan 2017, Josh Poimboeuf wrote:
> On Tue, Jan 10, 2017 at 11:14:51AM +0100, Miroslav Benes wrote:
> > On Mon, 9 Jan 2017, Josh Poimboeuf wrote:
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > ---
> > > arch/x86/include/asm/stacktrace.h | 5 +----
> > > arch/x86/include/asm/switch_to.h | 10 +++++++++-
> > > 2 files changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> > > index 20ce3db..2e41c50 100644
> > > --- a/arch/x86/include/asm/stacktrace.h
> > > +++ b/arch/x86/include/asm/stacktrace.h
> > > @@ -52,16 +52,13 @@ static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
> > > static inline unsigned long *
> > > get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
> > > {
> > > - struct inactive_task_frame *frame;
> > > -
> > > if (regs)
> > > return (unsigned long *)regs->bp;
> > >
> > > if (task == current)
> > > return __builtin_frame_address(0);
> > >
> > > - frame = (struct inactive_task_frame *)task->thread.sp;
> > > - return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
> > > + return &((struct inactive_task_frame *)task->thread.sp)->bp;
> >
> > You effectively remove one of the changes from the previous patch -
> > READ_ONCE_NOCHECK. Is it intentional?
>
> Yes, notice that it's no longer reading the value of bp on the stack.
> It's instead getting a pointer to it. Since there's no longer a stack
> access, READ_ONCE_NOCHECK is no longer needed.
Oh, of course. Sorry for the noise.
Miroslav
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Josh Poimboeuf <tipbot@zytor.com> |
|---|---|
| Date | 2017-01-12 11:20 +0100 |
| Subject | [tip:x86/urgent] x86/unwind: Include __schedule() in stack traces |
| Message-ID | <sYKEa-2K5-35@gated-at.bofh.it> |
| In reply to | #1554562 |
Commit-ID: 2c96b2fe9c57b4267c3f0a680d82d7cc52e1c447
Gitweb: http://git.kernel.org/tip/2c96b2fe9c57b4267c3f0a680d82d7cc52e1c447
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Mon, 9 Jan 2017 12:00:24 -0600
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 12 Jan 2017 09:28:28 +0100
x86/unwind: Include __schedule() in stack traces
In the following commit:
0100301bfdf5 ("sched/x86: Rewrite the switch_to() code")
... the layout of the 'inactive_task_frame' struct was designed to have
a frame pointer header embedded in it, so that the unwinder could use
the 'bp' and 'ret_addr' fields to report __schedule() on the stack (or
ret_from_fork() for newly forked tasks which haven't actually run yet).
Finish the job by changing get_frame_pointer() to return a pointer to
inactive_task_frame's 'bp' field rather than 'bp' itself. This allows
the unwinder to start one frame higher on the stack, so that it properly
reports __schedule().
Reported-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Jones <davej@codemonkey.org.uk>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/598e9f7505ed0aba86e8b9590aa528c6c7ae8dcd.1483978430.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/include/asm/stacktrace.h | 5 +----
arch/x86/include/asm/switch_to.h | 10 +++++++++-
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 20ce3db..2e41c50 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -52,16 +52,13 @@ static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
static inline unsigned long *
get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
{
- struct inactive_task_frame *frame;
-
if (regs)
return (unsigned long *)regs->bp;
if (task == current)
return __builtin_frame_address(0);
- frame = (struct inactive_task_frame *)task->thread.sp;
- return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
+ return &((struct inactive_task_frame *)task->thread.sp)->bp;
}
#else
static inline unsigned long *
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 5cb436a..fcc5cd3 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -36,7 +36,10 @@ static inline void prepare_switch_to(struct task_struct *prev,
asmlinkage void ret_from_fork(void);
-/* data that is pointed to by thread.sp */
+/*
+ * This is the structure pointed to by thread.sp for an inactive task. The
+ * order of the fields must match the code in __switch_to_asm().
+ */
struct inactive_task_frame {
#ifdef CONFIG_X86_64
unsigned long r15;
@@ -48,6 +51,11 @@ struct inactive_task_frame {
unsigned long di;
#endif
unsigned long bx;
+
+ /*
+ * These two fields must be together. They form a stack frame header,
+ * needed by get_frame_pointer().
+ */
unsigned long bp;
unsigned long ret_addr;
};
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web