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

From: Greentime Hu <green.hu@gmail.com>
To: Guo Ren <guoren@kernel.org>, greentime.hu@sifive.com
Cc: linux-arch <linux-arch@vger.kernel.org>,
	Palmer Dabbelt <palmer@sifive.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	linux-riscv@lists.infradead.org, Christoph Hellwig <hch@lst.de>,
	Mao Han <han_mao@c-sky.com>
Subject: Re: [PATCH V5 1/3] riscv: Add perf callchain support
Date: Mon, 26 Aug 2019 16:03:07 +0800	[thread overview]
Message-ID: <CAEbi=3dtDy=CMRagVrj0ihtpYqS+4NkK7eYmn6Gn=2bV9khWVg@mail.gmail.com> (raw)
In-Reply-To: <CAJF2gTS80XU=4z-_=N=oGV6GH-+8KXCa74DyhVMcRxJRBq5g4A@mail.gmail.com>

Hi Guo,

Guo Ren <guoren@kernel.org> 於 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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
