Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1182526 > unrolled thread
| Started by | AKASHI Takahiro <takahiro.akashi@linaro.org> |
|---|---|
| First post | 2015-07-13 07:40 +0200 |
| Last post | 2015-07-17 16:40 +0200 |
| Articles | 4 on this page of 24 — 5 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[RFC 2/3] arm64: refactor save_stack_trace() AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-07-13 07:40 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-07-15 02:30 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-15 05:00 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-07-15 13:50 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-15 17:00 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-07-16 02:30 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-07-16 03:10 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-16 03:40 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Will Deacon <will.deacon@arm.com> - 2015-07-17 12:50 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-16 16:30 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Jungseok Lee <jungseoklee85@gmail.com> - 2015-07-16 17:10 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-16 17:40 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Jungseok Lee <jungseoklee85@gmail.com> - 2015-07-16 18:00 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-16 22:30 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-16 18:20 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Mark Rutland <mark.rutland@arm.com> - 2015-07-17 14:50 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-17 15:00 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-17 15:10 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Jungseok Lee <jungseoklee85@gmail.com> - 2015-07-17 16:30 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-17 16:50 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Jungseok Lee <jungseoklee85@gmail.com> - 2015-07-17 17:00 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Mark Rutland <mark.rutland@arm.com> - 2015-07-16 16:30 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Steven Rostedt <rostedt@goodmis.org> - 2015-07-16 16:40 +0200
Re: [RFC 2/3] arm64: refactor save_stack_trace() Jungseok Lee <jungseoklee85@gmail.com> - 2015-07-17 16:40 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Jungseok Lee <jungseoklee85@gmail.com> |
|---|---|
| Date | 2015-07-17 17:00 +0200 |
| Message-ID | <pNfhg-8D-23@gated-at.bofh.it> |
| In reply to | #1186882 |
On Jul 17, 2015, at 11:41 PM, Steven Rostedt wrote: > On Fri, 17 Jul 2015 23:28:13 +0900 > Jungseok Lee <jungseoklee85@gmail.com> wrote: > > >> >> I have reviewed and tested the kernel including this patch and only [RFC 1/3]. > > 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. > Welcome! 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. >> >> Now, the number of entries and max_stack_size are always okay, but unexpected functions, >> such as ftrace_ops_no_ops and ftrace_call, are *sometimes* listed as follows. >> >> $ cat /sys/kernel/debug/tracing/stack_trace >> >> Depth Size Location (49 entries) >> ----- ---- -------- >> 0) 4456 16 arch_counter_read+0xc/0x24 >> 1) 4440 16 ktime_get+0x44/0xb4 >> 2) 4424 48 get_drm_timestamp+0x30/0x40 >> 3) 4376 16 drm_get_last_vbltimestamp+0x94/0xb4 >> 4) 4360 80 drm_handle_vblank+0x84/0x3c0 >> 5) 4280 144 mdp5_irq+0x118/0x130 >> 6) 4136 80 msm_irq+0x2c/0x68 >> 7) 4056 32 handle_irq_event_percpu+0x60/0x210 >> 8) 4024 96 handle_irq_event+0x50/0x80 >> 9) 3928 64 handle_fasteoi_irq+0xb0/0x178 >> 10) 3864 48 generic_handle_irq+0x38/0x54 >> 11) 3816 32 __handle_domain_irq+0x68/0xbc >> 12) 3784 64 gic_handle_irq+0x38/0x88 >> 13) 3720 280 el1_irq+0x64/0xd8 >> 14) 3440 168 ftrace_ops_no_ops+0xb4/0x16c >> 15) 3272 64 ftrace_call+0x0/0x4 >> 16) 3208 16 _raw_spin_lock_irqsave+0x14/0x70 >> 17) 3192 32 msm_gpio_set+0x44/0xb4 >> 18) 3160 48 _gpiod_set_raw_value+0x68/0x148 >> 19) 3112 64 gpiod_set_value+0x40/0x70 >> 20) 3048 32 gpio_led_set+0x3c/0x94 >> 21) 3016 48 led_set_brightness+0x50/0xa4 >> 22) 2968 32 led_trigger_event+0x4c/0x78 >> 23) 2936 48 mmc_request_done+0x38/0x84 >> 24) 2888 32 sdhci_tasklet_finish+0xcc/0x12c >> 25) 2856 48 tasklet_action+0x64/0x120 >> 26) 2808 48 __do_softirq+0x114/0x2f0 >> 27) 2760 128 irq_exit+0x98/0xd8 >> 28) 2632 32 __handle_domain_irq+0x6c/0xbc >> 29) 2600 64 gic_handle_irq+0x38/0x88 >> 30) 2536 280 el1_irq+0x64/0xd8 >> 31) 2256 168 ftrace_ops_no_ops+0xb4/0x16c >> 32) 2088 64 ftrace_call+0x0/0x4 > > Like I stated before, the above looks to be an interrupt coming in > while the tracing was happening. This looks legitimate to me. I'm > guessing that arm64 uses one stack for normal context and interrupt > context, where as x86 uses a separate stack for interrupt context. Okay. Thanks for a kind explanation. Best Regards Jungseok Lee-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2015-07-16 16:30 +0200 |
| Message-ID | <pMSkG-Sa-35@gated-at.bofh.it> |
| In reply to | #1185167 |
On Thu, Jul 16, 2015 at 02:08:03AM +0100, AKASHI Takahiro wrote:
> On 07/16/2015 09:27 AM, AKASHI Takahiro wrote:
> > On 07/16/2015 01:13 AM, Steven Rostedt wrote:
> >> On Wed, 15 Jul 2015 10:55:36 -0400
> >> Steven Rostedt <rostedt@goodmis.org> wrote:
> >>
> >>
> >>> I'll take a look at it and try to clean up the code.
> >>
> >> Does the following patch make sense for you?
> >
> > Looks nice. The patch greatly simplifies changes on arm64 side.
>
> As follows:
>
> - Takahiro AKASHI
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index c5534fa..868d6f1 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -15,6 +15,7 @@
>
> #define MCOUNT_ADDR ((unsigned long)_mcount)
> #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
> +#define FTRACE_STACK_FRAME_OFFSET 4 /* sync it up with stacktrace.c */
Is there any reason we couldn't have the arch code dump the stack depth
for each function when it walks the stack to generate the stack trace?
That means we can provide a more precise result (because we know the
layour of our own stackframes), and we only need walk the stack once to
do so.
The downside is that we need a new function per-arch to do so.
Mark.
>
> #ifndef __ASSEMBLY__
> #include <linux/compat.h>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1da6029..2c1bf7d 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -260,6 +260,13 @@ static inline void ftrace_kill(void) { }
> #endif /* CONFIG_FUNCTION_TRACER */
>
> #ifdef CONFIG_STACK_TRACER
> +/*
> + * the offset value to add to return address from save_stack_trace()
> + */
> +#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 9384647..c5b9748 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;
> }
>
> @@ -131,7 +131,8 @@ check_stack(unsigned long ip, unsigned long *stack)
> p = start;
>
> for (; p < top && i < max_stack_trace.nr_entries; p++) {
> - 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);
> --
> 1.7.9.5
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2015-07-16 16:40 +0200 |
| Message-ID | <pMSum-13y-7@gated-at.bofh.it> |
| In reply to | #1185908 |
On Thu, 16 Jul 2015 15:28:34 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > > diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h > > index c5534fa..868d6f1 100644 > > --- a/arch/arm64/include/asm/ftrace.h > > +++ b/arch/arm64/include/asm/ftrace.h > > @@ -15,6 +15,7 @@ > > > > #define MCOUNT_ADDR ((unsigned long)_mcount) > > #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE > > +#define FTRACE_STACK_FRAME_OFFSET 4 /* sync it up with stacktrace.c */ > > Is there any reason we couldn't have the arch code dump the stack depth > for each function when it walks the stack to generate the stack trace? > > That means we can provide a more precise result (because we know the > layour of our own stackframes), and we only need walk the stack once to > do so. > > The downside is that we need a new function per-arch to do so. Or we make check_patch() a weak function, and let archs override it. I can possibly break up the code a bit to have helper functions where things are the same. I want x86 to be able to track irq stacks as well, but there's no way to do that generically yet, so having arch specific functions has been on my todo list. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jungseok Lee <jungseoklee85@gmail.com> |
|---|---|
| Date | 2015-07-17 16:40 +0200 |
| Message-ID | <pNeXU-8dA-5@gated-at.bofh.it> |
| In reply to | #1185167 |
On Jul 17, 2015, at 11:04 AM, AKASHI Takahiro wrote:
> Jungseok,
>
> Thank you for your testing and reviews.
You're welcome.
> On 07/16/2015 10:29 PM, Jungseok Lee wrote:
>> On Jul 16, 2015, at 10:08 AM, AKASHI Takahiro wrote:
>>
>> Hi, AKASHI
>>
>>> On 07/16/2015 09:27 AM, AKASHI Takahiro wrote:
>>>> On 07/16/2015 01:13 AM, Steven Rostedt wrote:
>>>>> On Wed, 15 Jul 2015 10:55:36 -0400
>>>>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>>>>
>>>>>
>>>>>> I'll take a look at it and try to clean up the code.
>>>>>
>>>>> Does the following patch make sense for you?
>>>>
>>>> Looks nice. The patch greatly simplifies changes on arm64 side.
>>>
>>> As follows:
>>>
>>> - Takahiro AKASHI
>>>
>>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>>> index c5534fa..868d6f1 100644
>>> --- a/arch/arm64/include/asm/ftrace.h
>>> +++ b/arch/arm64/include/asm/ftrace.h
>>> @@ -15,6 +15,7 @@
>>>
>>> #define MCOUNT_ADDR ((unsigned long)_mcount)
>>> #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
>>> +#define FTRACE_STACK_FRAME_OFFSET 4 /* sync it up with stacktrace.c */
>
> Well,
> #define FTRACE_STACK_FRAME_OFFSET AARCH64_INSN_SIZE
> might be better.
Agree.
>
>> How about binding it to -4 in unwind_frame function?
>
> Do you mean like this?
> In unwind_frame(),
> frame->pc = *(unsigned long*)(fp + 8) - AARCH64_INSN_SIZE;
Exactly.
>
>> IMHO, it would help other developers trying to change stack trace code
>> be aware of this stack tracer feature.
>>
>>> #ifndef __ASSEMBLY__
>>> #include <linux/compat.h>
>>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>>> index 1da6029..2c1bf7d 100644
>>> --- a/include/linux/ftrace.h
>>> +++ b/include/linux/ftrace.h
>>> @@ -260,6 +260,13 @@ static inline void ftrace_kill(void) { }
>>> #endif /* CONFIG_FUNCTION_TRACER */
>>>
>>> #ifdef CONFIG_STACK_TRACER
>>> +/*
>>> + * the offset value to add to return address from save_stack_trace()
>>> + */
>>> +#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 9384647..c5b9748 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;
>>> }
>>>
>>> @@ -131,7 +131,8 @@ check_stack(unsigned long ip, unsigned long *stack)
>>> p = start;
>>>
>>> for (; p < top && i < max_stack_trace.nr_entries; p++) {
>>> - 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);
>>> --
>>
>> I've prepared a kernel with the following patches and reviewed them.
>>
>> 1) Steve's clean up patch
>> 2) This patch
>> 3) [RFC 2/3]
>>
>> AFAIU, [RFC 3/3] is not needed any more thanks to Steve's patch.
>
> We don't need [2/3].
Okay. So, I've played a kernel including Steve's latest patch and only [1/3].
Best Regards
Jungseok Lee--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web