| Message ID | 1357EA74-B972-4B99-ADB0-BC7E8F06DDB5@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 18 Jul 2015 00:34:21 +0900 Jungseok Lee <jungseoklee85@gmail.com> wrote: > > 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. You could just add: Link: http://lkml.kernel.org/r/1436765375-7119-3-git-send-email-takahiro.akashi@linaro.org to your change log, and that will point to this thread. That's what the Link tag was made for. > > Steve, > > I think this is my best at this point. > OK, I'll just wait for the others to give their ack. -- Steve
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
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.
(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
>
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 >>
On Jul 21, 2015, at 7:26 PM, AKASHI Takahiro wrote: > 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. AFAIU, stack_max_size would be more accurate if a separate stack is introduced for interrupt context. However, it might be unnecessary at this point due to complexity. > 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(). I should have run it with function_graph. The issue is reproduced easily on my environment. I don't see other issues yet when enabling other tracers. Best Regards Jungseok Lee
Hi guys, On Mon, Jul 20, 2015 at 05:20:04PM +0100, Will Deacon wrote: > 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. [...] > The arm64 bits look fine to me: > > Acked-by: Will Deacon <will.deacon@arm.com> What happened to this? Is it queued someplace, or are we waiting for a new version? Will
On Mon, 3 Aug 2015 10:09:51 +0100 Will Deacon <will.deacon@arm.com> wrote: > > The arm64 bits look fine to me: > > > > Acked-by: Will Deacon <will.deacon@arm.com> > > What happened to this? Is it queued someplace, or are we waiting for a > new version? I went on vacation :-) I'm back and trying to catch up. I'll be pulling in a bunch of things today. Hopefully I can get to this. -- Steve
On Mon, Aug 03, 2015 at 03:01:40PM +0100, Steven Rostedt wrote: > On Mon, 3 Aug 2015 10:09:51 +0100 > Will Deacon <will.deacon@arm.com> wrote: > > > > > The arm64 bits look fine to me: > > > > > > Acked-by: Will Deacon <will.deacon@arm.com> > > > > What happened to this? Is it queued someplace, or are we waiting for a > > new version? > > I went on vacation :-) I'm back and trying to catch up. I'll be > pulling in a bunch of things today. Hopefully I can get to this. Cheers, Steve. Just wanted to make sure it didn't got lost (I'd briefly forgotten about it myself!). I hope you had a good holiday. Will
On Aug 3, 2015, at 6:09 PM, Will Deacon wrote: > Hi guys, Hi Will, > On Mon, Jul 20, 2015 at 05:20:04PM +0100, Will Deacon wrote: >> 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. > > [...] > >> The arm64 bits look fine to me: >> >> Acked-by: Will Deacon <will.deacon@arm.com> > > What happened to this? Is it queued someplace, or are we waiting for a > new version? There are two issues in the current version. 1) The change does not work correctly when function_graph feature is enabled. 2) Akashi have raised an issue that size field of stack tracer is inaccurate. So, I think this patch set is not ready yet. Best Regards Jungseok Lee
On Tue, 4 Aug 2015 01:30:50 +0900 Jungseok Lee <jungseoklee85@gmail.com> wrote: > There are two issues in the current version. > 1) The change does not work correctly when function_graph feature is enabled. > 2) Akashi have raised an issue that size field of stack tracer is inaccurate. > > So, I think this patch set is not ready yet. Do you still want me to add code that does: if (*p == (stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET)) { ? If you expect to need that, I can get it into the next merge window and you can base the code of that in the merge window after that. -- Steve
On Aug 4, 2015, at 1:57 AM, Steven Rostedt wrote: > On Tue, 4 Aug 2015 01:30:50 +0900 > Jungseok Lee <jungseoklee85@gmail.com> wrote: > > >> There are two issues in the current version. >> 1) The change does not work correctly when function_graph feature is enabled. >> 2) Akashi have raised an issue that size field of stack tracer is inaccurate. >> >> So, I think this patch set is not ready yet. > > Do you still want me to add code that does: > > if (*p == (stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET)) { > > ? > > If you expect to need that, I can get it into the next merge window and > you can base the code of that in the merge window after that. It would be better to add the snippet when a new version is ready. That way might help to figure out easily why the macro is introduced and how it can be used in architecture code. Best Regards Jungseok Lee
On Tue, 4 Aug 2015 02:22:16 +0900 Jungseok Lee <jungseoklee85@gmail.com> wrote: > It would be better to add the snippet when a new version is ready. > That way might help to figure out easily why the macro is introduced and how > it can be used in architecture code. OK, which ever. -- Steve
On 08/04/2015 02:32 AM, Steven Rostedt wrote: > On Tue, 4 Aug 2015 02:22:16 +0900 > Jungseok Lee <jungseoklee85@gmail.com> wrote: > > >> It would be better to add the snippet when a new version is ready. >> That way might help to figure out easily why the macro is introduced and how >> it can be used in architecture code. > > > OK, which ever. I'm making best efforts to fix the problem, but it seems to me that the current check_stack() doesn't fit to arm64 due to the differences in stack frame usage on architectures. Although my current patch doesn't solve all the issues I've noticed so far, I'd like to get any comments about whether I'm doing it the right way. Please take a look at my new vesion (v2) of RFC. Thanks, -Takahiro AKASHI > -- Steve >
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; } diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 1da6029..6566201 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -260,6 +260,9 @@ static inline void ftrace_kill(void) { } #endif /* CONFIG_FUNCTION_TRACER */ #ifdef CONFIG_STACK_TRACER +#ifndef FTRACE_STACK_FRAME_OFFSET +#define FTRACE_STACK_FRAME_OFFSET 0 +#endif extern int stack_tracer_enabled; int stack_trace_sysctl(struct ctl_table *table, int write, diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c index b746399..30521ea 100644 --- a/kernel/trace/trace_stack.c +++ b/kernel/trace/trace_stack.c @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack) /* Skip over the overhead of the stack tracer itself */ for (i = 0; i < max_stack_trace.nr_entries; i++) { - if (stack_dump_trace[i] == ip) + if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip) break; } @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack) for (; p < top && i < max_stack_trace.nr_entries; p++) { if (stack_dump_trace[i] == ULONG_MAX) break; - if (*p == stack_dump_trace[i]) { + if (*p == (stack_dump_trace[i] + + FTRACE_STACK_FRAME_OFFSET)) { stack_dump_trace[x] = stack_dump_trace[i++]; this_size = stack_dump_index[x++] = (top - p) * sizeof(unsigned long);