| Message ID | 20170614211556.2062728-4-arnd@arndb.de |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello, Arnd Bergmann, on mer. 14 juin 2017 23:15:38 +0200, wrote: > As reported by kernelci, some functions in the VT code use significant > amounts of kernel stack when local variables get inlined into the caller > multiple times: > > drivers/tty/vt/keyboard.c: In function 'kbd_keycode': > drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=] > > Annotating those functions as noinline_if_stackbloat prevents the inlining > and reduces the overall stack usage in this driver. > --- a/drivers/tty/vt/keyboard.c > +++ b/drivers/tty/vt/keyboard.c > @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt) > /* > * Helper Functions. > */ > -static void put_queue(struct vc_data *vc, int ch) > +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch) > { > tty_insert_flip_char(&vc->port, ch, 0); > tty_schedule_flip(&vc->port); > } I'm surprised that this be able generate so much stack use: the tty_insert_flip_char inline only uses a pointer and an int. And I'm surprised that multiple inlines can accumulate stack usage. I however agree that it's a bad idea to inline it in functions where it's called so many times (and we're talking about the keyboard anyway). > -static void puts_queue(struct vc_data *vc, char *cp) > +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp) I don't see why, it's only called once in the callers. k_fn, however, is called several times in k_pad, so that could be why, but then it's rather be the inlining of k_fn which is a bad idea. > -static void fn_send_intr(struct vc_data *vc) > +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc) This one is only referenced, not called, I don't see how that could pose problem. Samuel
On Wed, Jun 14, 2017 at 11:28 PM, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote: > Hello, > > Arnd Bergmann, on mer. 14 juin 2017 23:15:38 +0200, wrote: >> As reported by kernelci, some functions in the VT code use significant >> amounts of kernel stack when local variables get inlined into the caller >> multiple times: >> >> drivers/tty/vt/keyboard.c: In function 'kbd_keycode': >> drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=] >> >> Annotating those functions as noinline_if_stackbloat prevents the inlining >> and reduces the overall stack usage in this driver. > > >> --- a/drivers/tty/vt/keyboard.c >> +++ b/drivers/tty/vt/keyboard.c >> @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt) >> /* >> * Helper Functions. >> */ >> -static void put_queue(struct vc_data *vc, int ch) >> +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch) >> { >> tty_insert_flip_char(&vc->port, ch, 0); >> tty_schedule_flip(&vc->port); >> } > > I'm surprised that this be able generate so much stack use: the > tty_insert_flip_char inline only uses a pointer and an int. > > And I'm surprised that multiple inlines can accumulate stack usage. The reason is that CONFIG_KASAN forces each local variable to have a separate location on the stack whenever it gets passed into an external function (tty_insert_flip_string_flags in this case), so the sanitizer is able to report exactly which instance caused the problem. > I however agree that it's a bad idea to inline it in functions where > it's called so many times (and we're talking about the keyboard anyway). > >> -static void puts_queue(struct vc_data *vc, char *cp) >> +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp) > > I don't see why, it's only called once in the callers. k_fn, however, is > called several times in k_pad, so that could be why, but then it's > rather be the inlining of k_fn which is a bad idea. It's called by applkey, which in turn is called by k_pad(), and this all gets inlined by default. >> -static void fn_send_intr(struct vc_data *vc) >> +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc) > > This one is only referenced, not called, I don't see how that could pose > problem. I was surprised by this as well, but it seems that gcc these days is smart enough to turn the indirect function calls for k_handler[type] and/or f_handler[value] into inlines again when it has already determined the index to be constant. It's been a while since I looked at the patch, and I'd have to disassemble it again to figure out the details, but I'm pretty sure I needed this to get the stack usage down to normal levels. Arnd
Arnd Bergmann, on mer. 14 juin 2017 23:56:39 +0200, wrote: > > I however agree that it's a bad idea to inline it in functions where > > it's called so many times (and we're talking about the keyboard anyway). > > > >> -static void puts_queue(struct vc_data *vc, char *cp) > >> +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp) > > > > I don't see why, it's only called once in the callers. k_fn, however, is > > called several times in k_pad, so that could be why, but then it's > > rather be the inlining of k_fn which is a bad idea. > > It's called by applkey, which in turn is called by k_pad(), k_pad calls applkey twice only. Is that really to be considered bloat? > >> -static void fn_send_intr(struct vc_data *vc) > >> +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc) > > > > This one is only referenced, not called, I don't see how that could pose > > problem. > > I was surprised by this as well, but it seems that gcc these days is > smart enough to turn the indirect function calls for k_handler[type] > and/or f_handler[value] into inlines again when it has already > determined the index to be constant. Cool :) But I don't see how it can see find it out constant. The only fn_handler[] caller is k_spec, using value as index. The only caller of f_handler[] is kbd_keycode, using type as index, and keysym&0xff as value. That is definitely not constant :) And it's only one caller, I don't see how that can bloat. Samuel
On Wed, Jun 14, 2017 at 11:15:38PM +0200, Arnd Bergmann wrote: > As reported by kernelci, some functions in the VT code use significant > amounts of kernel stack when local variables get inlined into the caller > multiple times: > > drivers/tty/vt/keyboard.c: In function 'kbd_keycode': > drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=] > > Annotating those functions as noinline_if_stackbloat prevents the inlining > and reduces the overall stack usage in this driver. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/tty/vt/keyboard.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c > index f4166263bb3a..c0d111444a0e 100644 > --- a/drivers/tty/vt/keyboard.c > +++ b/drivers/tty/vt/keyboard.c > @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt) > /* > * Helper Functions. > */ > -static void put_queue(struct vc_data *vc, int ch) > +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch) > { > tty_insert_flip_char(&vc->port, ch, 0); > tty_schedule_flip(&vc->port); > } Ugh, really? We have to start telling gcc not to be stupid here? That's not going to be easy, and will just entail us doing this all over the place, right? The code isn't asking to be inlined, so why is gcc allowing it to be done that way? Doesn't that imply gcc is the problem here? thanks, greg k-h
On Thu, Jun 15, 2017 at 06:52:21AM +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 14, 2017 at 11:15:38PM +0200, Arnd Bergmann wrote: > > As reported by kernelci, some functions in the VT code use significant > > amounts of kernel stack when local variables get inlined into the caller > > multiple times: > > > > drivers/tty/vt/keyboard.c: In function 'kbd_keycode': > > drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=] > > > > Annotating those functions as noinline_if_stackbloat prevents the inlining > > and reduces the overall stack usage in this driver. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/tty/vt/keyboard.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c > > index f4166263bb3a..c0d111444a0e 100644 > > --- a/drivers/tty/vt/keyboard.c > > +++ b/drivers/tty/vt/keyboard.c > > @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt) > > /* > > * Helper Functions. > > */ > > -static void put_queue(struct vc_data *vc, int ch) > > +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch) > > { > > tty_insert_flip_char(&vc->port, ch, 0); > > tty_schedule_flip(&vc->port); > > } > > Ugh, really? We have to start telling gcc not to be stupid here? > That's not going to be easy, and will just entail us doing this all over > the place, right? > > The code isn't asking to be inlined, so why is gcc allowing it to be > done that way? Doesn't that imply gcc is the problem here? Wait, you are now, in this patch, _asking_ for it to be inlined. How is that solving anything? greg k-h
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c index f4166263bb3a..c0d111444a0e 100644 --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt) /* * Helper Functions. */ -static void put_queue(struct vc_data *vc, int ch) +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch) { tty_insert_flip_char(&vc->port, ch, 0); tty_schedule_flip(&vc->port); } -static void puts_queue(struct vc_data *vc, char *cp) +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp) { while (*cp) { tty_insert_flip_char(&vc->port, *cp, 0); @@ -555,7 +555,7 @@ static void fn_inc_console(struct vc_data *vc) set_console(i); } -static void fn_send_intr(struct vc_data *vc) +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc) { tty_insert_flip_char(&vc->port, 0, TTY_BREAK); tty_schedule_flip(&vc->port);
As reported by kernelci, some functions in the VT code use significant amounts of kernel stack when local variables get inlined into the caller multiple times: drivers/tty/vt/keyboard.c: In function 'kbd_keycode': drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=] Annotating those functions as noinline_if_stackbloat prevents the inlining and reduces the overall stack usage in this driver. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/tty/vt/keyboard.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)