Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460217 > unrolled thread

Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

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.

Contents

  Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-11 09:30 +0200
    Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-11 16:30 +0200
      Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-11 17:00 +0200
        Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-11 18:10 +0200
          Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations Andy Lutomirski <luto@amacapital.net> - 2016-08-11 21:00 +0200
            Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and  implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-11 21:20 +0200

#1460217 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Aug 10, 2016 5:16 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>
> On Wed, Aug 10, 2016 at 12:25:11AM -0700, Andy Lutomirski wrote:
> > On Aug 10, 2016 2:27 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> > >
> > > On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> > > > On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > > > > new file mode 100644
> > > > > index 0000000..f28f1b5
> > > > > --- /dev/null
> > > > > +++ b/arch/x86/kernel/unwind_frame.c
> > > > > @@ -0,0 +1,84 @@
> > > > > +#include <linux/sched.h>
> > > > > +#include <asm/ptrace.h>
> > > > > +#include <asm/bitops.h>
> > > > > +#include <asm/stacktrace.h>
> > > > > +#include <asm/unwind.h>
> > > > > +
> > > > > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > > > > +
> > > > > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > > > > +{
> > > > > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > > > > +       unsigned long addr;
> > > > > +
> > > > > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > > > > +               return 0;
> > > > > +
> > > > > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > > > > +                                    addr_p);
> > > > > +
> > > > > +       return __kernel_text_address(addr) ? addr : 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > > > > +
> > > > > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > > > > +                              size_t len)
> > > > > +{
> > > > > +       struct stack_info *info = &state->stack_info;
> > > > > +
> > > > > +       if (on_stack(info, addr, len))
> > > > > +               return true;
> > > > > +
> > > > > +       if (get_stack_info(info->next_sp, state->task, info,
> > > > > +                          &state->stack_mask))
> > > > > +               goto unknown;
> > > > > +
> > > > > +       if (!on_stack(info, addr, len))
> > > > > +               goto unknown;
> > > > > +
> > > > > +       return true;
> > > > > +
> > > > > +unknown:
> > > > > +       info->type = STACK_TYPE_UNKNOWN;
> > > > > +       return false;
> > > > > +}
> > > > > +
> > > > > +bool unwind_next_frame(struct unwind_state *state)
> > > > > +{
> > > > > +       unsigned long *next_bp;
> > > > > +
> > > > > +       if (unwind_done(state))
> > > > > +               return false;
> > > > > +
> > > > > +       next_bp = (unsigned long *)*state->bp;
> > > > > +
> > > > > +       /*
> > > > > +        * Make sure the next frame is on a valid stack and can be accessed
> > > > > +        * safely.
> > > > > +        */
> > > > > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > > > > +               return false;
> > > > > +
> > > > > +       /* move to the next frame */
> > > > > +       state->bp = next_bp;
> > > > > +       return true;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > > > > +
> > > > > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > > > > +                   struct pt_regs *regs, unsigned long *sp)
> > > > > +{
> > > > > +       memset(state, 0, sizeof(*state));
> > > > > +
> > > > > +       state->task = task;
> > > > > +       state->bp = get_frame_pointer(task, regs);
> > > > > +
> > > > > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > > > > +                      &state->stack_mask);
> > > > > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > > > > +
> > > > > +       /* unwind to the first frame after the specified stack pointer */
> > > > > +       while (state->bp < sp && !unwind_done(state))
> > > > > +               unwind_next_frame(state);
> > > >
> > > > Do we unwind all the frames here?  It seems strange to me that in a
> > > > function named __unwind_start(), we unwind all the frames.
> > >
> > > It just skips any stack frames before the specified "sp" pointer.
> > > Several callers use this, for example, to start at regs->sp instead of
> > > the current stack frame.  I'll try to make the comment clearer.
> > >
> >
> > Are you checking the right condition?  Shouldn't this check that sp is
> > in bounds for the current stack if a stack switch happened?
>
> You're right.
>
> > I admit I don't fully understand the use case.  If someone wants to
> > start a trace in the middle, shouldn't they just pass regs in?
>
> The regs aren't always available.  Some callers just want to skip the
> first few frames so the stack dump code itself isn't traced.

I suspect that all users are okay with your algorithm simply because
they don't switch stacks.  Maybe the thing to do is to stop advancing
when sp is passed or if the stack switches at all.

Could you point me at a user that passes anything other than regs->sp
here?  On brief inspection, I haven't found any at all.

--Andy

[toc] | [next] | [standalone]

