From: AKASHI Takahiro <takahiro.akashi@linaro.org> To: Will Deacon <will.deacon@arm.com>, Jungseok Lee <jungseoklee85@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org>, Mark Rutland <Mark.Rutland@arm.com>, Catalin Marinas <Catalin.Marinas@arm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "broonie@kernel.org" <broonie@kernel.org>, "david.griego@linaro.org" <david.griego@linaro.org>, "olof@lixom.net" <olof@lixom.net>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [RFC 2/3] arm64: refactor save_stack_trace() Date: Tue, 21 Jul 2015 19:26:39 +0900 [thread overview] Message-ID: <55AE1E5F.6060407@linaro.org> (raw) In-Reply-To: <55AD89FE.1010706@linaro.org> On 07/21/2015 08:53 AM, AKASHI Takahiro wrote: > Hi > > So i don't have to repost my patch here. Please use the original > commit log message[1/3] as is. > But please keep in mind that there is still an issue that I mentioned > in the cover letter; 'Size' field is inaccurate. > <reported size> = <its own dynamic local variables> > + <child's local variables> > and > <real size> = <reported size> + <its local variables> > - <child's local variables> > where "dynamic" means, for example, a variable allocated like the below: > int foo(int num) { > int array[num]; > ... > } > (See more details in my ascii art.) > > Such usage is seldom seen in the kernel, and <reported size> is > likely equal to <child's local variables>. In other words, we will > see one-line *displacement* in most cases. Well, I have a quick fix now, but it looks ugly. In addition, I found another issue; With function_graph tracer, the output is like: # cat /sys/kernel/tracing/stack_trace Depth Size Location (78 entries) ----- ---- -------- 0) 6184 32 update_min_vruntime+0x14/0x74 1) 6152 48 update_curr+0x6c/0x150 2) 6104 128 enqueue_task_fair+0x2f4/0xb9c 3) 5976 48 enqueue_task+0x48/0x90 ... 18) 5160 112 hrtimer_interrupt+0xa0/0x214 19) 5048 32 arch_timer_handler_phys+0x38/0x48 20) 5016 0 ftrace_graph_caller+0x2c/0x30 21) 5016 64 ftrace_graph_caller+0x2c/0x30 22) 4952 32 ftrace_graph_caller+0x2c/0x30 23) 4920 64 ftrace_graph_caller+0x2c/0x30 ... Since, with function_graph tracer, we modify LR register in a stack frame when we enter into a function, we have to manage such special cases in save_stack_trace() or check_stack() as x86 does in print_ftrace_graph_addr(). Thanks, -Takahiro AKASHI > (We'd better mention it explicitly in the commmit?) > > Thanks, > -Takahiro AKASHI > > > On 07/21/2015 01:20 AM, Will Deacon wrote: >> Hi all, >> >> On Fri, Jul 17, 2015 at 04:34:21PM +0100, Jungseok Lee wrote: >>> On Jul 17, 2015, at 11:59 PM, Jungseok Lee wrote: >>>> On Jul 17, 2015, at 11:41 PM, Steven Rostedt wrote: >>>>> Thanks! Can you repost patch 1 with the changes I recommended, so that >>>>> I can get an Acked-by from the arm64 maintainers and pull all the >>>>> changes in together. This is fine for a 4.3 release, right? That is, it >>>>> doesn't need to go into 4.2-rcs. >>>>> >>>> >>>> It's not hard to repost a patch, but I feel like we have to wait for Akashi's response. >>>> Also, it might be needed to consider Mark's comment on arch part. >>>> >>>> If they are okay, I will proceed. >>> >>> The [RFC 1/3] patch used in my environment is shaped as follows. >>> I leave the hunk for *only* clear synchronization. This is why I choose this format >>> instead of reposting a patch. I hope it would help to track down this thread. >>> >>> I think this is my best at this point. >>> >>> ----8<---- >>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h >>> index c5534fa..2b43e20 100644 >>> --- a/arch/arm64/include/asm/ftrace.h >>> +++ b/arch/arm64/include/asm/ftrace.h >>> @@ -13,8 +13,9 @@ >>> >>> #include <asm/insn.h> >>> >>> -#define MCOUNT_ADDR ((unsigned long)_mcount) >>> -#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE >>> +#define MCOUNT_ADDR ((unsigned long)_mcount) >>> +#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE >>> +#define FTRACE_STACK_FRAME_OFFSET AARCH64_INSN_SIZE >>> >>> #ifndef __ASSEMBLY__ >>> #include <linux/compat.h> >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index 407991b..9ab67af 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -20,6 +20,7 @@ >>> #include <linux/sched.h> >>> #include <linux/stacktrace.h> >>> >>> +#include <asm/insn.h> >>> #include <asm/stacktrace.h> >>> >>> /* >>> @@ -52,7 +53,7 @@ int notrace unwind_frame(struct stackframe *frame) >>> * -4 here because we care about the PC at time of bl, >>> * not where the return will go. >>> */ >>> - frame->pc = *(unsigned long *)(fp + 8) - 4; >>> + frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE; >>> >>> return 0; >>> } >> >> The arm64 bits look fine to me: >> >> Acked-by: Will Deacon <will.deacon@arm.com> >> >> Steve: feel free to take this along with the other ftrace changes. I don't >> anticipate any conflicts, but if anything crops up in -next we can sort >> it out then. >> >> Thanks! >> >> Will >>
WARNING: multiple messages have this Message-ID (diff)
From: takahiro.akashi@linaro.org (AKASHI Takahiro) To: linux-arm-kernel@lists.infradead.org Subject: [RFC 2/3] arm64: refactor save_stack_trace() Date: Tue, 21 Jul 2015 19:26:39 +0900 [thread overview] Message-ID: <55AE1E5F.6060407@linaro.org> (raw) In-Reply-To: <55AD89FE.1010706@linaro.org> On 07/21/2015 08:53 AM, AKASHI Takahiro wrote: > Hi > > So i don't have to repost my patch here. Please use the original > commit log message[1/3] as is. > But please keep in mind that there is still an issue that I mentioned > in the cover letter; 'Size' field is inaccurate. > <reported size> = <its own dynamic local variables> > + <child's local variables> > and > <real size> = <reported size> + <its local variables> > - <child's local variables> > where "dynamic" means, for example, a variable allocated like the below: > int foo(int num) { > int array[num]; > ... > } > (See more details in my ascii art.) > > Such usage is seldom seen in the kernel, and <reported size> is > likely equal to <child's local variables>. In other words, we will > see one-line *displacement* in most cases. Well, I have a quick fix now, but it looks ugly. In addition, I found another issue; With function_graph tracer, the output is like: # cat /sys/kernel/tracing/stack_trace Depth Size Location (78 entries) ----- ---- -------- 0) 6184 32 update_min_vruntime+0x14/0x74 1) 6152 48 update_curr+0x6c/0x150 2) 6104 128 enqueue_task_fair+0x2f4/0xb9c 3) 5976 48 enqueue_task+0x48/0x90 ... 18) 5160 112 hrtimer_interrupt+0xa0/0x214 19) 5048 32 arch_timer_handler_phys+0x38/0x48 20) 5016 0 ftrace_graph_caller+0x2c/0x30 21) 5016 64 ftrace_graph_caller+0x2c/0x30 22) 4952 32 ftrace_graph_caller+0x2c/0x30 23) 4920 64 ftrace_graph_caller+0x2c/0x30 ... Since, with function_graph tracer, we modify LR register in a stack frame when we enter into a function, we have to manage such special cases in save_stack_trace() or check_stack() as x86 does in print_ftrace_graph_addr(). Thanks, -Takahiro AKASHI > (We'd better mention it explicitly in the commmit?) > > Thanks, > -Takahiro AKASHI > > > On 07/21/2015 01:20 AM, Will Deacon wrote: >> Hi all, >> >> On Fri, Jul 17, 2015 at 04:34:21PM +0100, Jungseok Lee wrote: >>> On Jul 17, 2015, at 11:59 PM, Jungseok Lee wrote: >>>> On Jul 17, 2015, at 11:41 PM, Steven Rostedt wrote: >>>>> Thanks! Can you repost patch 1 with the changes I recommended, so that >>>>> I can get an Acked-by from the arm64 maintainers and pull all the >>>>> changes in together. This is fine for a 4.3 release, right? That is, it >>>>> doesn't need to go into 4.2-rcs. >>>>> >>>> >>>> It's not hard to repost a patch, but I feel like we have to wait for Akashi's response. >>>> Also, it might be needed to consider Mark's comment on arch part. >>>> >>>> If they are okay, I will proceed. >>> >>> The [RFC 1/3] patch used in my environment is shaped as follows. >>> I leave the hunk for *only* clear synchronization. This is why I choose this format >>> instead of reposting a patch. I hope it would help to track down this thread. >>> >>> I think this is my best at this point. >>> >>> ----8<---- >>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h >>> index c5534fa..2b43e20 100644 >>> --- a/arch/arm64/include/asm/ftrace.h >>> +++ b/arch/arm64/include/asm/ftrace.h >>> @@ -13,8 +13,9 @@ >>> >>> #include <asm/insn.h> >>> >>> -#define MCOUNT_ADDR ((unsigned long)_mcount) >>> -#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE >>> +#define MCOUNT_ADDR ((unsigned long)_mcount) >>> +#define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE >>> +#define FTRACE_STACK_FRAME_OFFSET AARCH64_INSN_SIZE >>> >>> #ifndef __ASSEMBLY__ >>> #include <linux/compat.h> >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index 407991b..9ab67af 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -20,6 +20,7 @@ >>> #include <linux/sched.h> >>> #include <linux/stacktrace.h> >>> >>> +#include <asm/insn.h> >>> #include <asm/stacktrace.h> >>> >>> /* >>> @@ -52,7 +53,7 @@ int notrace unwind_frame(struct stackframe *frame) >>> * -4 here because we care about the PC at time of bl, >>> * not where the return will go. >>> */ >>> - frame->pc = *(unsigned long *)(fp + 8) - 4; >>> + frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE; >>> >>> return 0; >>> } >> >> The arm64 bits look fine to me: >> >> Acked-by: Will Deacon <will.deacon@arm.com> >> >> Steve: feel free to take this along with the other ftrace changes. I don't >> anticipate any conflicts, but if anything crops up in -next we can sort >> it out then. >> >> Thanks! >> >> Will >>
next prev parent reply other threads:[~2015-07-21 10:26 UTC|newest] Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-07-13 5:29 [RFC 0/3] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro 2015-07-13 5:29 ` AKASHI Takahiro 2015-07-13 5:29 ` [RFC 1/3] ftrace: adjust a function's pc to search for in check_stack() for arm64 AKASHI Takahiro 2015-07-13 5:29 ` AKASHI Takahiro 2015-07-13 15:24 ` Steven Rostedt 2015-07-13 15:24 ` Steven Rostedt 2015-07-15 0:22 ` AKASHI Takahiro 2015-07-15 0:22 ` AKASHI Takahiro 2015-07-13 5:29 ` [RFC 2/3] arm64: refactor save_stack_trace() AKASHI Takahiro 2015-07-13 5:29 ` AKASHI Takahiro 2015-07-14 12:47 ` Jungseok Lee 2015-07-14 12:47 ` Jungseok Lee 2015-07-14 13:31 ` Steven Rostedt 2015-07-14 13:31 ` Steven Rostedt 2015-07-15 0:20 ` AKASHI Takahiro 2015-07-15 0:20 ` AKASHI Takahiro 2015-07-15 2:51 ` Steven Rostedt 2015-07-15 2:51 ` Steven Rostedt 2015-07-15 11:41 ` AKASHI Takahiro 2015-07-15 11:41 ` AKASHI Takahiro 2015-07-15 14:55 ` Steven Rostedt 2015-07-15 14:55 ` Steven Rostedt 2015-07-15 16:13 ` Steven Rostedt 2015-07-15 16:13 ` Steven Rostedt 2015-07-16 0:27 ` AKASHI Takahiro 2015-07-16 0:27 ` AKASHI Takahiro 2015-07-16 1:08 ` AKASHI Takahiro 2015-07-16 1:08 ` AKASHI Takahiro 2015-07-16 1:38 ` Steven Rostedt 2015-07-16 1:38 ` Steven Rostedt 2015-07-17 10:46 ` Will Deacon 2015-07-17 10:46 ` Will Deacon 2015-07-16 13:29 ` Jungseok Lee 2015-07-16 13:29 ` Jungseok Lee 2015-07-16 13:54 ` Jungseok Lee 2015-07-16 13:54 ` Jungseok Lee 2015-07-16 14:24 ` Steven Rostedt 2015-07-16 14:24 ` Steven Rostedt 2015-07-16 15:01 ` Jungseok Lee 2015-07-16 15:01 ` Jungseok Lee 2015-07-16 15:31 ` Steven Rostedt 2015-07-16 15:31 ` Steven Rostedt 2015-07-16 15:52 ` Jungseok Lee 2015-07-16 15:52 ` Jungseok Lee 2015-07-16 20:22 ` Steven Rostedt 2015-07-16 20:22 ` Steven Rostedt 2015-07-17 2:49 ` AKASHI Takahiro 2015-07-17 2:49 ` AKASHI Takahiro 2015-07-17 3:21 ` Steven Rostedt 2015-07-17 3:21 ` Steven Rostedt 2015-07-16 16:16 ` Steven Rostedt 2015-07-16 16:16 ` Steven Rostedt 2015-07-17 12:40 ` Mark Rutland 2015-07-17 12:40 ` Mark Rutland 2015-07-17 12:51 ` Steven Rostedt 2015-07-17 12:51 ` Steven Rostedt 2015-07-17 13:00 ` Steven Rostedt 2015-07-17 13:00 ` Steven Rostedt 2015-07-17 14:28 ` Jungseok Lee 2015-07-17 14:28 ` Jungseok Lee 2015-07-17 14:41 ` Steven Rostedt 2015-07-17 14:41 ` Steven Rostedt 2015-07-17 14:59 ` Jungseok Lee 2015-07-17 14:59 ` Jungseok Lee 2015-07-17 15:34 ` Jungseok Lee 2015-07-17 15:34 ` Jungseok Lee 2015-07-17 16:01 ` Steven Rostedt 2015-07-17 16:01 ` Steven Rostedt 2015-07-20 16:20 ` Will Deacon 2015-07-20 16:20 ` Will Deacon 2015-07-20 23:53 ` AKASHI Takahiro 2015-07-20 23:53 ` AKASHI Takahiro 2015-07-21 10:26 ` AKASHI Takahiro [this message] 2015-07-21 10:26 ` AKASHI Takahiro 2015-07-21 14:34 ` Jungseok Lee 2015-07-21 14:34 ` Jungseok Lee 2015-08-03 9:09 ` Will Deacon 2015-08-03 9:09 ` Will Deacon 2015-08-03 14:01 ` Steven Rostedt 2015-08-03 14:01 ` Steven Rostedt 2015-08-03 14:04 ` Will Deacon 2015-08-03 14:04 ` Will Deacon 2015-08-03 16:30 ` Jungseok Lee 2015-08-03 16:30 ` Jungseok Lee 2015-08-03 16:57 ` Steven Rostedt 2015-08-03 16:57 ` Steven Rostedt 2015-08-03 17:22 ` Jungseok Lee 2015-08-03 17:22 ` Jungseok Lee 2015-08-03 17:32 ` Steven Rostedt 2015-08-03 17:32 ` Steven Rostedt 2015-08-04 7:41 ` AKASHI Takahiro 2015-08-04 7:41 ` AKASHI Takahiro 2015-07-17 2:04 ` AKASHI Takahiro 2015-07-17 2:04 ` AKASHI Takahiro 2015-07-17 14:38 ` Jungseok Lee 2015-07-17 14:38 ` Jungseok Lee 2015-07-16 14:28 ` Mark Rutland 2015-07-16 14:28 ` Mark Rutland 2015-07-16 14:34 ` Steven Rostedt 2015-07-16 14:34 ` Steven Rostedt 2015-07-17 2:09 ` AKASHI Takahiro 2015-07-17 2:09 ` AKASHI Takahiro 2015-07-13 5:29 ` [RFC 3/3] arm64: ftrace: mcount() should not create a stack frame AKASHI Takahiro 2015-07-13 5:29 ` AKASHI Takahiro 2015-07-13 15:01 ` [RFC 0/3] arm64: ftrace: fix incorrect output from stack tracer Jungseok Lee 2015-07-13 15:01 ` Jungseok Lee
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=55AE1E5F.6060407@linaro.org \ --to=takahiro.akashi@linaro.org \ --cc=Catalin.Marinas@arm.com \ --cc=Mark.Rutland@arm.com \ --cc=broonie@kernel.org \ --cc=david.griego@linaro.org \ --cc=jungseoklee85@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=olof@lixom.net \ --cc=rostedt@goodmis.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.