Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

philippe44
Copy link

See #7822

@CLAassistant
Copy link

CLAassistant commented Nov 4, 2021

CLA assistant check
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.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 4, 2021
@github-actions github-actions bot changed the title allow each zone of memory allocated to heap to have different control block allow each zone of memory allocated to heap to have different control block (IDFGH-6152) Nov 4, 2021
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@igrr
Copy link
Member

igrr commented Nov 4, 2021

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
Copy link
Author

philippe44 commented Nov 4, 2021

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 philippe44 changed the base branch from release/v4.3 to master November 4, 2021 22:08
@philippe44
Copy link
Author

philippe44 commented Nov 5, 2021

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
Copy link
Author

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

@philippe44
Copy link
Author

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
Copy link

sle118 commented Nov 20, 2021

I wish this could get some traction!

@X-Ryl669
Copy link
Contributor

@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
Copy link
Contributor

X-Ryl669 commented Jan 1, 2022

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
Copy link

sle118 commented May 22, 2022

bump
Bump

@SoucheSouche
Copy link
Collaborator

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
Copy link
Author

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
Copy link

sle118 commented Aug 12, 2022

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
Copy link
Author

@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.

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Aug 22, 2022
@SoucheSouche
Copy link
Collaborator

@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
Copy link
Contributor

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

@SoucheSouche
Copy link
Collaborator

@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
Copy link
Contributor

X-Ryl669 commented Sep 19, 2022

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
Copy link
Author

What are the failing tests?

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Selected for Development Issue is selected for development labels Sep 20, 2022
@SoucheSouche
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Contributor

X-Ryl669 commented Sep 20, 2022

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

espressif-bot pushed a commit that referenced this pull request Oct 27, 2022
- remove tlsf_platform.h from esp-idf since the fl_index is now calculated
  based on the size of the requested heap
- update CMakeLists.txt accordingly

* based on the changes made to the TLSF in #7829
* contributes to fix #7822
@SoucheSouche
Copy link
Collaborator

SoucheSouche commented Nov 25, 2022

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.

@espressif-bot espressif-bot added Resolution: Done Issue is done internally Status: Done Issue is done internally and removed Status: In Progress Work is in progress labels Dec 23, 2022
@zikalino
Copy link
Contributor

ok, so we can close this pr

@zikalino zikalino closed this Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants