Re: [RFC patch 16/41] tracing: Remove the ULONG_MAX stack trace hackery
On Wed, 10 Apr 2019 21:34:25 -0500
Josh Poimboeuf wrote:
> > --- a/kernel/trace/trace_stack.c
> > +++ b/kernel/trace/trace_stack.c
> > @@ -18,8 +18,7 @@
> >
> > #include "trace.h"
> >
> > -static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES+1] =
> > -{ [0 ... (STACK_TRACE_ENTRIES)] = ULONG_MAX };
> > +static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES + 1];
>
> Is the "+ 1" still needed? AFAICT, accesses to this array never go past
> nr_entries.
Probably not. But see this for an explanation:
http://lkml.kernel.org/r/20180620110758.crunhd5bfep7zuiz@kili.mountain
>
> Also I've been staring at the code but I can't figure out why
> max_entries is "- 1".
>
> struct stack_trace stack_trace_max = {
> .max_entries= STACK_TRACE_ENTRIES - 1,
> .entries= _dump_trace[0],
> };
>
Well, it had a reason in the past, but there doesn't seem to be a
reason today. Looking at git history, that code was originally:
.max_entries= STACK_TRACE_ENTRIES - 1,
.entries= _dump_trace[1],
Where we had to make max_entries -1 as we started at the first index
into the array.
I'll have to take a new look into this code. After Thomas's clean up
here, I'm sure we can simplify it a bit more.
-- Steve
Re: [RFC patch 16/41] tracing: Remove the ULONG_MAX stack trace hackery
On Wed, Apr 10, 2019 at 12:28:10PM +0200, Thomas Gleixner wrote:
> No architecture terminates the stack trace with ULONG_MAX anymore. As the
> code checks the number of entries stored anyway there is no point in
> keeping all that ULONG_MAX magic around.
>
> The histogram code zeroes the storage before saving the stack, so if the
> trace is shorter than the maximum number of entries it can terminate the
> print loop if a zero entry is detected.
>
> Signed-off-by: Thomas Gleixner
> Cc: Steven Rostedt
> ---
> kernel/trace/trace_events_hist.c |2 +-
> kernel/trace/trace_stack.c | 20 +---
> 2 files changed, 6 insertions(+), 16 deletions(-)
>
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5246,7 +5246,7 @@ static void hist_trigger_stacktrace_prin
> unsigned int i;
>
> for (i = 0; i < max_entries; i++) {
> - if (stacktrace_entries[i] == ULONG_MAX)
> + if (!stacktrace_entries[i])
> return;
>
> seq_printf(m, "%*c", 1 + spaces, ' ');
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -18,8 +18,7 @@
>
> #include "trace.h"
>
> -static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES+1] =
> - { [0 ... (STACK_TRACE_ENTRIES)] = ULONG_MAX };
> +static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES + 1];
Is the "+ 1" still needed? AFAICT, accesses to this array never go past
nr_entries.
Also I've been staring at the code but I can't figure out why
max_entries is "- 1".
struct stack_trace stack_trace_max = {
.max_entries= STACK_TRACE_ENTRIES - 1,
.entries= _dump_trace[0],
};
--
Josh
[RFC patch 16/41] tracing: Remove the ULONG_MAX stack trace hackery
No architecture terminates the stack trace with ULONG_MAX anymore. As the
code checks the number of entries stored anyway there is no point in
keeping all that ULONG_MAX magic around.
The histogram code zeroes the storage before saving the stack, so if the
trace is shorter than the maximum number of entries it can terminate the
print loop if a zero entry is detected.
Signed-off-by: Thomas Gleixner
Cc: Steven Rostedt
---
kernel/trace/trace_events_hist.c |2 +-
kernel/trace/trace_stack.c | 20 +---
2 files changed, 6 insertions(+), 16 deletions(-)
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -5246,7 +5246,7 @@ static void hist_trigger_stacktrace_prin
unsigned int i;
for (i = 0; i < max_entries; i++) {
- if (stacktrace_entries[i] == ULONG_MAX)
+ if (!stacktrace_entries[i])
return;
seq_printf(m, "%*c", 1 + spaces, ' ');
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -18,8 +18,7 @@
#include "trace.h"
-static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES+1] =
-{ [0 ... (STACK_TRACE_ENTRIES)] = ULONG_MAX };
+static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES + 1];
unsigned stack_trace_index[STACK_TRACE_ENTRIES];
/*
@@ -52,10 +51,7 @@ void stack_trace_print(void)
stack_trace_max.nr_entries);
for (i = 0; i < stack_trace_max.nr_entries; i++) {
- if (stack_dump_trace[i] == ULONG_MAX)
- break;
- if (i+1 == stack_trace_max.nr_entries ||
- stack_dump_trace[i+1] == ULONG_MAX)
+ if (i + 1 == stack_trace_max.nr_entries)
size = stack_trace_index[i];
else
size = stack_trace_index[i] - stack_trace_index[i+1];
@@ -150,8 +146,6 @@ check_stack(unsigned long ip, unsigned l
p = start;
for (; p < top && i < stack_trace_max.nr_entries; p++) {
- if (stack_dump_trace[i] == ULONG_MAX)
- break;
/*
* The READ_ONCE_NOCHECK is used to let KASAN know that
* this is not a stack-out-of-bounds error.
@@ -183,8 +177,6 @@ check_stack(unsigned long ip, unsigned l
}
stack_trace_max.nr_entries = x;
- for (; x < i; x++)
- stack_dump_trace[x] = ULONG_MAX;
if (task_stack_end_corrupted(current)) {
stack_trace_print();
@@ -286,7 +278,7 @@ static void *
{
long n = *pos - 1;
- if (n >= stack_trace_max.nr_entries || stack_dump_trace[n] == ULONG_MAX)
+ if (n >= stack_trace_max.nr_entries)
return NULL;
m->private = (void *)n;
@@ -360,12 +352,10 @@ static int t_show(struct seq_file *m, vo
i = *(long *)v;
- if (i >= stack_trace_max.nr_entries ||
- stack_dump_trace[i] == ULONG_MAX)
+ if (i >= stack_trace_max.nr_entries)
return 0;
- if (i+1 == stack_trace_max.nr_entries ||
- stack_dump_trace[i+1] == ULONG_MAX)
+ if (i + 1 == stack_trace_max.nr_entries)
size = stack_trace_index[i];
else
size = stack_trace_index[i] - stack_trace_index[i+1];
