linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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;
as well as URLs for NNTP newsgroup(s).