On Mon, Feb 29, 2016 at 01:20:28PM +0100, Arnd Bergmann wrote:
> When map_word gets too large, we use a lot of kernel stack, and for
> MTD_MAP_BANK_WIDTH_32, this means we use more than the recommended
> 1024 bytes in a number of functions:
>
> drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
> drivers/mtd/chips/cfi_cmdset_0020.c:651:1: warning: the frame size of 1336 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_erase_varsize':
> drivers/mtd/chips/cfi_cmdset_0020.c:972:1: warning: the frame size of 1208 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> drivers/mtd/chips/cfi_cmdset_0001.c: In function 'do_write_buffer':
> drivers/mtd/chips/cfi_cmdset_0001.c:1835:1: warning: the frame size of 1240 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> This can be avoided if all operations on the map word are done
> indirectly and the stack gets reused between the calls. We can
> mostly achieve this by selecting MTD_COMPLEX_MAPPINGS whenever
> MTD_MAP_BANK_WIDTH_32 is set, but for the case that no other
> bank width is enabled, we also need to use a non-constant
> map_bankwidth() to convince the compiler to use less stack.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> v2: added 'if HAS_IOMEM' to avoid a Kconfig warning on user-mode-linux.
>
> Originally sent out on Jan 1, sorry for taking my time to follow
> up with a new version.
>
> drivers/mtd/chips/Kconfig | 1 +
> include/linux/mtd/map.h | 19 +++++++------------
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/chips/Kconfig b/drivers/mtd/chips/Kconfig
> index 741ec69e0b46..cd162cbc3746 100644
> --- a/drivers/mtd/chips/Kconfig
> +++ b/drivers/mtd/chips/Kconfig
> @@ -117,6 +117,7 @@ config MTD_MAP_BANK_WIDTH_16
>
> config MTD_MAP_BANK_WIDTH_32
> bool "Support 256-bit buswidth" if MTD_CFI_GEOMETRY
> + select MTD_COMPLEX_MAPPINGS if HAS_IOMEM
> default n
> help
> If you wish to support CFI devices on a physical bus which is
> diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
> index 5e0eb7ccabd4..3aa56e3104bb 100644
> --- a/include/linux/mtd/map.h
> +++ b/include/linux/mtd/map.h
> @@ -122,18 +122,13 @@
> #endif
>
> #ifdef CONFIG_MTD_MAP_BANK_WIDTH_32
> -# ifdef map_bankwidth
> -# undef map_bankwidth
> -# define map_bankwidth(map) ((map)->bankwidth)
> -# undef map_bankwidth_is_large
> -# define map_bankwidth_is_large(map) (map_bankwidth(map) > BITS_PER_LONG/8)
> -# undef map_words
> -# define map_words(map) map_calc_words(map)
> -# else
> -# define map_bankwidth(map) 32
> -# define map_bankwidth_is_large(map) (1)
> -# define map_words(map) map_calc_words(map)
> -# endif
> +/* always use indirect access for 256-bit to preserve kernel stack */
> +# undef map_bankwidth
> +# define map_bankwidth(map) ((map)->bankwidth)
> +# undef map_bankwidth_is_large
> +# define map_bankwidth_is_large(map) (map_bankwidth(map) > BITS_PER_LONG/8)
> +# undef map_words
> +# define map_words(map) map_calc_words(map)
> #define map_bankwidth_is_32(map) (map_bankwidth(map) == 32)
> #undef MAX_MAP_BANKWIDTH
> #define MAX_MAP_BANKWIDTH 32
Looking a little closer at this... why do we need the changes to
include/linux/mtd/map.h again? It should be fine to leave these
definitions as-is, right? They don't contribute to the large stack
usage, do they?
Maybe I'm just missing something obvious, so please do enlighten :)
Brian
On Friday 04 March 2016 13:21:59 Brian Norris wrote:
>
> Looking a little closer at this... why do we need the changes to
> include/linux/mtd/map.h again? It should be fine to leave these
> definitions as-is, right? They don't contribute to the large stack
> usage, do they?
>
> Maybe I'm just missing something obvious, so please do enlighten
>
It's been a while since I created the patch, and the originally
failing configuration currently doesn't produce this (probably
because something else changed). I remember that it was something
rather subtle, but don't exactly remember what happened.
I've reverted the patch now, trying to reproduce it on my
randconfig setup, but I might not be able to get back to you
in the next week while I'm traveling.
Arnd
On Sat, Mar 05, 2016 at 12:25:24AM +0100, Arnd Bergmann wrote:
> On Friday 04 March 2016 13:21:59 Brian Norris wrote:
> >
> > Looking a little closer at this... why do we need the changes to
> > include/linux/mtd/map.h again? It should be fine to leave these
> > definitions as-is, right? They don't contribute to the large stack
> > usage, do they?
> >
> > Maybe I'm just missing something obvious, so please do enlighten
> >
>
> It's been a while since I created the patch, and the originally
> failing configuration currently doesn't produce this (probably
> because something else changed). I remember that it was something
> rather subtle, but don't exactly remember what happened.
>
> I've reverted the patch now, trying to reproduce it on my
> randconfig setup, but I might not be able to get back to you
> in the next week while I'm traveling.
FWIW, I took a little look at this, and I can reproduce this myself. I
can get a large frame size on at least 2 of the 3 functions you report.
I think most of the gain you get with this patch is due to the Kconfig
change (MTD_COMPLEX_MAPPINGS) because it forces an extra level of
indirection. The other changes seem to give a modest decrease in size,
though it's less clear why exactly.
Anyway, I think the problem isn't primarily with anything you're
touching here exactly, but with the fact that we're putting several
copies of the 'map_word' typedef on the stack, and doing assignment to
it as if it's a typical variable. But with MAP_WIDTH_32, this is a
32-byte object, and I assume that these (too) long functions are
introducing enough complexity that the compiler has to have several
copies of them. To properly fix all this, it seems like the code could
use some more attention, and not just the superficial changes here.
Anyway, I'm interested if you have any more thoughts on this. If you
still think this is worthwhile, I can probably just take it. (It looks
OK, despite the above comments.)
Brian
On Friday 18 March 2016 10:44:19 Brian Norris wrote:
> On Sat, Mar 05, 2016 at 12:25:24AM +0100, Arnd Bergmann wrote:
> > On Friday 04 March 2016 13:21:59 Brian Norris wrote:
> > >
> > > Looking a little closer at this... why do we need the changes to
> > > include/linux/mtd/map.h again? It should be fine to leave these
> > > definitions as-is, right? They don't contribute to the large stack
> > > usage, do they?
> > >
> > > Maybe I'm just missing something obvious, so please do enlighten
> > >
> >
> > It's been a while since I created the patch, and the originally
> > failing configuration currently doesn't produce this (probably
> > because something else changed). I remember that it was something
> > rather subtle, but don't exactly remember what happened.
> >
> > I've reverted the patch now, trying to reproduce it on my
> > randconfig setup, but I might not be able to get back to you
> > in the next week while I'm traveling.
>
> FWIW, I took a little look at this, and I can reproduce this myself. I
> can get a large frame size on at least 2 of the 3 functions you report.
> I think most of the gain you get with this patch is due to the Kconfig
> change (MTD_COMPLEX_MAPPINGS) because it forces an extra level of
> indirection. The other changes seem to give a modest decrease in size,
> though it's less clear why exactly.
>
> Anyway, I think the problem isn't primarily with anything you're
> touching here exactly, but with the fact that we're putting several
> copies of the 'map_word' typedef on the stack, and doing assignment to
> it as if it's a typical variable. But with MAP_WIDTH_32, this is a
> 32-byte object, and I assume that these (too) long functions are
> introducing enough complexity that the compiler has to have several
> copies of them. To properly fix all this, it seems like the code could
> use some more attention, and not just the superficial changes here.
>
> Anyway, I'm interested if you have any more thoughts on this. If you
> still think this is worthwhile, I can probably just take it. (It looks
> OK, despite the above comments.)
I'd like to get this fixed in one way or another, but I'm not
particularly attached to my specific workaround.
If you don't think anyone will be able to reduce the number of
map_word instances on the stack, please add my patch so at least
we get rid of the warning and make it less likely to hit the bug.
An alternative might be to optionally move the map_word_and/clr/or/...
operations out of line for large sizes of map_word, so we don't
have all instances on the stack at once.
Arnd
On Fri, Mar 18, 2016 at 09:25:23PM +0100, Arnd Bergmann wrote:
> On Friday 18 March 2016 10:44:19 Brian Norris wrote:
> > FWIW, I took a little look at this, and I can reproduce this myself. I
> > can get a large frame size on at least 2 of the 3 functions you report.
> > I think most of the gain you get with this patch is due to the Kconfig
> > change (MTD_COMPLEX_MAPPINGS) because it forces an extra level of
> > indirection. The other changes seem to give a modest decrease in size,
> > though it's less clear why exactly.
> >
> > Anyway, I think the problem isn't primarily with anything you're
> > touching here exactly, but with the fact that we're putting several
> > copies of the 'map_word' typedef on the stack, and doing assignment to
> > it as if it's a typical variable. But with MAP_WIDTH_32, this is a
> > 32-byte object, and I assume that these (too) long functions are
> > introducing enough complexity that the compiler has to have several
> > copies of them. To properly fix all this, it seems like the code could
> > use some more attention, and not just the superficial changes here.
> >
> > Anyway, I'm interested if you have any more thoughts on this. If you
> > still think this is worthwhile, I can probably just take it. (It looks
> > OK, despite the above comments.)
>
> I'd like to get this fixed in one way or another, but I'm not
> particularly attached to my specific workaround.
>
> If you don't think anyone will be able to reduce the number of
> map_word instances on the stack, please add my patch so at least
> we get rid of the warning and make it less likely to hit the bug.
OK, pushed to l2-mtd.git.
> An alternative might be to optionally move the map_word_and/clr/or/...
> operations out of line for large sizes of map_word, so we don't
> have all instances on the stack at once.
Hmm, I guess given the right amount of macro magic, it might be possible
to do this while still remaining optimal for small word sizes. But I'm
not sure anyone is really tackling these sorts of things on these
ancient drivers.
Brian