Re: [PATCH V5 1/3] riscv: Add perf callchain support

[Greentime Hu]

Hi Guo,

Guo Ren <guoren@xxxxxxxxxx> æ 2019å8æ24æ éå äå8:54åéï
>
> Please check CONFIG_FRAME_POINTER
>
> 1 *frame = *((struct stackframe *)frame->fp - 1);
> This code is origionally from riscv/kernel/stacktrace.c: walk_stackframe
>
> In linux/Makefile it'll involve the options for gcc to definitely
> store ra & prev_fp in fp pointer.
> ifdef CONFIG_FRAME_POINTER
> KBUILD_CFLAGS += -fno-omit-frame-pointer -fno-optimize-sibling-calls
>
> So --call-graph fp need depends on CONFIG_FRAME_POINTER.
>

I am pretty sure CONFIG_FRAME_POINTER is Y
# zcat /proc/config.gz |grep CONFIG_FRAME_POINTER
CONFIG_FRAME_POINTER=y

This is not going to go wrong every time, the probability of error is
about one tenth or one quarter. randomly
There may be some conditions that we have not considered.

I add one more condition to check if it is a valid virtual address and
it( ./perf record -e cpu-clock --call-graph fp ls) passes 1000 times
without failure in Unleashed board based on 5.3-rc5.
Here is my patch. Please have  a look at it. I am not sure if it is a
good solution.

diff --git a/arch/riscv/kernel/perf_callchain.c
b/arch/riscv/kernel/perf_callchain.c
index d75d15c13dc7..4717942435df 100644
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
@@ -18,6 +18,8 @@ static int unwind_frame_kernel(struct stackframe *frame)
                return -EPERM;
        if (frame->fp < CONFIG_PAGE_OFFSET)
                return -EPERM;
+       if (!virt_addr_valid(frame->fp))
+               return -EPERM;

        *frame = *((struct stackframe *)frame->fp - 1);
        if (__kernel_text_address(frame->ra)) {

It could catch cases called in this way.

[ 1381.936586] frame->fp=:ffffffff00547550
[ 1382.038542] CPU: 1 PID: 135 Comm: ls Not tainted
5.3.0-rc5-00003-gb008f6bcd67c-dirty #14
[ 1382.307440] Call Trace:
[ 1382.388947] [<ffffffe0002a2d8e>] walk_stackframe+0x0/0x9a
[ 1382.568053] [<ffffffe0002a2f5a>] show_stack+0x2a/0x34
[ 1382.735960] [<ffffffe00083dcd6>] dump_stack+0x62/0x7c
[ 1382.903863] [<ffffffe0002a49e0>] perf_callchain_kernel+0xd8/0x102
[ 1383.106558] [<ffffffe000340a6e>] get_perf_callchain+0x136/0x1f2
[ 1383.303128] [<ffffffe00033d480>] perf_callchain+0x52/0x6e
[ 1383.482553] [<ffffffe00033d50a>] perf_prepare_sample+0x6e/0x476
[ 1383.679357] [<ffffffe00033d92e>] perf_event_output_forward+0x1c/0x50
[ 1383.890633] [<ffffffe000338b4c>] __perf_event_overflow+0x6a/0xa4
[ 1384.090279] [<ffffffe000338c40>] perf_swevent_hrtimer+0xba/0x106
[ 1384.290094] [<ffffffe0002f307c>] __hrtimer_run_queues+0x84/0x108
[ 1384.489694] [<ffffffe0002f36b8>] hrtimer_interrupt+0xca/0x1ce
[ 1384.680974] [<ffffffe00072f572>] riscv_timer_interrupt+0x32/0x3a
[ 1384.880449] [<ffffffe000854b34>] do_IRQ+0x64/0xbe
[ 1385.036698] [<ffffffe0002a1c5c>] ret_from_exception+0x0/0xc

[13915.697989] frame->fp=:fffffffffffff000
[13915.799937] CPU: 2 PID: 663 Comm: ls Not tainted
5.3.0-rc5-00003-gb008f6bcd67c-dirty #14
[13916.068832] Call Trace:
[13916.150380] [<ffffffe0002a2d8e>] walk_stackframe+0x0/0x9a
[13916.329450] [<ffffffe0002a2f5a>] show_stack+0x2a/0x34
[13916.497360] [<ffffffe00083dcd6>] dump_stack+0x62/0x7c
[13916.665265] [<ffffffe0002a49e0>] perf_callchain_kernel+0xd8/0x102
[13916.867949] [<ffffffe000340a6e>] get_perf_callchain+0x136/0x1f2
[13917.064526] [<ffffffe00033d480>] perf_callchain+0x52/0x6e
[13917.243950] [<ffffffe00033d50a>] perf_prepare_sample+0x6e/0x476
[13917.440759] [<ffffffe00033d92e>] perf_event_output_forward+0x1c/0x50
[13917.652021] [<ffffffe000338b4c>] __perf_event_overflow+0x6a/0xa4
[13917.851683] [<ffffffe000338c40>] perf_swevent_hrtimer+0xba/0x106
[13918.051494] [<ffffffe0002f307c>] __hrtimer_run_queues+0x84/0x108
[13918.251094] [<ffffffe0002f36b8>] hrtimer_interrupt+0xca/0x1ce
[13918.442379] [<ffffffe00072f572>] riscv_timer_interrupt+0x32/0x3a
[13918.641840] [<ffffffe000854b34>] do_IRQ+0x64/0xbe
[13918.798082] [<ffffffe0002a1c5c>] ret_from_exception+0x0/0xc