lkml.org 
[lkml]   [2021]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [v3] tracing: use %ps format string to print symbols
On Tue, 19 Oct 2021 17:33:13 +0200
Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
>
> clang started warning about excessive stack usage in
> hist_trigger_print_key()
>
> kernel/trace/trace_events_hist.c:4723:13: error: stack frame size (1336) exceeds limit (1024) in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than]
>
> The problem is that there are two 512-byte arrays on the stack if
> hist_trigger_stacktrace_print() gets inlined. I don't think this has
> changed in the past five years, but something probably changed the
> inlining decisions made by the compiler, so the problem is now made
> more obvious.
>
> Rather than printing the symbol names into separate buffers, it
> seems we can simply use the special %ps format string modifier
> to print the pointers symbolically and get rid of both buffers.
>
> Marking hist_trigger_stacktrace_print() would be a simpler
> way of avoiding the warning, but that would not address the
> excessive stack usage.
>
> Fixes: 69a0200c2e25 ("tracing: Add hist trigger support for stacktraces as keys")
> Link: https://lore.kernel.org/all/20211015095704.49a99859@gandalf.local.home/
> Reviewed-by: Tom Zanussi <zanussi@kernel.org>
> Tested-by: Tom Zanussi <zanussi@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

BTW, there are other functions under kernel/trace still allocating
"char str[KSYM_SYMBOL_LEN]". I guess those also should be changed if
this is the stack usage issue, right?

Thank you,

> ---
> Changes in v3:
> - Fix 32-bit cast warning
>
> Changes in v2:
> - Use %pS instead of %ps to include offset in third string
> - add (void*) cast to avoid compile-time warnings
> ---
> kernel/trace/trace_events_hist.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index f01e442716e2..632386b73fc3 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4706,7 +4706,6 @@ static void hist_trigger_stacktrace_print(struct seq_file *m,
> unsigned long *stacktrace_entries,
> unsigned int max_entries)
> {
> - char str[KSYM_SYMBOL_LEN];
> unsigned int spaces = 8;
> unsigned int i;
>
> @@ -4715,8 +4714,7 @@ static void hist_trigger_stacktrace_print(struct seq_file *m,
> return;
>
> seq_printf(m, "%*c", 1 + spaces, ' ');
> - sprint_symbol(str, stacktrace_entries[i]);
> - seq_printf(m, "%s\n", str);
> + seq_printf(m, "%pS\n", (void*)stacktrace_entries[i]);
> }
> }
>
> @@ -4726,7 +4724,6 @@ static void hist_trigger_print_key(struct seq_file *m,
> struct tracing_map_elt *elt)
> {
> struct hist_field *key_field;
> - char str[KSYM_SYMBOL_LEN];
> bool multiline = false;
> const char *field_name;
> unsigned int i;
> @@ -4747,14 +4744,12 @@ static void hist_trigger_print_key(struct seq_file *m,
> seq_printf(m, "%s: %llx", field_name, uval);
> } else if (key_field->flags & HIST_FIELD_FL_SYM) {
> uval = *(u64 *)(key + key_field->offset);
> - sprint_symbol_no_offset(str, uval);
> - seq_printf(m, "%s: [%llx] %-45s", field_name,
> - uval, str);
> + seq_printf(m, "%s: [%llx] %-45ps", field_name,
> + uval, (void *)(uintptr_t)uval);
> } else if (key_field->flags & HIST_FIELD_FL_SYM_OFFSET) {
> uval = *(u64 *)(key + key_field->offset);
> - sprint_symbol(str, uval);
> - seq_printf(m, "%s: [%llx] %-55s", field_name,
> - uval, str);
> + seq_printf(m, "%s: [%llx] %-55pS", field_name,
> + uval, (void *)(uintptr_t)uval);
> } else if (key_field->flags & HIST_FIELD_FL_EXECNAME) {
> struct hist_elt_data *elt_data = elt->private_data;
> char *comm;
> --
> 2.29.2
>


--
Masami Hiramatsu <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2021-10-21 02:36    [W:0.063 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site