* [PATCH] Function stack size and its name mismatch in arm64
@ 2019-07-31 9:04 Jiping Ma
2019-07-31 10:57 ` Steven Rostedt
2019-07-31 14:46 ` Mark Rutland
0 siblings, 2 replies; 4+ messages in thread
From: Jiping Ma @ 2019-07-31 9:04 UTC (permalink / raw)
To: rostedt, mingo; +Cc: linux-kernel, jiping.ma2
The PC of one the frame is matched to the next frame function, rather
than the function of his frame.
Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
---
kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..ed80b95abf06 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -40,16 +40,28 @@ static void print_max_stack(void)
pr_emerg(" Depth Size Location (%d entries)\n"
" ----- ---- --------\n",
+#ifdef CONFIG_ARM64
+ stack_trace_nr_entries - 1);
+#else
stack_trace_nr_entries);
-
+#endif
+#ifdef CONFIG_ARM64
+ for (i = 1; i < stack_trace_nr_entries; i++) {
+#else
for (i = 0; i < stack_trace_nr_entries; i++) {
+#endif
if (i + 1 == stack_trace_nr_entries)
size = stack_trace_index[i];
else
size = stack_trace_index[i] - stack_trace_index[i+1];
+#ifdef CONFIG_ARM64
+ pr_emerg("%3ld) %8d %5d %pS\n", i-1, stack_trace_index[i],
+ size, (void *)stack_dump_trace[i-1]);
+#else
pr_emerg("%3ld) %8d %5d %pS\n", i, stack_trace_index[i],
size, (void *)stack_dump_trace[i]);
+#endif
}
}
@@ -324,8 +336,11 @@ static int t_show(struct seq_file *m, void *v)
seq_printf(m, " Depth Size Location"
" (%d entries)\n"
" ----- ---- --------\n",
+#ifdef CONFIG_ARM64
+ stack_trace_nr_entries - 1);
+#else
stack_trace_nr_entries);
-
+#endif
if (!stack_tracer_enabled && !stack_trace_max_size)
print_disabled(m);
@@ -334,6 +349,10 @@ static int t_show(struct seq_file *m, void *v)
i = *(long *)v;
+#ifdef CONFIG_ARM64
+ if (i == 0)
+ return 0;
+#endif
if (i >= stack_trace_nr_entries)
return 0;
@@ -342,9 +361,14 @@ static int t_show(struct seq_file *m, void *v)
else
size = stack_trace_index[i] - stack_trace_index[i+1];
+#ifdef CONFIG_ARM64
+ seq_printf(m, "%3ld) %8d %5d ", i-1, stack_trace_index[i], size);
+ trace_lookup_stack(m, i-1);
+#else
seq_printf(m, "%3ld) %8d %5d ", i, stack_trace_index[i], size);
trace_lookup_stack(m, i);
+#endif
return 0;
}
--
2.18.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Function stack size and its name mismatch in arm64
2019-07-31 9:04 [PATCH] Function stack size and its name mismatch in arm64 Jiping Ma
@ 2019-07-31 10:57 ` Steven Rostedt
2019-07-31 13:56 ` James Morse
2019-07-31 14:46 ` Mark Rutland
1 sibling, 1 reply; 4+ messages in thread
From: Steven Rostedt @ 2019-07-31 10:57 UTC (permalink / raw)
To: Jiping Ma; +Cc: mingo, linux-kernel
On Wed, 31 Jul 2019 17:04:37 +0800
Jiping Ma <jiping.ma2@windriver.com> wrote:
Hi Jiping,
Note, the subject is not properly written, as it is missing the
subsystem. In this case, it should start with "tracing: "
> The PC of one the frame is matched to the next frame function, rather
> than the function of his frame.
The above change log doesn't make sense. I have no idea what the actual
problem is here. Why is this different for arm64 and no one else? Seems
the bug is with the stack logic code in arm64 not here.
>
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> ---
> kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 5d16f73898db..ed80b95abf06 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -40,16 +40,28 @@ static void print_max_stack(void)
>
> pr_emerg(" Depth Size Location (%d entries)\n"
> " ----- ---- --------\n",
> +#ifdef CONFIG_ARM64
We do not allow arch specific defines in generic code. Otherwise this
would blow up and become unmaintainable. Not to mention it makes the
code ugly and hard to follow.
Please explain the problem better. I'm sure there's much better ways to
solve this than this patch.
Thanks,
-- Steve
> + stack_trace_nr_entries - 1);
> +#else
> stack_trace_nr_entries);
> -
> +#endif
> +#ifdef CONFIG_ARM64
> + for (i = 1; i < stack_trace_nr_entries; i++) {
> +#else
> for (i = 0; i < stack_trace_nr_entries; i++) {
> +#endif
> if (i + 1 == stack_trace_nr_entries)
> size = stack_trace_index[i];
> else
> size = stack_trace_index[i] - stack_trace_index[i+1];
>
> +#ifdef CONFIG_ARM64
> + pr_emerg("%3ld) %8d %5d %pS\n", i-1, stack_trace_index[i],
> + size, (void *)stack_dump_trace[i-1]);
> +#else
> pr_emerg("%3ld) %8d %5d %pS\n", i, stack_trace_index[i],
> size, (void *)stack_dump_trace[i]);
> +#endif
> }
> }
>
> @@ -324,8 +336,11 @@ static int t_show(struct seq_file *m, void *v)
> seq_printf(m, " Depth Size Location"
> " (%d entries)\n"
> " ----- ---- --------\n",
> +#ifdef CONFIG_ARM64
> + stack_trace_nr_entries - 1);
> +#else
> stack_trace_nr_entries);
> -
> +#endif
> if (!stack_tracer_enabled && !stack_trace_max_size)
> print_disabled(m);
>
> @@ -334,6 +349,10 @@ static int t_show(struct seq_file *m, void *v)
>
> i = *(long *)v;
>
> +#ifdef CONFIG_ARM64
> + if (i == 0)
> + return 0;
> +#endif
> if (i >= stack_trace_nr_entries)
> return 0;
>
> @@ -342,9 +361,14 @@ static int t_show(struct seq_file *m, void *v)
> else
> size = stack_trace_index[i] - stack_trace_index[i+1];
>
> +#ifdef CONFIG_ARM64
> + seq_printf(m, "%3ld) %8d %5d ", i-1, stack_trace_index[i], size);
> + trace_lookup_stack(m, i-1);
> +#else
> seq_printf(m, "%3ld) %8d %5d ", i, stack_trace_index[i], size);
>
> trace_lookup_stack(m, i);
> +#endif
>
> return 0;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Function stack size and its name mismatch in arm64
2019-07-31 10:57 ` Steven Rostedt
@ 2019-07-31 13:56 ` James Morse
0 siblings, 0 replies; 4+ messages in thread
From: James Morse @ 2019-07-31 13:56 UTC (permalink / raw)
To: Jiping Ma; +Cc: Steven Rostedt, mingo, linux-kernel, linux-arm-kernel
Hi Jiping,
(CC: +linux-arm-kernel)
On 31/07/2019 11:57, Steven Rostedt wrote:
> On Wed, 31 Jul 2019 17:04:37 +0800
> Jiping Ma <jiping.ma2@windriver.com> wrote:
> Note, the subject is not properly written, as it is missing the
> subsystem. In this case, it should start with "tracing: "
>
>
>> The PC of one the frame is matched to the next frame function, rather
>> than the function of his frame.
>
> The above change log doesn't make sense. I have no idea what the actual
> problem is here. Why is this different for arm64 and no one else? Seems
> the bug is with the stack logic code in arm64 not here.
Please copy the mailing list for the arm64 arch code too.
Is this thing a recent change? arm64's stacktrace code gained some better protection for
loops at -rc2.
Thanks,
James
>> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
>> index 5d16f73898db..ed80b95abf06 100644
>> --- a/kernel/trace/trace_stack.c
>> +++ b/kernel/trace/trace_stack.c
>> @@ -40,16 +40,28 @@ static void print_max_stack(void)
>>
>> pr_emerg(" Depth Size Location (%d entries)\n"
>> " ----- ---- --------\n",
>> +#ifdef CONFIG_ARM64
>
> We do not allow arch specific defines in generic code. Otherwise this
> would blow up and become unmaintainable. Not to mention it makes the
> code ugly and hard to follow.
>
> Please explain the problem better. I'm sure there's much better ways to
> solve this than this patch.
>
> Thanks,
>
> -- Steve
>
>
>
>> + stack_trace_nr_entries - 1);
>> +#else
>> stack_trace_nr_entries);
>> -
>> +#endif
>> +#ifdef CONFIG_ARM64
>> + for (i = 1; i < stack_trace_nr_entries; i++) {
>> +#else
>> for (i = 0; i < stack_trace_nr_entries; i++) {
>> +#endif
>> if (i + 1 == stack_trace_nr_entries)
>> size = stack_trace_index[i];
>> else
>> size = stack_trace_index[i] - stack_trace_index[i+1];
>>
>> +#ifdef CONFIG_ARM64
>> + pr_emerg("%3ld) %8d %5d %pS\n", i-1, stack_trace_index[i],
>> + size, (void *)stack_dump_trace[i-1]);
>> +#else
>> pr_emerg("%3ld) %8d %5d %pS\n", i, stack_trace_index[i],
>> size, (void *)stack_dump_trace[i]);
>> +#endif
>> }
>> }
>>
>> @@ -324,8 +336,11 @@ static int t_show(struct seq_file *m, void *v)
>> seq_printf(m, " Depth Size Location"
>> " (%d entries)\n"
>> " ----- ---- --------\n",
>> +#ifdef CONFIG_ARM64
>> + stack_trace_nr_entries - 1);
>> +#else
>> stack_trace_nr_entries);
>> -
>> +#endif
>> if (!stack_tracer_enabled && !stack_trace_max_size)
>> print_disabled(m);
>>
>> @@ -334,6 +349,10 @@ static int t_show(struct seq_file *m, void *v)
>>
>> i = *(long *)v;
>>
>> +#ifdef CONFIG_ARM64
>> + if (i == 0)
>> + return 0;
>> +#endif
>> if (i >= stack_trace_nr_entries)
>> return 0;
>>
>> @@ -342,9 +361,14 @@ static int t_show(struct seq_file *m, void *v)
>> else
>> size = stack_trace_index[i] - stack_trace_index[i+1];
>>
>> +#ifdef CONFIG_ARM64
>> + seq_printf(m, "%3ld) %8d %5d ", i-1, stack_trace_index[i], size);
>> + trace_lookup_stack(m, i-1);
>> +#else
>> seq_printf(m, "%3ld) %8d %5d ", i, stack_trace_index[i], size);
>>
>> trace_lookup_stack(m, i);
>> +#endif
>>
>> return 0;
>> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Function stack size and its name mismatch in arm64
2019-07-31 9:04 [PATCH] Function stack size and its name mismatch in arm64 Jiping Ma
2019-07-31 10:57 ` Steven Rostedt
@ 2019-07-31 14:46 ` Mark Rutland
1 sibling, 0 replies; 4+ messages in thread
From: Mark Rutland @ 2019-07-31 14:46 UTC (permalink / raw)
To: Jiping Ma
Cc: rostedt, mingo, linux-kernel, linux-arm-kernel, will.deacon,
catalin.marinas
Hi,
If you have a patch affecting arm64, please Cc LAKML and the arm64
maintainers. I've added them to this sub-thread.
On Wed, Jul 31, 2019 at 05:04:37PM +0800, Jiping Ma wrote:
> The PC of one the frame is matched to the next frame function, rather
> than the function of his frame.
As Steve said in another reply, please could you explain the problem
more thoroughly? An example would be very helpful.
It sounds like arm64 behaves differently to other architectures here,
which could be an arm64-specific bug, or it could be that the behaviour
is inconsistent across archtiectures and needs more general cleanup.
Thanks,
Mark.
>
> Signed-off-by: Jiping Ma <jiping.ma2@windriver.com>
> ---
> kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 5d16f73898db..ed80b95abf06 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -40,16 +40,28 @@ static void print_max_stack(void)
>
> pr_emerg(" Depth Size Location (%d entries)\n"
> " ----- ---- --------\n",
> +#ifdef CONFIG_ARM64
> + stack_trace_nr_entries - 1);
> +#else
> stack_trace_nr_entries);
> -
> +#endif
> +#ifdef CONFIG_ARM64
> + for (i = 1; i < stack_trace_nr_entries; i++) {
> +#else
> for (i = 0; i < stack_trace_nr_entries; i++) {
> +#endif
> if (i + 1 == stack_trace_nr_entries)
> size = stack_trace_index[i];
> else
> size = stack_trace_index[i] - stack_trace_index[i+1];
>
> +#ifdef CONFIG_ARM64
> + pr_emerg("%3ld) %8d %5d %pS\n", i-1, stack_trace_index[i],
> + size, (void *)stack_dump_trace[i-1]);
> +#else
> pr_emerg("%3ld) %8d %5d %pS\n", i, stack_trace_index[i],
> size, (void *)stack_dump_trace[i]);
> +#endif
> }
> }
>
> @@ -324,8 +336,11 @@ static int t_show(struct seq_file *m, void *v)
> seq_printf(m, " Depth Size Location"
> " (%d entries)\n"
> " ----- ---- --------\n",
> +#ifdef CONFIG_ARM64
> + stack_trace_nr_entries - 1);
> +#else
> stack_trace_nr_entries);
> -
> +#endif
> if (!stack_tracer_enabled && !stack_trace_max_size)
> print_disabled(m);
>
> @@ -334,6 +349,10 @@ static int t_show(struct seq_file *m, void *v)
>
> i = *(long *)v;
>
> +#ifdef CONFIG_ARM64
> + if (i == 0)
> + return 0;
> +#endif
> if (i >= stack_trace_nr_entries)
> return 0;
>
> @@ -342,9 +361,14 @@ static int t_show(struct seq_file *m, void *v)
> else
> size = stack_trace_index[i] - stack_trace_index[i+1];
>
> +#ifdef CONFIG_ARM64
> + seq_printf(m, "%3ld) %8d %5d ", i-1, stack_trace_index[i], size);
> + trace_lookup_stack(m, i-1);
> +#else
> seq_printf(m, "%3ld) %8d %5d ", i, stack_trace_index[i], size);
>
> trace_lookup_stack(m, i);
> +#endif
>
> return 0;
> }
> --
> 2.18.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-31 14:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 9:04 [PATCH] Function stack size and its name mismatch in arm64 Jiping Ma
2019-07-31 10:57 ` Steven Rostedt
2019-07-31 13:56 ` James Morse
2019-07-31 14:46 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®