From 90ac786cf460847a90829cf71d7a871cff41e591 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 13 Oct 2022 10:02:29 +0200 Subject: [PATCH 1/4] heap: Update the component to incorporate the new TLSF implementation - 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 https://github.com/espressif/esp-idf/pull/7829 * contributes to fix https://github.com/espressif/esp-idf/issues/7822 --- components/heap/CMakeLists.txt | 5 ---- components/heap/multi_heap.c | 17 ++++++----- components/heap/tlsf | 2 +- components/heap/tlsf_platform.h | 51 --------------------------------- 4 files changed, 9 insertions(+), 66 deletions(-) delete mode 100644 components/heap/tlsf_platform.h diff --git a/components/heap/CMakeLists.txt b/components/heap/CMakeLists.txt index a05c1bc4dbe..61cb81219a3 100644 --- a/components/heap/CMakeLists.txt +++ b/components/heap/CMakeLists.txt @@ -8,11 +8,6 @@ set(includes "include") if(NOT CONFIG_HEAP_TLSF_USE_ROM_IMPL) set(priv_includes "tlsf") list(APPEND srcs "tlsf/tlsf.c") - if(NOT CMAKE_BUILD_EARLY_EXPANSION) - set_source_files_properties(tlsf/tlsf.c - PROPERTIES COMPILE_FLAGS - "-include ../tlsf_platform.h") - endif() endif() if(NOT CONFIG_HEAP_POISONING_DISABLED) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 8481268d553..5be09bb8c2a 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,16 @@ 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); + /* Do not specify any maximum size for the allocations so that the default configuration is used */ + const size_t max_bytes = 0; + + result->heap_data = tlsf_create_with_pool(start_ptr + sizeof(heap_t), size, max_bytes); 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 +420,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 +433,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/tlsf b/components/heap/tlsf index ab17d6798d1..13da0fff7f5 160000 --- a/components/heap/tlsf +++ b/components/heap/tlsf @@ -1 +1 @@ -Subproject commit ab17d6798d1561758827b6553d56d57f19aa4d66 +Subproject commit 13da0fff7f54623b20785dbeb466df420f52e510 diff --git a/components/heap/tlsf_platform.h b/components/heap/tlsf_platform.h deleted file mode 100644 index f40e0ca0f9d..00000000000 --- a/components/heap/tlsf_platform.h +++ /dev/null @@ -1,51 +0,0 @@ -/* - * SPDX-FileCopyrightText: 2017-2022 Espressif Systems (Shanghai) CO LTD - * - * SPDX-License-Identifier: Apache-2.0 - */ -#pragma once - -#include -#include - -#ifdef __cplusplus -extern "C" { -#endif - -#ifdef ESP_PLATFORM -#include "soc/soc.h" - -#if !CONFIG_SPIRAM -#define TLSF_MAX_POOL_SIZE (SOC_DIRAM_DRAM_HIGH - SOC_DIRAM_DRAM_LOW) -#else -#define TLSF_MAX_POOL_SIZE SOC_EXTRAM_DATA_SIZE -#endif -#endif - -#if (TLSF_MAX_POOL_SIZE <= (256 * 1024)) -#define FL_INDEX_MAX_PLATFORM 18 //Each pool can have up 256KB -#elif (TLSF_MAX_POOL_SIZE <= (512 * 1024)) -#define FL_INDEX_MAX_PLATFORM 19 //Each pool can have up 512KB -#elif (TLSF_MAX_POOL_SIZE <= (1 * 1024 * 1024)) -#define FL_INDEX_MAX_PLATFORM 20 //Each pool can have up 1MB -#elif (TLSF_MAX_POOL_SIZE <= (2 * 1024 * 1024)) -#define FL_INDEX_MAX_PLATFORM 21 //Each pool can have up 2MB -#elif (TLSF_MAX_POOL_SIZE <= (4 * 1024 * 1024)) -#define FL_INDEX_MAX_PLATFORM 22 //Each pool can have up 4MB -#elif (TLSF_MAX_POOL_SIZE <= (8 * 1024 * 1024)) -#define FL_INDEX_MAX_PLATFORM 23 //Each pool can have up 8MB -#elif (TLSF_MAX_POOL_SIZE <= (16 * 1024 * 1024)) -#define FL_INDEX_MAX_PLATFORM 24 //Each pool can have up 16MB -#elif (TLSF_MAX_POOL_SIZE <= (32 * 1024 * 1024)) -#define FL_INDEX_MAX_PLATFORM 25 //Each pool can have up 32MB -#else -#error "Higher TLSF pool sizes should be added for this new config" -#endif - -/* Include from the TLSF submodule to force TLSF_INDEX_MAX_PLATFORM to be defined - * when the TLSF repository is compiled in the IDF environment. */ -#include "tlsf_common.h" - -#ifdef __cplusplus -} -#endif From 7de6565722b09362fdd2fc8c9d6f3e6eb6aacea4 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 13 Oct 2022 10:03:33 +0200 Subject: [PATCH 2/4] heap: fix comment and return condition in heap_caps_check_add_region_allowed() (See cd805a5ab1c8299ab931edf40777fcc7912bd6ca) --- components/heap/heap_caps_init.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 2b532b6e2aa..2a513f46198 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -170,13 +170,13 @@ esp_err_t heap_caps_add_region(intptr_t start, intptr_t end) bool heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, intptr_t start, intptr_t end) { /* - * We assume that in any region, the "start" must be stictly less than the end. + * We assume that in any region, the "start" must be strictly less than the end. * Specially, the 3rd scenario can be allowed. For example, allocate memory from heap, * then change the capability and call this function to create a new region for special * application. * This 'start = start' and 'end = end' scenario is incorrect because the same region - * cannot be add twice. For example, add the .bss memory to region twice, if not do the - * check, it will cause exception. + * cannot be added twice. In fact, registering the same memory region as a heap twice + * would cause a corruption and then an exception at runtime. * * the existing heap region s(tart) e(nd) * |----------------------| @@ -201,7 +201,7 @@ bool heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, bool condition_4 = start < heap_end && end > heap_end; // if true then region not allowed bool condition_6 = start == heap_start && end == heap_end; // if true then region not allowed - return (condition_2 || condition_4 || condition_6) ? false: true; + return !(condition_2 || condition_4 || condition_6); } esp_err_t heap_caps_add_region_with_caps(const uint32_t caps[], intptr_t start, intptr_t end) From 6f11d0f2970cb9e82d1b71b05d06f7b51653f753 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 13 Oct 2022 10:05:53 +0200 Subject: [PATCH 3/4] heap: Update target and host tests after incorporation of the new TLSF implementation - update the target and host tests to consider the new TLSF api and the metadata size only when the target is not using the rom implementation of the TLSF --- .../test_apps/main/test_runtime_heap_reg.c | 27 +++---- .../test_multi_heap_host/test_multi_heap.cpp | 78 +++++++++++++------ 2 files changed, 65 insertions(+), 40 deletions(-) diff --git a/components/heap/test_apps/main/test_runtime_heap_reg.c b/components/heap/test_apps/main/test_runtime_heap_reg.c index 3ecbdf74a5a..283b7a082f8 100644 --- a/components/heap/test_apps/main/test_runtime_heap_reg.c +++ b/components/heap/test_apps/main/test_runtime_heap_reg.c @@ -15,18 +15,16 @@ #include "esp_system.h" #include "heap_memory_layout.h" -#include "../tlsf/tlsf.h" - extern void set_leak_threshold(int threshold); /* NOTE: This is not a well-formed unit test, it leaks memory */ TEST_CASE("Allocate new heap at runtime", "[heap]") { - // 84 bytes of overhead to account for multi_heap structs and eventual - // poisoning bytes + size of control_t from tlsf - const size_t HEAP_OVERHEAD_MAX = tlsf_size() + 84; - const size_t MIN_HEAP_SIZE = HEAP_OVERHEAD_MAX + tlsf_block_size_min(); - const size_t BUF_SZ = MIN_HEAP_SIZE; + /* The value of the heap overhead is calculated for the worst case scenario + * where the tlsf has a size of metadata fixed at runtime. + */ + const size_t HEAP_OVERHEAD_MAX = 3248; + const size_t BUF_SZ = 3500; void *buffer = malloc(BUF_SZ); TEST_ASSERT_NOT_NULL(buffer); @@ -46,12 +44,8 @@ TEST_CASE("Allocate new heap at runtime", "[heap]") */ TEST_CASE("Allocate new heap with new capability", "[heap]") { - // 84 bytes of overhead to account for multi_heap structs and eventual - // poisoning bytes + size of control_t from tlsf - const size_t HEAP_OVERHEAD = tlsf_size() + 84; - const size_t MIN_HEAP_SIZE = HEAP_OVERHEAD + tlsf_block_size_min(); - const size_t BUF_SZ = MIN_HEAP_SIZE; - const size_t ALLOC_SZ = tlsf_block_size_min(); + const size_t BUF_SZ = 3500; + const size_t ALLOC_SZ = 64; const uint32_t MALLOC_CAP_INVENTED = (1 << 30); /* this must be unused in esp_heap_caps.h */ @@ -67,7 +61,7 @@ TEST_CASE("Allocate new heap with new capability", "[heap]") TEST_ASSERT_NOT_NULL( heap_caps_malloc(ALLOC_SZ, MALLOC_CAP_INVENTED) ); // set the leak threshold to a bigger value as this test leaks memory - set_leak_threshold(-3000); + set_leak_threshold(-4000); } /* NOTE: This is not a well-formed unit test. @@ -76,8 +70,11 @@ TEST_CASE("Allocate new heap with new capability", "[heap]") TEST_CASE("Add .bss memory to heap region runtime", "[heap]") { +/* The value of the heap overhead is calculated for the worst case scenario + * where the tlsf has a size of metadata fixed at runtime. + */ #define HEAP_OVERHEAD_MAX 3248 -#define BUF_SZ 3260 +#define BUF_SZ 3500 static uint8_t s_buffer[BUF_SZ]; printf("s_buffer start %08x end %08x\n", (intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ); 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 c3cac1cad17..a892681c7f9 100644 --- a/components/heap/test_multi_heap_host/test_multi_heap.cpp +++ b/components/heap/test_multi_heap_host/test_multi_heap.cpp @@ -2,12 +2,30 @@ #include "multi_heap.h" #include "../multi_heap_config.h" +#include "../tlsf/tlsf.h" #include "../tlsf/tlsf_common.h" #include "../tlsf/tlsf_block_functions.h" #include #include +/* The functions __malloc__ and __free__ are used to call the libc + * malloc and free and allocate memory from the host heap. Since the test + * `TEST_CASE("multi_heap many random allocations", "[multi_heap]")` + * calls multi_heap_allocation_impl() with sizes that can go up to 8MB, + * an allocatation on the heap will be prefered rather than the stack which + * might not have the necessary memory. + */ +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 @@ -61,10 +79,11 @@ TEST_CASE("multi_heap simple allocations", "[multi_heap]") TEST_CASE("multi_heap fragmentation", "[multi_heap]") { - uint8_t small_heap[4 * 1024]; + const size_t HEAP_SIZE = 4 * 1024; + uint8_t small_heap[HEAP_SIZE]; multi_heap_handle_t heap = multi_heap_register(small_heap, sizeof(small_heap)); - const size_t alloc_size = 128; + const size_t alloc_size = 500; void *p[4]; for (int i = 0; i < 4; i++) { @@ -204,20 +223,22 @@ 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__(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); - const int ITERATIONS = 10000; + const int ITERATIONS = 5000; for (int i = 0; i < ITERATIONS; i++) { /* check all pointers allocated so far are valid inside big_heap */ @@ -228,11 +249,11 @@ TEST_CASE("multi_heap many random allocations", "[multi_heap]") uint8_t n = rand() % NUM_POINTERS; - if (rand() % 4 == 0) { + if (i % 4 == 0) { /* 1 in 4 iterations, try to realloc the buffer instead of using malloc/free */ - size_t new_size = rand() % 1024; + size_t new_size = (rand() % 1023) + 1; void *new_p = multi_heap_realloc(heap, p[n], new_size); printf("realloc %p -> %p (%zu -> %zu)\n", p[n], new_p, s[n], new_size); multi_heap_check(heap, true); @@ -241,13 +262,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 +291,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 +313,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]") @@ -393,8 +421,9 @@ TEST_CASE("multi_heap minimum-size allocations", "[multi_heap]") TEST_CASE("multi_heap_realloc()", "[multi_heap]") { + const size_t HEAP_SIZE = 4 * 1024; const uint32_t PATTERN = 0xABABDADA; - uint8_t small_heap[4 * 1024]; + uint8_t small_heap[HEAP_SIZE]; multi_heap_handle_t heap = multi_heap_register(small_heap, sizeof(small_heap)); uint32_t *a = (uint32_t *)multi_heap_malloc(heap, 64); @@ -404,7 +433,6 @@ TEST_CASE("multi_heap_realloc()", "[multi_heap]") REQUIRE( b > a); /* 'b' takes the block after 'a' */ *a = PATTERN; - uint32_t *c = (uint32_t *)multi_heap_realloc(heap, a, 72); REQUIRE( multi_heap_check(heap, true)); REQUIRE( c != NULL ); @@ -414,13 +442,12 @@ TEST_CASE("multi_heap_realloc()", "[multi_heap]") #ifndef MULTI_HEAP_POISONING_SLOW // "Slow" poisoning implementation doesn't reallocate in place, so these // test will fail... - uint32_t *d = (uint32_t *)multi_heap_realloc(heap, c, 36); REQUIRE( multi_heap_check(heap, true) ); REQUIRE( c == d ); /* 'c' block should be shrunk in-place */ REQUIRE( *d == PATTERN); - - uint32_t *e = (uint32_t *)multi_heap_malloc(heap, 64); + // biggest allocation possible to completely fill the block left free after it was reallocated + uint32_t *e = (uint32_t *)multi_heap_malloc(heap, 60); REQUIRE( multi_heap_check(heap, true)); REQUIRE( a == e ); /* 'e' takes the block formerly occupied by 'a' */ @@ -429,11 +456,7 @@ TEST_CASE("multi_heap_realloc()", "[multi_heap]") REQUIRE( multi_heap_check(heap, true) ); REQUIRE( f == b ); /* 'b' should be extended in-place, over space formerly occupied by 'd' */ -#ifdef MULTI_HEAP_POISONING -#define TOO_MUCH 7420 + 1 -#else -#define TOO_MUCH 7420 + 1 -#endif +#define TOO_MUCH HEAP_SIZE + 1 /* not enough contiguous space left in the heap */ uint32_t *g = (uint32_t *)multi_heap_realloc(heap, e, TOO_MUCH); REQUIRE( g == NULL ); @@ -443,7 +466,8 @@ TEST_CASE("multi_heap_realloc()", "[multi_heap]") g = (uint32_t *)multi_heap_realloc(heap, e, 128); REQUIRE( multi_heap_check(heap, true) ); REQUIRE( e == g ); /* 'g' extends 'e' in place, into the space formerly held by 'f' */ -#endif + +#endif // MULTI_HEAP_POISONING_SLOW } // TLSF only accepts heaps aligned to 4-byte boundary so @@ -542,8 +566,12 @@ TEST_CASE("multi_heap poisoning detection", "[multi_heap]") /* register the heap memory. One free block only will be available */ multi_heap_handle_t heap = multi_heap_register(heap_mem, HEAP_SIZE); + control_t *tlsf_ptr = (control_t*)(heap_mem + 20); + const size_t control_t_size = tlsf_ptr->size; + const size_t heap_t_size = 20; + /* offset in memory at which to find the first free memory byte */ - const size_t free_memory_offset = sizeof(multi_heap_info_t) + sizeof(control_t) + block_header_overhead; + const size_t free_memory_offset = heap_t_size + control_t_size + sizeof(block_header_t) - block_header_overhead; /* block header of the free block under test in the heap () */ const block_header_t* block = (block_header_t*)(heap_mem + free_memory_offset - sizeof(block_header_t)); From 508194be50882c52bb698671be23e2022152a590 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 13 Oct 2022 16:00:09 +0200 Subject: [PATCH 4/4] heap: Remove size check in multi_heap.c when registering a new heap The tlsf now checks for size validity when creating a new heap. The check previously done in multi_heap_register_impl() is no longer valid since the tlsf_size() is not known at this time (as the metadata size is linked ot the size of the memory region passed as parameter when calling tlsf_create_with_pool()) The tlsf_create_with_pool() will return a null pointer if the size of the memory is not big enough to hold the metadata overhead and at least a small block. Update the test according to the changes in TLSF API --- components/heap/multi_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 5be09bb8c2a..09b29e6fc8c 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(NULL) + tlsf_block_size_min() + sizeof(heap_t))) { + if(size < (sizeof(heap_t))) { //Region too small to be a heap. return NULL; }