sys/tlsf: move heap intialization for tlsf-malloc to auto-init #12021

haukepetersen · 2019-08-16T09:07:24Z

Contribution description

Currently, when tlsf-malloc is used, the initialization of the heap memory is left to the application. This poses a problem when modules/packages use malloc and if they are initialized from auto-init -> this leads to tlsf-malloc crashing, as not memory is allocated to it at that point.

This PR solves this issue by moving the buffer allocation for tlsf-malloc into auto-init, before any other modules are initialized. The buffer size is determined by TLSF_MALLOC_HEAPSIZE, which is per default set to 4k, but can simply be redefined from the build system...

Testing procedure

As of now, examples/ccn-lite-relay is the only application in RIOT using tlsf-malloc. For verification, simply compile this PR and compare the used memory with master, it should be identical. Next you can alter the value for TLSF_MALLOC_HEAPSIZE, and the RAM usage of the build should change by the difference of the new value and 10240.

Reproducing the crash behavior pointed out above is slightly more difficult, as the code is not yet PRed. I found it when using NimBLE and CCN-lite at the same time, as NimBLE has some rare malloc´ calls in its initialization code, and those fail once tlsf-malloc` is used...

Issues/PRs references

See issues #4490, #5796.

benpicco

This looks reasonable.
The application should not need to bother with initializing the memory allocator.

jcarrano · 2019-08-16T10:15:02Z

@haukepetersen This may no be enough- there is code that runs before auto init. The standard library itself could be using malloc for once-off allocation of buffers.

See #4490, #5796. Also see my PoC branch tlsf-init-experiments and in particular this file.

benpicco · 2019-08-16T10:20:36Z

The standard library itself could be using malloc for once-off allocation of buffers.

Is this really a concern though? If it did, it would crash already without this patch, just by using tlsf-malloc.

jcarrano · 2019-08-16T10:27:28Z

If it did, it would crash already without this patch, just by using tlsf-malloc

@benpicco it sort of does, see #5796 (comment)

Of course, this patch is better than nothing. Still the right thing would be to do this before the libc initialization:

https://github.com/jcarrano/RIOT/blob/52ca87a82fcdd20c9a1d526dde4034add0aa0bfc/examples/tlsf-init-test/main.c#L81-L86

haukepetersen · 2019-08-16T10:50:46Z

Yes, it is correct that this PR does not help for any malloc calls issued during sys init before auto init is called (as stated in #4490). The goal of this PR was not to solve this solution once and for all, but to provide a mid-term fix, that at least enhances the current situation to some extend, where there are not ugly hacks needed when using e.g. ccn-lite with other libraries...

Including the tlsf-malloc initialization somewhere earlier in the boot process would of course be the preferred solution (and we should aim for that), but it takes significantly more effort than this simple fix and I can't/do not want to wait for this... So can you live with this solution for now?

jcarrano

I'm working on a solution based on my previous work. I will include a test too.

jcarrano · 2019-08-16T14:16:12Z

sys/auto_init/auto_init.c

@@ -20,6 +20,11 @@

 #include "auto_init.h"

+#ifdef MODULE_TLSF_MALLOC
+#include "tlsf-malloc.h"
+static uint8_t _tlsf_pool[TLSF_MALLOC_HEAPSIZE];


On platforms other than malloc this can be derived from _sheap and _eheap linker symbols, which has the advantage of automatically using all available heap.

jcarrano · 2019-08-16T14:17:06Z

sys/auto_init/auto_init.c

@@ -93,6 +98,10 @@

 void auto_init(void)
 {
+#ifdef MODULE_TLSF_MALLOC
+    /* NOTE: this should be initialized before any other modules */


This is right, but is not enough. It should be initialized before the C library itself (that is what .preinit_array is for).

haukepetersen · 2019-08-19T09:28:56Z

I'm working on a solution based on my previous work. I will include a test too.

Sounds good. Are you planning to PR this anytime soon (like this week)? If so, there is no to keep this PR...

jcarrano · 2019-08-19T10:11:07Z

Yes, this week. I already got it working on Friday, but I'm trying to figure out how to specify how much heap to use, maybe you can help me. I'm using sbrk() to reserve a piece of heap at startup.

Heap grows upwards and stack downwards and they eventually meet and when this happens things go boom. Keep in mind tlsf does not have any way to grow a memory pool, only to add new ones.

If I try to reserve the maximum heap possible, from sbrk(0) (that is the current heap limit) to _eheap then there is no space left for stack growth.
Setting a fixed number will not work nicely across multiple boards.
I could use a fixed number (say 4k) and then put some code in malloc() to sbrk() additional 4k blocks as needed and add them as a new pool. This has more complexity and AFAIK the pools are not merged.
I could initialize using a fraction (e.g 75%) of the maximum available heap.
I could initialize using all the available heap minus whatever we want to reserve for stacks.

kaspar030 · 2019-08-19T11:13:51Z

I already got it working on Friday, but I'm trying to figure out how to specify how much heap to use, maybe you can help me. I'm using sbrk() to reserve a piece of heap at startup.

IMO we should do this without sbrk(), as that is only a hack around _sheap and _eheap anyways. Let's just use those defines directly.

Heap grows upwards and stack downwards and they eventually meet and when this happens things go boom.

As discussed offline, this is not the case in RIOT, all stacks are statically allocated at runtime.

This test is currently failing because of RIOT-OS#4490, RIOT-OS#5796 and RIOT-OS#12021. When using TLSF as the system allocator it should be initialized - Automatically, as that is what the user expects. - Early in the boot process, since the C library mallocs internal buffers. Failing to do so will lead to a crash as the issues and this test shows. The test is blacklisted and will be whitelisted in the next commit with the fix.

The TLSF allocator needs to be initialized before use. This is an issue when it is used as a default system allocator since the user expects to be able to call malloc right away. This is made worse by the fact that the C library uses malloc internally to create buffers and that may happen before the user's code has a chance to run. As a consequence, even doing printf when using USEMODULE=tlsf-malloc will lead to a crash. A mechanism is needed to: 1. Initialize the pool early. 2. Determine which memory should be used as a heap and reserve it. Issue (1) is solved by adding the initializer to the C library's `.preinit_array`, which is a cross-file array of function pointers that run before the library is initialized -that is before _init(). See the newlib source code for more details. Point (2) is important because TLSF dows not support growing the pool, only adding new ones. We would like to initialize it with a pool as big as possible. In native (2) is handled by defining a static array of fixed size (given by TLSF_NATIVE_HEAPSIZE). Memory is plentiful in native and we down't care about the overhead of zeroing out this array. On embedded targets using newlib (this may be working on other plaforms, I only tested ARM) `sbrk()` is used to find the start of the heap and reserve it and the `_eheap` linker symbol is used to determine the end of the usable heap. An array is a bad choice here because the size would be board dependent and hard to determine without build-system magic and because it would be zeroed by default, making the boot sequence way longer. sbrk() does nothing more than move a pointer that marks the fraction of the space between _sheap and _eheap that is reserved. Since we are using the whole heap it might be tempting to just use the symbols to derive the pool location and size and to sidestep sbrk(). Especially since the memory allocation functions are expected to be the only users of such a feature. That "trick" would make the OS impossible to debug in case the was a mistake and some of the original allocation functions slipped through non-overriden. If sbrk is used to reserve the entirety of the space then that rogue function will try to call it and fail as no more heap is available. In fact this is how I found out that I was overriding the wrong functions (put a breakpoint int sbrk and show a traceback.) If sbrk is sidestepped one would have nasty and impossible to debug memory corruption errors. A third option could be to use the heap space directly and not define sbrk. This is beyond the scope of this change, but is probably the route to go for platform that do not define this call (but first do a thoroug investigation of how the libc works in that platform). Messing with the global system allocator is not an easy thing to do. I would say that tslf-malloc is ATM _only_ supported in native and cortex-m. Testing procedure: Run `tests/pkg_tlsf_malloc`. Fixes: RIOT-OS#4490, RIOT-OS#5796. Closes: RIOT-OS#12021

This test is currently failing because of RIOT-OS#4490, RIOT-OS#5796 and RIOT-OS#12021. When using TLSF as the system allocator it should be initialized - Automatically, as that is what the user expects. - Early in the boot process, since the C library mallocs internal buffers. Failing to do so will lead to a crash as the issues and this test shows. The test is blacklisted and will be whitelisted in the next commit with the fix.

The TLSF allocator needs to be initialized before use. This is an issue when it is used as a default system allocator since the user expects to be able to call malloc right away. This is made worse by the fact that the C library uses malloc internally to create buffers and that may happen before the user's code has a chance to run. As a consequence, even doing printf when using USEMODULE=tlsf-malloc will lead to a crash. A mechanism is needed to: 1. Initialize the pool early. 2. Determine which memory should be used as a heap and reserve it. Issue (1) is solved by adding the initializer to the C library's `.preinit_array`, which is a cross-file array of function pointers that run before the library is initialized -that is before _init(). See the newlib source code for more details. Point (2) is important because TLSF dows not support growing the pool, only adding new ones. We would like to initialize it with a pool as big as possible. In native (2) is handled by defining a static array of fixed size (given by TLSF_NATIVE_HEAPSIZE). Memory is plentiful in native and we down't care about the overhead of zeroing out this array. On embedded targets using newlib (this may be working on other plaforms, I only tested ARM) `sbrk()` is used to find the start of the heap and reserve it and the `_eheap` linker symbol is used to determine the end of the usable heap. An array is a bad choice here because the size would be board dependent and hard to determine without build-system magic and because it would be zeroed by default, making the boot sequence way longer. sbrk() does nothing more than move a pointer that marks the fraction of the space between _sheap and _eheap that is reserved. Since we are using the whole heap it might be tempting to just use the symbols to derive the pool location and size and to sidestep sbrk(). Especially since the memory allocation functions are expected to be the only users of such a feature. That "trick" would make the OS impossible to debug in case the was a mistake and some of the original allocation functions slipped through non-overriden. If sbrk is used to reserve the entirety of the space then that rogue function will try to call it and fail as no more heap is available. In fact this is how I found out that I was overriding the wrong functions (put a breakpoint int sbrk and show a traceback.) If sbrk is sidestepped one would have nasty and impossible to debug memory corruption errors. A third option could be to use the heap space directly and not define sbrk. This is beyond the scope of this change, but is probably the route to go for platform that do not define this call (but first do a thoroug investigation of how the libc works in that platform). Messing with the global system allocator is not an easy thing to do. I would say that tslf-malloc is ATM _only_ supported in native and cortex-m. Testing procedure: Run `tests/pkg_tlsf_malloc`. Fixes: RIOT-OS#4490, RIOT-OS#5796. Closes: RIOT-OS#12021

haukepetersen · 2019-08-22T07:33:16Z

closing in favor of #12032

Thanks @jcarrano for taking over and fixing it properly!

jcarrano · 2019-08-22T10:00:19Z

properly

I'm not bold enough to make that claim 😁 .

This test is currently failing because of RIOT-OS#4490, RIOT-OS#5796 and RIOT-OS#12021. When using TLSF as the system allocator it should be initialized - Automatically, as that is what the user expects. - Early in the boot process, since the C library mallocs internal buffers. Failing to do so will lead to a crash as the issues and this test shows. The test is blacklisted and will be whitelisted in the next commit with the fix.

The TLSF allocator needs to be initialized before use. This is an issue when it is used as a default system allocator since the user expects to be able to call malloc right away. This is made worse by the fact that the C library uses malloc internally to create buffers and that may happen before the user's code has a chance to run. As a consequence, even doing printf when using USEMODULE=tlsf-malloc will lead to a crash. A mechanism is needed to: 1. Initialize the pool early. 2. Determine which memory should be used as a heap and reserve it. Issue (1) is solved by adding the initializer to the C library's `.preinit_array`, which is a cross-file array of function pointers that run before the library is initialized -that is before _init(). See the newlib source code for more details. Point (2) is important because TLSF dows not support growing the pool, only adding new ones. We would like to initialize it with a pool as big as possible. In native (2) is handled by defining a static array of fixed size (given by TLSF_NATIVE_HEAPSIZE). Memory is plentiful in native and we down't care about the overhead of zeroing out this array. On embedded targets using newlib (this may be working on other plaforms, I only tested ARM) `sbrk()` is used to find the start of the heap and reserve it and the `_eheap` linker symbol is used to determine the end of the usable heap. An array is a bad choice here because the size would be board dependent and hard to determine without build-system magic and because it would be zeroed by default, making the boot sequence way longer. sbrk() does nothing more than move a pointer that marks the fraction of the space between _sheap and _eheap that is reserved. Since we are using the whole heap it might be tempting to just use the symbols to derive the pool location and size and to sidestep sbrk(). Especially since the memory allocation functions are expected to be the only users of such a feature. That "trick" would make the OS impossible to debug in case the was a mistake and some of the original allocation functions slipped through non-overriden. If sbrk is used to reserve the entirety of the space then that rogue function will try to call it and fail as no more heap is available. In fact this is how I found out that I was overriding the wrong functions (put a breakpoint int sbrk and show a traceback.) If sbrk is sidestepped one would have nasty and impossible to debug memory corruption errors. A third option could be to use the heap space directly and not define sbrk. This is beyond the scope of this change, but is probably the route to go for platform that do not define this call (but first do a thoroug investigation of how the libc works in that platform). Messing with the global system allocator is not an easy thing to do. I would say that tslf-malloc is ATM _only_ supported in native and cortex-m. Testing procedure: Run `tests/pkg_tlsf_malloc`. Fixes: RIOT-OS#4490, RIOT-OS#5796. Closes: RIOT-OS#12021

haukepetersen added 3 commits August 16, 2019 10:48

pkg/tlsf/malloc: define default heap size

458b728

sys/auto-init: init global tlsf-malloc pool on boot

615e6de

examples/ccn-lite-relay: remove tlsf heap init

a66f9b4

benpicco approved these changes Aug 16, 2019

View reviewed changes

jcarrano suggested changes Aug 16, 2019

View reviewed changes

jcarrano mentioned this pull request Aug 19, 2019

pkg/tlsf: Early initialization of memory pool. #12032

Open

jcarrano mentioned this pull request Aug 20, 2019

pkg/ccn-lite: remove dependency on tlsf-malloc. #12043

Merged

haukepetersen closed this Aug 22, 2019

haukepetersen deleted the opt_tlsf_autoinit branch August 22, 2019 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/tlsf: move heap intialization for tlsf-malloc to auto-init #12021

sys/tlsf: move heap intialization for tlsf-malloc to auto-init #12021

haukepetersen commented Aug 16, 2019 •

edited by jcarrano

benpicco left a comment

jcarrano commented Aug 16, 2019

benpicco commented Aug 16, 2019

jcarrano commented Aug 16, 2019

haukepetersen commented Aug 16, 2019

jcarrano left a comment

jcarrano Aug 16, 2019

jcarrano Aug 16, 2019

haukepetersen commented Aug 19, 2019

jcarrano commented Aug 19, 2019

kaspar030 commented Aug 19, 2019

haukepetersen commented Aug 22, 2019

jcarrano commented Aug 22, 2019

sys/tlsf: move heap intialization for tlsf-malloc to auto-init #12021

sys/tlsf: move heap intialization for tlsf-malloc to auto-init #12021

Conversation

haukepetersen commented Aug 16, 2019 • edited by jcarrano

Contribution description

Testing procedure

Issues/PRs references

benpicco left a comment

Choose a reason for hiding this comment

jcarrano commented Aug 16, 2019

benpicco commented Aug 16, 2019

jcarrano commented Aug 16, 2019

haukepetersen commented Aug 16, 2019

jcarrano left a comment

Choose a reason for hiding this comment

jcarrano Aug 16, 2019

Choose a reason for hiding this comment

jcarrano Aug 16, 2019

Choose a reason for hiding this comment

haukepetersen commented Aug 19, 2019

jcarrano commented Aug 19, 2019

kaspar030 commented Aug 19, 2019

haukepetersen commented Aug 22, 2019

jcarrano commented Aug 22, 2019

haukepetersen commented Aug 16, 2019 •

edited by jcarrano