Re: [RFC PATCH v2] perf/x86: make perf callchain work without CONFIG_FRAME_POINTER

[Kairui Song]

On Tue, Apr 16, 2019 at 12:59 AM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> On Mon, Apr 15, 2019 at 05:36:22PM +0200, Peter Zijlstra wrote:
> >
> > I'll mostly defer to Josh on unwinding, but a few comments below.
> >
> > On Tue, Apr 09, 2019 at 12:59:42AM +0800, Kairui Song wrote:
> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > > index e2b1447192a8..6075a4f94376 100644
> > > --- a/arch/x86/events/core.c
> > > +++ b/arch/x86/events/core.c
> > > @@ -2355,6 +2355,12 @@ void arch_perf_update_userpage(struct perf_event *event,
> > >     cyc2ns_read_end();
> > >  }
> > >
> > > +static inline int
> > > +valid_perf_registers(struct pt_regs *regs)
> > > +{
> > > +   return (regs->ip && regs->bp && regs->sp);
> > > +}
> >
> > I'm unconvinced by this, with both guess and orc having !bp is perfectly
> > valid.
> >
> > >  void
> > >  perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
> > >  {
> > > @@ -2366,11 +2372,17 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
> > >             return;
> > >     }
> > >
> > > -   if (perf_callchain_store(entry, regs->ip))
> > > +   if (valid_perf_registers(regs)) {
> > > +           if (perf_callchain_store(entry, regs->ip))
> > > +                   return;
> > > +           unwind_start(&state, current, regs, NULL);
> > > +   } else if (regs->sp) {
> > > +           unwind_start(&state, current, NULL, (unsigned long *)regs->sp);
> > > +   } else {
> > >             return;
> > > +   }
> >
> > AFAICT if we, by pure accident, end up with !bp for ORC, then we
> > initialize the unwind wrong.
> >
> > Note that @regs is mostly trivially correct, except for that tracepoint
> > case. So I don't think we should magic here.
>
> Ah, I didn't quite understand this code before, and I still don't
> really, but I guess the issue is that @regs can be either real or fake.
>
> In the real @regs case, we just want to always unwind starting from
> regs->sp.
>
> But in the fake @regs case, we should instead unwind from the current
> frame, skipping all frames until we hit the fake regs->sp.  Because
> starting from fake/incomplete regs is most likely going to cause
> problems with ORC (or DWARF for other arches).
>
> The idea of a fake regs is fragile and confusing.  Is it possible to
> just pass in the "skip" stack pointer directly instead?  That should
> work for both FP and non-FP.  And I _think_ there's no need to ever
> capture regs->bp anyway -- the stack pointer should be sufficient.

Hi, that will break some other usage, if perf_callchain_kernel is
called but it won't unwind to the callsite (could be produced by
attach an ebpf call to kprobe), things will also go wrong. It should
start with given registers when the register is valid.
And it's true with omit frame pointer BP value could be anything, so 0
is also valid, I think I need to find a better way to tell if we could
start with the registers value or direct start unwinding and skip
until got the stack.

>
> In other words, either regs should be "real", and skip_sp is NULL; or
> regs should be NULL and skip_sp should have a value.
>
> --
> Josh
--
Best Regards,
Kairui Song