[PATCH] tracing: events_hist: avoid using excessive stack space

[PATCH] tracing: events_hist: avoid using excessive stack space

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

In some configurations, clang produces a warning about an overly large
amount of stack space used in hist_trigger_print_key():

kernel/trace/trace_events_hist.c:4594:13: error: stack frame size of 1248 bytes in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than=]
static void hist_trigger_print_key(struct seq_file *m,

Moving the 'str' variable into a more local scope in the two places
where it gets used actually reduces the the used stack space here
and gets it below the warning limit, because the compiler can now
assume that it is safe to use the same stack slot that it has for
the stack of any inline function.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 kernel/trace/trace_events_hist.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c1abd63f1d6c..e3fe84f017a8 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4597,7 +4597,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;
@@ -4617,11 +4616,13 @@ static void hist_trigger_print_key(struct seq_file *m,
 			uval = *(u64 *)(key + key_field->offset);
 			seq_printf(m, "%s: %llx", field_name, uval);
 		} else if (key_field->flags & HIST_FIELD_FL_SYM) {
+			char str[KSYM_SYMBOL_LEN];
 			uval = *(u64 *)(key + key_field->offset);
 			sprint_symbol_no_offset(str, uval);
 			seq_printf(m, "%s: [%llx] %-45s", field_name,
 				   uval, str);
 		} else if (key_field->flags & HIST_FIELD_FL_SYM_OFFSET) {
+			char str[KSYM_SYMBOL_LEN];
 			uval = *(u64 *)(key + key_field->offset);
 			sprint_symbol(str, uval);
 			seq_printf(m, "%s: [%llx] %-55s", field_name,
-- 
2.29.2

Re: [PATCH] tracing: events_hist: avoid using excessive stack space

[Steven Rostedt]

On Fri, 14 May 2021 16:04:25 +0200
Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> In some configurations, clang produces a warning about an overly large
> amount of stack space used in hist_trigger_print_key():
> 
> kernel/trace/trace_events_hist.c:4594:13: error: stack frame size of 1248 bytes in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than=]
> static void hist_trigger_print_key(struct seq_file *m,
> 
> Moving the 'str' variable into a more local scope in the two places
> where it gets used actually reduces the the used stack space here
> and gets it below the warning limit, because the compiler can now
> assume that it is safe to use the same stack slot that it has for
> the stack of any inline function.

Thanks Arnd for the nice explanation of the rationale for this change.

But I still find it too subtle to my liking that we need to move the
declaration like this (and duplicate it twice) for internal behavior of the
compiler (where it can't figure out itself by the use cases if it can
optimize the stack).

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  kernel/trace/trace_events_hist.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index c1abd63f1d6c..e3fe84f017a8 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4597,7 +4597,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];

Instead, I think we should just make str static, as this should only be
called under the event_mutex. To be sure, we can also add:

	/* To protect the static str variable */
	lockdep_assert_held(&event_mutex);

-- Steve

>  	bool multiline = false;
>  	const char *field_name;
>  	unsigned int i;
> @@ -4617,11 +4616,13 @@ static void hist_trigger_print_key(struct seq_file *m,
>  			uval = *(u64 *)(key + key_field->offset);
>  			seq_printf(m, "%s: %llx", field_name, uval);
>  		} else if (key_field->flags & HIST_FIELD_FL_SYM) {
> +			char str[KSYM_SYMBOL_LEN];
>  			uval = *(u64 *)(key + key_field->offset);
>  			sprint_symbol_no_offset(str, uval);
>  			seq_printf(m, "%s: [%llx] %-45s", field_name,
>  				   uval, str);
>  		} else if (key_field->flags & HIST_FIELD_FL_SYM_OFFSET) {
> +			char str[KSYM_SYMBOL_LEN];
>  			uval = *(u64 *)(key + key_field->offset);
>  			sprint_symbol(str, uval);
>  			seq_printf(m, "%s: [%llx] %-55s", field_name,

Re: [PATCH] tracing: events_hist: avoid using excessive stack space

[Arnd Bergmann]

On Fri, May 14, 2021 at 4:16 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 14 May 2021 16:04:25 +0200 Arnd Bergmann <arnd@kernel.org> wrote:
>
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > In some configurations, clang produces a warning about an overly large
> > amount of stack space used in hist_trigger_print_key():
> >
> > kernel/trace/trace_events_hist.c:4594:13: error: stack frame size of 1248 bytes in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than=]
> > static void hist_trigger_print_key(struct seq_file *m,
> >
> > Moving the 'str' variable into a more local scope in the two places
> > where it gets used actually reduces the the used stack space here
> > and gets it below the warning limit, because the compiler can now
> > assume that it is safe to use the same stack slot that it has for
> > the stack of any inline function.
>
> Thanks Arnd for the nice explanation of the rationale for this change.
>
> But I still find it too subtle to my liking that we need to move the
> declaration like this (and duplicate it twice) for internal behavior of the
> compiler (where it can't figure out itself by the use cases if it can
> optimize the stack).

It's not really internal behavior of the compiler that causes the smaller
scope to help here, I'm sure gcc has to do the same thing regarding
the placement of this variable on the stack. Clang does have some related
cases that it doesn't do as well as gcc, but I don't think this is one of them.

The difference between the compilers or configurations is the way that
inlining decisions are made, so if a compiler inlines another function into this
one that also uses a lot of stack space, that will trigger the warning, while
a compiler that does not inline that would not warn but also not use any
less stack space in the code path that uses the nested function.

> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index c1abd63f1d6c..e3fe84f017a8 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -4597,7 +4597,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];
>
> Instead, I think we should just make str static, as this should only be
> called under the event_mutex. To be sure, we can also add:
>
>         /* To protect the static str variable */
>         lockdep_assert_held(&event_mutex);

Ok, I changed my local version to this, will send v2 once it passes
some more randconfig builds.

         Arnd

Re: [PATCH] tracing: events_hist: avoid using excessive stack space

[Nick Desaulniers]

On Fri, May 14, 2021 at 7:16 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 14 May 2021 16:04:25 +0200
> Arnd Bergmann <arnd@kernel.org> wrote:
>
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > In some configurations, clang produces a warning about an overly large
> > amount of stack space used in hist_trigger_print_key():
> >
> > kernel/trace/trace_events_hist.c:4594:13: error: stack frame size of 1248 bytes in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than=]
> > static void hist_trigger_print_key(struct seq_file *m,
> >
> > Moving the 'str' variable into a more local scope in the two places
> > where it gets used actually reduces the the used stack space here
> > and gets it below the warning limit, because the compiler can now
> > assume that it is safe to use the same stack slot that it has for
> > the stack of any inline function.
>
> Thanks Arnd for the nice explanation of the rationale for this change.
>
> But I still find it too subtle to my liking that we need to move the
> declaration like this (and duplicate it twice) for internal behavior of the
> compiler (where it can't figure out itself by the use cases if it can
> optimize the stack).

Under which configurations this warning was observed wasn't specified,
but I'd bet it's one of the sanitizers splitting this in two in order
to tell which branch may have overflowed the buffer.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] tracing: events_hist: avoid using excessive stack space

[Arnd Bergmann]

On Fri, May 14, 2021 at 10:19 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> On Fri, May 14, 2021 at 7:16 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Fri, 14 May 2021 16:04:25 +0200
> > Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > In some configurations, clang produces a warning about an overly large
> > > amount of stack space used in hist_trigger_print_key():
> > >
> > > kernel/trace/trace_events_hist.c:4594:13: error: stack frame size of 1248 bytes in function 'hist_trigger_print_key' [-Werror,-Wframe-larger-than=]
> > > static void hist_trigger_print_key(struct seq_file *m,
> > >
> > > Moving the 'str' variable into a more local scope in the two places
> > > where it gets used actually reduces the the used stack space here
> > > and gets it below the warning limit, because the compiler can now
> > > assume that it is safe to use the same stack slot that it has for
> > > the stack of any inline function.
> >
> > Thanks Arnd for the nice explanation of the rationale for this change.
> >
> > But I still find it too subtle to my liking that we need to move the
> > declaration like this (and duplicate it twice) for internal behavior of the
> > compiler (where it can't figure out itself by the use cases if it can
> > optimize the stack).
>
> Under which configurations this warning was observed wasn't specified,
> but I'd bet it's one of the sanitizers splitting this in two in order
> to tell which branch may have overflowed the buffer.

Right, I did not manage to find the exact conditions that trigger it.
I did *not* see it with KASAN enabled and I saw it with UBSAN both
on or off.

       Arnd