allow each zone of memory allocated to heap to have different control block (IDFGH-6152)

[philippe44]

See #7822

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Alvin1Zhang]

Thanks for your contribution.

[igrr]

Hi @philippe44, thank you for this useful PR. We had an internal feature request for this functionality for a while, very nice to see it implemented!

For your PR to be accepted, it needs to be made against the master branch of the repository. Could you rebase it and change the target branch, please? Once merged into master, we can consider backporting it to older release branches.

Additionally, would you mind adding some test cases for the new functionality? See components/heap/test directory for some of the heap-related test cases. For these changes, i think it would be good to create heap regions of different sizes inside the test case, and check that all operations still work as expected.

Finally, would you consider PR-ing these changes at https://github.com/mattconte/tlsf as well? We would like to not diverge too much from the upstream version, if possible, so that merging fixes from upstream doesn't become too difficult.

[philippe44]

I have an issue with rebasing on master... I'll try again later. Re also doing a PR to https://github.com/mattconte/tlsf, I can do that, but probably a bit later. It seems also that your code has largely diverged from the one there, no?

[edit]: for now, PR is not in order, I'm doing the move to master

[philippe44]

PR is now in order against master branch. Test cases still need to be done, but otherwise comments are welcome 😄.

• One question BTW: why did the control_t structure, which supposed to be opaque, moved to be heap_tlsf.h which exposes widely internal structure.

[philippe44]

PR submitted in the upstream version mattconte/tlsf#24 as well

[philippe44]

Let me know if you need something else as AFAIC, this PR is ready for review. I've also (per your request) added a specific PR to the TLSF repository from which it was inspired. I'm not sure the owner is still active though.

[sle118]

I wish this could get some traction!

[X-Ryl669]

@igrr Is there something preventing this PR to be merged ?
It seems tlsf is abandoned (no commit since 2020) so waiting on a dead project to merge a PR shouldn't block this feature, IMHO.

Loosing 10% of the available RAM as heap's algorithm overhead is too much to ignore a fix.

[X-Ryl669]

BTW, Changing the assert not to include the text is simple (see this patch, tlsf.c file) and saves a lot of flash for no downside, since in case the assert triggers, it contains the line and the source code line contains the text.

[sle118]

bump
Bump

[SoucheSouche]

The PR is conflicting with the latest master as the tlsf implementation was removed from the heap component and moved to a separate repository maintained as a fork of https://github.com/mattconte/tlsf. See 3737bf8.

@philippe44, I would suggest submitting mattconte/tlsf#24 in the espressif fork (https://github.com/espressif/tlsf). As some work is currently done in this repository. It would most definitely speed up the process of merging your work.

[philippe44]

Is it even worth doing it? It has been pending for so much time now that I'm not sure there any appetite for it.

[sle118]

I would have thought of this as a big improvement for projects that are pushing the platform, but I guess not everyone experiencing problems were able to figure out what the heck was going on

[philippe44]

@igrr understood. I wanted to make sure this PR was still of interest. I will see how to make a PR against your fork, but it might take me a couple of weeks.

[SoucheSouche]

@igrr understood. I wanted to make sure this PR was still of interest. I will see how to make a PR against your fork, but it might take me a couple of weeks.

Hi @philippe44, what is the status of the PR migration against the TLSF fork? Did you find some time to start working on it?

[X-Ryl669]

@philippe44 Please have a look at this commit, I've ported the patch to the latest master, it should merge directly.

[SoucheSouche]

@philippe44 Please have a look at this commit, I've ported the patch to the latest master, it should merge directly.

@X-Ryl669 your commit does not seem to be made against the upstream of the master branch of esp-idf. In the master branch (and since release v5.0) the tlsf implementation was moved out of esp-idf and put into espressif/tlsf. This PR should be made against the idf branch of the tlsf repository, not against the master branch of esp-idf.

[X-Ryl669]

Here's my port to both repositories: https://github.com/X-Ryl669/tlsf

And the patch to esp-idf master:

diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c
index 8481268d55..68d426246f 100644
--- a/components/heap/multi_heap.c
+++ b/components/heap/multi_heap.c
@@ -142,7 +142,7 @@ size_t multi_heap_get_allocated_size_impl(multi_heap_handle_t heap, void *p)
 multi_heap_handle_t multi_heap_register_impl(void *start_ptr, size_t size)
 {
     assert(start_ptr);
-    if(size < (tlsf_size() + tlsf_block_size_min() + sizeof(heap_t))) {
+    if(size < (tlsf_size(NULL) + tlsf_block_size_min() + sizeof(heap_t))) {
         //Region too small to be a heap.
         return NULL;
     }
@@ -150,13 +150,13 @@ multi_heap_handle_t multi_heap_register_impl(void *start_ptr, size_t size)
     heap_t *result = (heap_t *)start_ptr;
     size -= sizeof(heap_t);
 
-    result->heap_data = tlsf_create_with_pool(start_ptr + sizeof(heap_t), size);
+    result->heap_data = tlsf_create_with_pool(start_ptr + sizeof(heap_t), size, 0);
     if(!result->heap_data) {
         return NULL;
     }
 
     result->lock = NULL;
-    result->free_bytes = size - tlsf_size();
+    result->free_bytes = size - tlsf_size(result->heap_data);
     result->pool_size = size;
     result->minimum_free_bytes = result->free_bytes;
     return result;
@@ -417,7 +417,6 @@ static void multi_heap_get_info_tlsf(void* ptr, size_t size, int used, void* use
 
 void multi_heap_get_info_impl(multi_heap_handle_t heap, multi_heap_info_t *info)
 {
-    uint32_t sl_interval;
     uint32_t overhead;
 
     memset(info, 0, sizeof(multi_heap_info_t));
@@ -431,13 +430,10 @@ void multi_heap_get_info_impl(multi_heap_handle_t heap, multi_heap_info_t *info)
     /* TLSF has an overhead per block. Calculate the total amount of overhead, it shall not be
      * part of the allocated bytes */
     overhead = info->allocated_blocks * tlsf_alloc_overhead();
-    info->total_allocated_bytes = (heap->pool_size - tlsf_size()) - heap->free_bytes - overhead;
+    info->total_allocated_bytes = (heap->pool_size - tlsf_size(heap->heap_data)) - heap->free_bytes - overhead;
     info->minimum_free_bytes = heap->minimum_free_bytes;
     info->total_free_bytes = heap->free_bytes;
-    if (info->largest_free_block) {
-        sl_interval = (1 << (31 - __builtin_clz(info->largest_free_block))) / SL_INDEX_COUNT;
-        info->largest_free_block = info->largest_free_block & ~(sl_interval - 1);
-    }
+    info->largest_free_block = tlsf_fit_size(heap->heap_data, info->largest_free_block);
     multi_heap_internal_unlock(heap);
 }
 #endif
diff --git a/components/heap/test_multi_heap_host/test_multi_heap.cpp b/components/heap/test_multi_heap_host/test_multi_heap.cpp
index c3cac1cad1..893f310f66 100644
--- a/components/heap/test_multi_heap_host/test_multi_heap.cpp
+++ b/components/heap/test_multi_heap_host/test_multi_heap.cpp
@@ -8,6 +8,16 @@
 #include <string.h>
 #include <assert.h>
 
+static void *__malloc__(size_t bytes) 
+{
+    return malloc(bytes);
+}
+
+static void __free__(void *ptr) 
+{
+    free(ptr);
+}
+
 /* Insurance against accidentally using libc heap functions in tests */
 #undef free
 #define free #error
@@ -204,16 +214,18 @@ TEST_CASE("multi_heap defrag realloc", "[multi_heap]")
 #endif
 
 
-TEST_CASE("multi_heap many random allocations", "[multi_heap]")
+void multi_heap_allocation_impl(int heap_size)
 {
-    uint8_t big_heap[8 * 1024];
+    uint8_t *big_heap = (uint8_t *) __malloc__(2*heap_size);
     const int NUM_POINTERS = 64;
 
-    printf("Running multi-allocation test...\n");
+    printf("Running multi-allocation test with heap_size %d...\n", heap_size);
+
+    REQUIRE( big_heap );
+    multi_heap_handle_t heap = multi_heap_register(big_heap, heap_size);
 
     void *p[NUM_POINTERS] = { 0 };
     size_t s[NUM_POINTERS] = { 0 };
-    multi_heap_handle_t heap = multi_heap_register(big_heap, sizeof(big_heap));
 
     const size_t initial_free = multi_heap_free_size(heap);
 
@@ -241,13 +253,12 @@ TEST_CASE("multi_heap many random allocations", "[multi_heap]")
                 s[n] = new_size;
                 if (new_size > 0) {
                     REQUIRE( p[n] >= big_heap );
-                    REQUIRE( p[n] < big_heap + sizeof(big_heap) );
+                    REQUIRE( p[n] < big_heap + heap_size );
                     memset(p[n], n, new_size);
                 }
             }
             continue;
         }
-
         if (p[n] != NULL) {
             if (s[n] > 0) {
                 /* Verify pre-existing contents of p[n] */
@@ -271,14 +282,13 @@ TEST_CASE("multi_heap many random allocations", "[multi_heap]")
         printf("malloc %p (%zu)\n", p[n], s[n]);
         if (p[n] != NULL) {
             REQUIRE( p[n] >= big_heap );
-            REQUIRE( p[n] < big_heap + sizeof(big_heap) );
+            REQUIRE( p[n] < big_heap + heap_size );
         }
         if (!multi_heap_check(heap, true)) {
             printf("FAILED iteration %d after mallocing %p (%zu bytes)\n", i, p[n], s[n]);
             multi_heap_dump(heap);
             REQUIRE(0);
         }
-
         if (p[n] != NULL) {
             memset(p[n], n, s[n]);
         }
@@ -294,6 +304,15 @@ TEST_CASE("multi_heap many random allocations", "[multi_heap]")
     }
 
     REQUIRE( initial_free == multi_heap_free_size(heap) );
+    __free__(big_heap);
+}
+
+TEST_CASE("multi_heap many random allocations", "[multi_heap]")
+{
+    size_t poolsize[] = { 15, 255, 4095, 8191 };
+    for (size_t i = 0; i < sizeof(poolsize)/sizeof(size_t); i++) {
+        multi_heap_allocation_impl(poolsize[i] * 1024);
+    }  
 }
 
 TEST_CASE("multi_heap_get_info() function", "[multi_heap]")
diff --git a/components/heap/tlsf b/components/heap/tlsf
index ab17d6798d..3c0d5e7bc2 160000
--- a/components/heap/tlsf
+++ b/components/heap/tlsf
@@ -1 +1 @@
-Subproject commit ab17d6798d1561758827b6553d56d57f19aa4d66
+Subproject commit 3c0d5e7bc2c528ee02b3d0ba76a9c652f151e2fd

I've run the tests on the host, and it's failing on 3 of 58157 tests. I don't know if it's expected or not, so @philippe44 please confirm. It looks like the change in the heap control's structure is responsible for the failed asserts but I don't know what to expect here.

[philippe44]

What are the failing tests?

[SoucheSouche]

Here's my port to both repositories: https://github.com/X-Ryl669/tlsf

And the patch to esp-idf master:

Hi @X-Ryl669, thank you for the port. I finished checking your changes against espressif/tlsf and they seem good. Could you create the PR on the repo?

But before doing that, I would suggest reverting your changes made to tlsf_assert() (i.e, using the strings as comments). I can see that this was discussed a while back in this issue and was closed as 'won't do'. Those changes are not related to this PR and should be discussed in a separate issue/PR.

[X-Ryl669]

@philippe44 :

./esp-idf/components/heap/test_multi_heap_host/test_multi_heap.cpp:91: FAILED:
  REQUIRE( multi_heap_malloc(heap, alloc_size * 5) == __null )
with expansion:c data, size: 92 bytes, Free: Yes 
  0xffb72b98 == 0data, size: 32 bytes, Free: No 
[...]
./esp-idf/components/heap/test_multi_heap_host/test_multi_heap.cpp:306: FAILED:
  REQUIRE( initial_free == multi_heap_free_size(heap) )
with expansion:
  260160 (0x3f840) == 258756 (0x3f2c4)
[...]
./esp-idf/components/heap/test_multi_heap_host/test_multi_heap.cpp:599: FAILED:
  REQUIRE( is_heap_ok == true )
with expansion:
  false == true
[...]

[X-Ryl669]

@SoucheSouche I'm waiting for confirmation that the code doesn't break anything, and the original author was Philippe, not me.

I've updated my fork without the string replacement in asserts.

[philippe44]

@X-Ryl669: I'll check why these test fails, it's strange indeed. For the string replacement, I think I remember this indeed has been discussed and there was another way to not have these consume memory, so I'm of course fine with what @SoucheSouche would prefer there.

Thanks very much for taking the time to port that modification to the new esp-idf, this is very kind of you

[X-Ryl669]

As a side note, even when building with COMPILER_OPTIMIZATION_ASSERTIONS_SILENT=y, you'll end up with complete file source path in the binary as shown here (but no more the assert'ed text):

$ strings build/some.elf | grep "\.c"
[...]
/home/[...]/pthread.c
/builds/idf/crosstool-NG/.build/xtensa-esp32-elf/src/newlib/newlib/libc/stdio/flags.c
/builds/idf/crosstool-NG/.build/xtensa-esp32-elf/src/newlib/newlib/libc/locale/localeconv.c
/builds/idf/crosstool-NG/.build/xtensa-esp32-elf/src/newlib/newlib/libc/stdlib/mprec.c
/builds/idf/crosstool-NG/.build/xtensa-esp32-elf/src/newlib/newlib/libm/math/s_frexp.c
/builds/idf/crosstool-NG/.build/xtensa-esp32-elf/src/newlib/newlib/libc/locale/locale.c
/builds/idf/crosstool-NG/.build/xtensa-esp32-elf/src/newlib/newlib/libc/stdlib/mbtowc_r.c
esp_clk.c
esp_app_desc.c
pthread.c
cpu_start.c
cache_err_int.c
[...]

What's worst, is that files appear twice in the binary, once with the full path and once without.
I guess the assert macro is broken in esp-idf now, as I'm seeing:

/* __FILENAME__ points to the file name instead of path + filename
 * e.g __FILE__ points to "/apps/test.c" where as __FILENAME__ points to "test.c"
 */
#define __FILENAME__ (__builtin_strrchr( "/" __FILE__, '/') + 1)

This doesn't strip the __FILE__ from the binary, but simply store the __FILE__ in the binary plus a pointer to the basename of the file. So what appears as an flash size optimization make the situation worse.
To only capture the filename, it can be done, but only with C++ tricks, see my answer here

Anyway, that's should be part of another PR...

[SoucheSouche]

I am not sure about the reason why the first 2 tests are failing but I will have a look as well. But for the last one ./esp-idf/components/heap/test_multi_heap_host/test_multi_heap.cpp:599: FAILED:, this test was added recently and was not updated to the new implementation. It calculates some offset in the heap based on the size of control_t which doesn't seem relevant anymore.

[SoucheSouche]

Hello @philippe44, @X-Ryl669,

I used @X-Ryl669 work on the TLSF submodule to incorporate this PR into the upstream and investigate the failed tests.
As suspected all the failed tests were due to the changes in metadata overhead in created TLSF heaps. I fixed them locally and also ran the set of target tests successfully.

What I suggest doing from there is for @X-Ryl669 to create the PR in the TLSF submodule after incorporating the patch below.
multiple_heap_tlsf.zip

I will then create an MR in ESP-IDF to merge the changes from TLSF submodule in the upstream and backport those changes to the release/v5.0 as well (since the TLSF is also a submodule in this release).

Referencing this MR, I will also create a backport to release/v4.4 using the work of @philippe44 from this PR since it targets a version of ESP-IDF where the TLSF implementation was not done in a separate submodule.

In this manner, both contributions will be used to serve the implementation of this new feature.
Let me know if you agree with the steps I mentioned.

[X-Ryl669]

I've applied your patch, but it doesn't contain any change for the failing tests. Did you miss some commit ?

Anyway, here's the PR in tlsf repository, I've allowed changes by maintainers, you'll need to fix it either there or here.

[SoucheSouche]

I've applied your patch, but it doesn't contain any change for the failing tests. Did you miss some commit ?

Anyway, here's the PR in tlsf repository, I've allowed changes by maintainers, you'll need to fix it either there or here.

Nothing needs to be fixed in the TLSF implementation concerning the failing tests. Only the tests themselves need updates but I will update them on the MR I will create in ESP-IDF after I merge your PR in the TLSF submodule.

[SoucheSouche]

Hello everyone,

The feature ported to the TLSF repository by @X-Ryl669 will be merged this week hopefully.
After reviewing the code I added a couple of commits on top of the original ones.

• I added a couple of input validation in the functions that are now taking a tlsf_t as input to check for null pointer
• I updated the control_construct() function as follow:
  • the fl_index_max is calculated to always fit the requested heap size in the tightest way possible. This implies that the FL_INDEX_MAX_MIN was removed and tlsf_size() now expects a tlsf_t pointer to return a size. The metadata overhead (sl_bitmaps and blocks) will therefore be as small as possible in any case. This also means that the control_construct() now does an input validation on the size passed as parameter to make sure that the metadata actually fit in the requested size and leave enough space to also allocate a small block of memory.

I will reflect those changes when creating the backport to 4.4 from @philippe44 work as well.

[SoucheSouche]

Hi everyone, the PR on the TLSF repository was merged. I will now follow up on the ESP-IDF PRs and backports.

[SoucheSouche]

Hi everyone, The feature was merged to master and v4.4 on GitLab. It will be available on those respective branches after syncing the GitHub mirror. The backport to v5.0 8s still on hold.

[zikalino]

ok, so we can close this pr