#1460572 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> On Aug 10, 2016 5:16 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > On Wed, Aug 10, 2016 at 12:25:11AM -0700, Andy Lutomirski wrote:
> > > On Aug 10, 2016 2:27 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> > > >
> > > > On Tue, Aug 09, 2016 at 06:17:41PM -0500, Nilay Vaish wrote:
> > > > > On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > > > > > new file mode 100644
> > > > > > index 0000000..f28f1b5
> > > > > > --- /dev/null
> > > > > > +++ b/arch/x86/kernel/unwind_frame.c
> > > > > > @@ -0,0 +1,84 @@
> > > > > > +#include <linux/sched.h>
> > > > > > +#include <asm/ptrace.h>
> > > > > > +#include <asm/bitops.h>
> > > > > > +#include <asm/stacktrace.h>
> > > > > > +#include <asm/unwind.h>
> > > > > > +
> > > > > > +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
> > > > > > +
> > > > > > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > > > > > +{
> > > > > > +       unsigned long *addr_p = unwind_get_return_address_ptr(state);
> > > > > > +       unsigned long addr;
> > > > > > +
> > > > > > +       if (state->stack_info.type == STACK_TYPE_UNKNOWN)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> > > > > > +                                    addr_p);
> > > > > > +
> > > > > > +       return __kernel_text_address(addr) ? addr : 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > > > > > +
> > > > > > +static bool update_stack_state(struct unwind_state *state, void *addr,
> > > > > > +                              size_t len)
> > > > > > +{
> > > > > > +       struct stack_info *info = &state->stack_info;
> > > > > > +
> > > > > > +       if (on_stack(info, addr, len))
> > > > > > +               return true;
> > > > > > +
> > > > > > +       if (get_stack_info(info->next_sp, state->task, info,
> > > > > > +                          &state->stack_mask))
> > > > > > +               goto unknown;
> > > > > > +
> > > > > > +       if (!on_stack(info, addr, len))
> > > > > > +               goto unknown;
> > > > > > +
> > > > > > +       return true;
> > > > > > +
> > > > > > +unknown:
> > > > > > +       info->type = STACK_TYPE_UNKNOWN;
> > > > > > +       return false;
> > > > > > +}
> > > > > > +
> > > > > > +bool unwind_next_frame(struct unwind_state *state)
> > > > > > +{
> > > > > > +       unsigned long *next_bp;
> > > > > > +
> > > > > > +       if (unwind_done(state))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       next_bp = (unsigned long *)*state->bp;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * Make sure the next frame is on a valid stack and can be accessed
> > > > > > +        * safely.
> > > > > > +        */
> > > > > > +       if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       /* move to the next frame */
> > > > > > +       state->bp = next_bp;
> > > > > > +       return true;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > > > > > +
> > > > > > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > > > > > +                   struct pt_regs *regs, unsigned long *sp)
> > > > > > +{
> > > > > > +       memset(state, 0, sizeof(*state));
> > > > > > +
> > > > > > +       state->task = task;
> > > > > > +       state->bp = get_frame_pointer(task, regs);
> > > > > > +
> > > > > > +       get_stack_info(state->bp, state->task, &state->stack_info,
> > > > > > +                      &state->stack_mask);
> > > > > > +       update_stack_state(state, state->bp, FRAME_HEADER_SIZE);
> > > > > > +
> > > > > > +       /* unwind to the first frame after the specified stack pointer */
> > > > > > +       while (state->bp < sp && !unwind_done(state))
> > > > > > +               unwind_next_frame(state);
> > > > >
> > > > > Do we unwind all the frames here?  It seems strange to me that in a
> > > > > function named __unwind_start(), we unwind all the frames.
> > > >
> > > > It just skips any stack frames before the specified "sp" pointer.
> > > > Several callers use this, for example, to start at regs->sp instead of
> > > > the current stack frame.  I'll try to make the comment clearer.
> > > >
> > >
> > > Are you checking the right condition?  Shouldn't this check that sp is
> > > in bounds for the current stack if a stack switch happened?
> >
> > You're right.
> >
> > > I admit I don't fully understand the use case.  If someone wants to
> > > start a trace in the middle, shouldn't they just pass regs in?
> >
> > The regs aren't always available.  Some callers just want to skip the
> > first few frames so the stack dump code itself isn't traced.
> 
> I suspect that all users are okay with your algorithm simply because
> they don't switch stacks.  Maybe the thing to do is to stop advancing
> when sp is passed or if the stack switches at all.

There are actually some cases which could be broken by the sloppy "while
state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
points to a different stack than the current one.  It seemed to work in
my testing, but I guess I got lucky, with irq percpu stack addresses
being smaller than the thread stack addresses.

> Could you point me at a user that passes anything other than regs->sp
> here?  On brief inspection, I haven't found any at all.

For example, see show_stack().

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1460590

On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
>> > > I admit I don't fully understand the use case.  If someone wants to
>> > > start a trace in the middle, shouldn't they just pass regs in?
>> >
>> > The regs aren't always available.  Some callers just want to skip the
>> > first few frames so the stack dump code itself isn't traced.
>>
>> I suspect that all users are okay with your algorithm simply because
>> they don't switch stacks.  Maybe the thing to do is to stop advancing
>> when sp is passed or if the stack switches at all.
>
> There are actually some cases which could be broken by the sloppy "while
> state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> points to a different stack than the current one.  It seemed to work in
> my testing, but I guess I got lucky, with irq percpu stack addresses
> being smaller than the thread stack addresses.
>
>> Could you point me at a user that passes anything other than regs->sp
>> here?  On brief inspection, I haven't found any at all.
>
> For example, see show_stack().

Is that a non-trivial case?  show_stack() is generating sp *and* bp.
Why not just pass both of them all the way in to show_trace_log_lvl
(which the existing code already does) and then pass it into
unwind_start?

Alternatively, since that risks causing a bit of loss if you implement
DWARF, you could add an unwind_start_here() function that captures the
state in the calling function and pass the result all the way through.
Or you could write a silly asm helper that literally fills in a struct
pt_regs for the current context (although that could get a bit awkward
for 32-bit, since pt_regs doesn't really contain sp there).

If there are genuinely zero non-trivial cases, then this should fully
solve the problem, no?

Aside: the current code looks a bit silly to me:

    unsigned long stack;
    ...
        sp = &stack;
        bp = stack_frame(current, NULL);

Why not just force a frame pointer and read out sp and bp using asm?

--Andy

[toc] | [prev] | [next] | [standalone]

#1460645 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> >> > > I admit I don't fully understand the use case.  If someone wants to
> >> > > start a trace in the middle, shouldn't they just pass regs in?
> >> >
> >> > The regs aren't always available.  Some callers just want to skip the
> >> > first few frames so the stack dump code itself isn't traced.
> >>
> >> I suspect that all users are okay with your algorithm simply because
> >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> >> when sp is passed or if the stack switches at all.
> >
> > There are actually some cases which could be broken by the sloppy "while
> > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > points to a different stack than the current one.  It seemed to work in
> > my testing, but I guess I got lucky, with irq percpu stack addresses
> > being smaller than the thread stack addresses.
> >
> >> Could you point me at a user that passes anything other than regs->sp
> >> here?  On brief inspection, I haven't found any at all.
> >
> > For example, see show_stack().
> 
> Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> Why not just pass both of them all the way in to show_trace_log_lvl
> (which the existing code already does) and then pass it into
> unwind_start?
> 
> Alternatively, since that risks causing a bit of loss if you implement
> DWARF, you could add an unwind_start_here() function that captures the
> state in the calling function and pass the result all the way through.
> Or you could write a silly asm helper that literally fills in a struct
> pt_regs for the current context (although that could get a bit awkward
> for 32-bit, since pt_regs doesn't really contain sp there).
> 
> If there are genuinely zero non-trivial cases, then this should fully
> solve the problem, no?

Hm, what do you mean by non-trivial cases?

As far as I can tell, they're all trivial, and we don't need the caller
to provide bp.  Just start with the current frame's bp and unwind until
bp and sp are on the same stack and bp > sp.

> Aside: the current code looks a bit silly to me:
> 
>     unsigned long stack;
>     ...
>         sp = &stack;
>         bp = stack_frame(current, NULL);
> 
> Why not just force a frame pointer and read out sp and bp using asm?

Due to the above-discussed algorithm, we no longer need to get bp in
show_stack() (though I forgot to remove the get_frame_pointer() call in
my patches).

And I changed the 'sp = &stack' to a call to get_stack_pointer().

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1460742

On Aug 11, 2016 7:09 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>
> On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> > On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> > >> > > I admit I don't fully understand the use case.  If someone wants to
> > >> > > start a trace in the middle, shouldn't they just pass regs in?
> > >> >
> > >> > The regs aren't always available.  Some callers just want to skip the
> > >> > first few frames so the stack dump code itself isn't traced.
> > >>
> > >> I suspect that all users are okay with your algorithm simply because
> > >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> > >> when sp is passed or if the stack switches at all.
> > >
> > > There are actually some cases which could be broken by the sloppy "while
> > > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > > points to a different stack than the current one.  It seemed to work in
> > > my testing, but I guess I got lucky, with irq percpu stack addresses
> > > being smaller than the thread stack addresses.
> > >
> > >> Could you point me at a user that passes anything other than regs->sp
> > >> here?  On brief inspection, I haven't found any at all.
> > >
> > > For example, see show_stack().
> >
> > Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> > Why not just pass both of them all the way in to show_trace_log_lvl
> > (which the existing code already does) and then pass it into
> > unwind_start?
> >
> > Alternatively, since that risks causing a bit of loss if you implement
> > DWARF, you could add an unwind_start_here() function that captures the
> > state in the calling function and pass the result all the way through.
> > Or you could write a silly asm helper that literally fills in a struct
> > pt_regs for the current context (although that could get a bit awkward
> > for 32-bit, since pt_regs doesn't really contain sp there).
> >
> > If there are genuinely zero non-trivial cases, then this should fully
> > solve the problem, no?
>
> Hm, what do you mean by non-trivial cases?
>
> As far as I can tell, they're all trivial, and we don't need the caller
> to provide bp.  Just start with the current frame's bp and unwind until
> bp and sp are on the same stack and bp > sp.

Given that your unwind-till-we-get-there algorithm isn't quite
correct, wouldn't it be easier to just pass all the needed information
through?  Especially since that information is there (in at least one
case) on current kernels, so the diff would be smaller.  It seems
overcomplicated to me to regenerate the state that was already in the
registers by unwinding to it rather than just passing it in to the
unwinder directly.

--Andy

[toc] | [prev] | [next] | [standalone]

#1460745 — Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

On Thu, Aug 11, 2016 at 11:58:13AM -0700, Andy Lutomirski wrote:
> On Aug 11, 2016 7:09 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > On Thu, Aug 11, 2016 at 07:58:34AM -0700, Andy Lutomirski wrote:
> > > On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
> > > >> > > I admit I don't fully understand the use case.  If someone wants to
> > > >> > > start a trace in the middle, shouldn't they just pass regs in?
> > > >> >
> > > >> > The regs aren't always available.  Some callers just want to skip the
> > > >> > first few frames so the stack dump code itself isn't traced.
> > > >>
> > > >> I suspect that all users are okay with your algorithm simply because
> > > >> they don't switch stacks.  Maybe the thing to do is to stop advancing
> > > >> when sp is passed or if the stack switches at all.
> > > >
> > > > There are actually some cases which could be broken by the sloppy "while
> > > > state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> > > > points to a different stack than the current one.  It seemed to work in
> > > > my testing, but I guess I got lucky, with irq percpu stack addresses
> > > > being smaller than the thread stack addresses.
> > > >
> > > >> Could you point me at a user that passes anything other than regs->sp
> > > >> here?  On brief inspection, I haven't found any at all.
> > > >
> > > > For example, see show_stack().
> > >
> > > Is that a non-trivial case?  show_stack() is generating sp *and* bp.
> > > Why not just pass both of them all the way in to show_trace_log_lvl
> > > (which the existing code already does) and then pass it into
> > > unwind_start?
> > >
> > > Alternatively, since that risks causing a bit of loss if you implement
> > > DWARF, you could add an unwind_start_here() function that captures the
> > > state in the calling function and pass the result all the way through.
> > > Or you could write a silly asm helper that literally fills in a struct
> > > pt_regs for the current context (although that could get a bit awkward
> > > for 32-bit, since pt_regs doesn't really contain sp there).
> > >
> > > If there are genuinely zero non-trivial cases, then this should fully
> > > solve the problem, no?
> >
> > Hm, what do you mean by non-trivial cases?
> >
> > As far as I can tell, they're all trivial, and we don't need the caller
> > to provide bp.  Just start with the current frame's bp and unwind until
> > bp and sp are on the same stack and bp > sp.
> 
> Given that your unwind-till-we-get-there algorithm isn't quite
> correct

But it *will* be correct in v3, with a minor change.  Sneak preview:

	/*
	 * The caller can optionally provide a stack pointer directly
	 * (first_sp) or indirectly (regs->sp), which indicates which stack
	 * frame to start unwinding at.  Skip ahead until we reach that frame.
	 */
	while (!unwind_done(state) &&
	       (!on_stack(&state->stack_info, first_sp, sizeof(*first_sp) ||
		state->bp < first_sp)))
		unwind_next_frame(state);

> wouldn't it be easier to just pass all the needed information
> through?  Especially since that information is there (in at least one
> case) on current kernels, so the diff would be smaller.

Why focus on diff size?  Isn't it the resulting code that's most
important?  It's not like removing the bp argument causes a big diff
anyway.

> It seems overcomplicated to me to regenerate the state that was
> already in the registers by unwinding to it rather than just passing
> it in to the unwinder directly.

Passing in the frame pointer when it isn't needed makes the interface
more complex.  It creates more opportunity for the caller to mess things
up and more edge cases to consider in the unwinder.

And as you mentioned, it's not compatible with DWARF.  Adding
unwind_start_here() would add even more interface complexity.

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web