Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190418160730.11901-1-kasong@redhat.com> <20190419005821.ttlpav7gviaed7am@treble>
In-Reply-To: <20190419005821.ttlpav7gviaed7am@treble>
From:   Kairui Song <kasong@redhat.com>
Date:   Fri, 19 Apr 2019 10:17:49 +0800
Message-ID: <CACPcB9fsYqs+xaj6w6ExKdpi-Pk4RqiD5HxTyURpee_arYQANg@mail.gmail.com>
Subject: Re: [RFC PATCH v3] perf/x86: make perf callchain work without CONFIG_FRAME_POINTER
To:     Josh Poimboeuf <jpoimboe@redhat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 19, 2019 at 8:58 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> I still don't like using regs->bp because it results in different code
> paths for FP and ORC.  In the FP case, the regs are treated like real
> regs even though they're fake.
>
> Something like the below would be much simpler.  Would this work?  I don't
> know if any other code relies on the fake regs->bp or regs->sp.

Works perfectly. My only concern is that FP path used to work very
well, not sure it's a good idea to change it, and this may bring some
extra overhead for FP path.

>
> (BTW, tomorrow is a US holiday so I may not be very responsive until
> Monday.)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index de1a924a4914..f315425d8468 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2382,6 +2382,15 @@ void arch_perf_update_userpage(struct perf_event *event,
>         cyc2ns_read_end();
>  }
>
> +/*
> + * Determine whether the regs were taken from an irq/exception handler rather
> + * than from perf_arch_fetch_caller_regs().
> + */
> +static bool perf_hw_regs(struct pt_regs *regs)
> +{
> +       return regs->flags & X86_EFLAGS_FIXED;
> +}
> +
>  void
>  perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
>  {
> @@ -2393,11 +2402,15 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
>                 return;
>         }
>
> -       if (perf_callchain_store(entry, regs->ip))
> -               return;
> +       if (perf_hw_regs(regs)) {
> +               if (perf_callchain_store(entry, regs->ip))
> +                       return;
> +               unwind_start(&state, current, regs, NULL);
> +       } else {
> +               unwind_start(&state, current, NULL, (void *)regs->sp);
> +       }
>
> -       for (unwind_start(&state, current, regs, NULL); !unwind_done(&state);
> -            unwind_next_frame(&state)) {
> +       for (; !unwind_done(&state); unwind_next_frame(&state)) {
>                 addr = unwind_get_return_address(&state);
>                 if (!addr || perf_callchain_store(entry, addr))
>                         return;
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index 04768a3a5454..1392d5e6e8d6 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -308,14 +308,9 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs);
>   */
>  #define perf_arch_fetch_caller_regs(regs, __ip)                {       \
>         (regs)->ip = (__ip);                                    \
> -       (regs)->bp = caller_frame_pointer();                    \
> +       (regs)->sp = (unsigned long)__builtin_frame_address(0); \
>         (regs)->cs = __KERNEL_CS;                               \
>         regs->flags = 0;                                        \
> -       asm volatile(                                           \
> -               _ASM_MOV "%%"_ASM_SP ", %0\n"                   \
> -               : "=m" ((regs)->sp)                             \
> -               :: "memory"                                     \
> -       );                                                      \
>  }
>
>  struct perf_guest_switch_msr {
> diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> index d6d758a187b6..a8d0cdf48616 100644
> --- a/arch/x86/include/asm/stacktrace.h
> +++ b/arch/x86/include/asm/stacktrace.h
> @@ -100,19 +100,6 @@ struct stack_frame_ia32 {
>      u32 return_address;
>  };
>
> -static inline unsigned long caller_frame_pointer(void)
> -{
> -       struct stack_frame *frame;
> -
> -       frame = __builtin_frame_address(0);
> -
> -#ifdef CONFIG_FRAME_POINTER
> -       frame = frame->next_frame;
> -#endif
> -
> -       return (unsigned long)frame;
> -}
> -
>  void show_opcodes(struct pt_regs *regs, const char *loglvl);
>  void show_ip(struct pt_regs *regs, const char *loglvl);
>  #endif /* _ASM_X86_STACKTRACE_H */
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index f3864e1c5569..0f560069aeec 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1062,7 +1062,7 @@ static inline void perf_arch_fetch_caller_regs(struct pt_regs *regs, unsigned lo
>   * the nth caller. We only need a few of the regs:
>   * - ip for PERF_SAMPLE_IP
>   * - cs for user_mode() tests
> - * - bp for callchains
> + * - sp for callchains
>   * - eflags, for future purposes, just in case
>   */
>  static inline void perf_fetch_caller_regs(struct pt_regs *regs)

-- 
Best Regards,
Kairui Song