From 5a1cb7118498cee380795532c46456d5a931e42a Mon Sep 17 00:00:00 2001 From: armando Date: Mon, 17 Mar 2025 11:19:23 +0800 Subject: [PATCH 1/3] fix(mmu): fixed esp_mmu_map concurrent issue and add related docs --- .../test_apps/mspi/main/test_app_main.c | 2 +- components/esp_mm/esp_mmu_map.c | 42 ++++++++++++------- components/esp_mm/include/esp_mmu_map.h | 4 -- docs/en/api-reference/system/mm.rst | 4 +- docs/zh_CN/api-reference/system/mm.rst | 4 +- 5 files changed, 34 insertions(+), 22 deletions(-) diff --git a/components/esp_hw_support/test_apps/mspi/main/test_app_main.c b/components/esp_hw_support/test_apps/mspi/main/test_app_main.c index 3b4a02ffbc..44ef580099 100644 --- a/components/esp_hw_support/test_apps/mspi/main/test_app_main.c +++ b/components/esp_hw_support/test_apps/mspi/main/test_app_main.c @@ -10,7 +10,7 @@ #include "esp_newlib.h" // load partition table in tests will use memory -#define TEST_MEMORY_LEAK_THRESHOLD (450) +#define TEST_MEMORY_LEAK_THRESHOLD (600) void setUp(void) { diff --git a/components/esp_mm/esp_mmu_map.c b/components/esp_mm/esp_mmu_map.c index 80b99772df..32dadf7a82 100644 --- a/components/esp_mm/esp_mmu_map.c +++ b/components/esp_mm/esp_mmu_map.c @@ -9,6 +9,7 @@ #include #include #include +#include #include "sdkconfig.h" #include "esp_attr.h" #include "esp_log.h" @@ -106,6 +107,10 @@ typedef struct { * Only the first `num_regions` items are valid */ mem_region_t mem_regions[SOC_MMU_LINEAR_ADDRESS_REGION_NUM]; + /** + * driver layer mutex lock + */ + _lock_t mutex; } mmu_ctx_t; static mmu_ctx_t s_mmu_ctx; @@ -443,17 +448,15 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target, ESP_RETURN_ON_FALSE((paddr_start % CONFIG_MMU_PAGE_SIZE == 0), ESP_ERR_INVALID_ARG, TAG, "paddr must be rounded up to the nearest multiple of CONFIG_MMU_PAGE_SIZE"); ESP_RETURN_ON_ERROR(s_mem_caps_check(caps), TAG, "invalid caps"); + _lock_acquire(&s_mmu_ctx.mutex); + mem_block_t *dummy_head = NULL; + mem_block_t *dummy_tail = NULL; size_t aligned_size = ALIGN_UP_BY(size, CONFIG_MMU_PAGE_SIZE); int32_t found_region_id = s_find_available_region(s_mmu_ctx.mem_regions, s_mmu_ctx.num_regions, aligned_size, caps, target); - if (found_region_id == -1) { - ESP_EARLY_LOGE(TAG, "no such vaddr range"); - return ESP_ERR_NOT_FOUND; - } + ESP_GOTO_ON_FALSE(found_region_id != -1, ESP_ERR_NOT_FOUND, err, TAG, "no such vaddr range"); //Now we're sure we can find an available block inside a certain region mem_region_t *found_region = &s_mmu_ctx.mem_regions[found_region_id]; - mem_block_t *dummy_head = NULL; - mem_block_t *dummy_tail = NULL; mem_block_t *new_block = NULL; if (TAILQ_EMPTY(&found_region->mem_block_head)) { @@ -502,7 +505,6 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target, } if (is_enclosed) { - ESP_LOGW(TAG, "paddr block is mapped already, vaddr_start: %p, size: 0x%x", (void *)mem_block->vaddr_start, mem_block->size); /* * This condition is triggered when `s_is_enclosed` is true and hence * we are sure that `paddr_start` >= `mem_block->paddr_start`. @@ -512,12 +514,11 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target, */ const uint32_t new_paddr_offset = paddr_start - mem_block->paddr_start; *out_ptr = (void *)mem_block->vaddr_start + new_paddr_offset; - return ESP_ERR_INVALID_STATE; + ESP_GOTO_ON_FALSE(false, ESP_ERR_INVALID_STATE, err, TAG, "paddr block is mapped already, vaddr_start: %p, size: 0x%x", (void *)mem_block->vaddr_start, mem_block->size); } if (!allow_overlap && is_overlapped) { - ESP_LOGE(TAG, "paddr block is overlapped with an already mapped paddr block"); - return ESP_ERR_INVALID_ARG; + ESP_GOTO_ON_FALSE(false, ESP_ERR_INVALID_ARG, err, TAG, "paddr block is overlapped with an already mapped paddr block"); } #endif //#if ENABLE_PADDR_CHECK @@ -552,7 +553,6 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target, assert(found); //insert the to-be-mapped new block to the list TAILQ_INSERT_BEFORE(found_block, new_block, entries); - //Finally, we update the max_slot_size found_region->max_slot_size = max_slot_len; @@ -574,6 +574,7 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target, //do mapping s_do_mapping(target, new_block->vaddr_start, paddr_start, aligned_size); *out_ptr = (void *)new_block->vaddr_start; + _lock_release(&s_mmu_ctx.mutex); return ESP_OK; @@ -584,6 +585,7 @@ err: if (dummy_head) { free(dummy_head); } + _lock_release(&s_mmu_ctx.mutex); return ret; } @@ -626,11 +628,13 @@ esp_err_t esp_mmu_unmap(void *ptr) { ESP_RETURN_ON_FALSE(ptr, ESP_ERR_INVALID_ARG, TAG, "null pointer"); + esp_err_t ret = ESP_FAIL; mem_region_t *region = NULL; mem_block_t *mem_block = NULL; uint32_t ptr_laddr = mmu_ll_vaddr_to_laddr((uint32_t)ptr); size_t slot_len = 0; + _lock_acquire(&s_mmu_ctx.mutex); for (int i = 0; i < s_mmu_ctx.num_regions; i++) { ESP_COMPILER_DIAGNOSTIC_PUSH_IGNORE("-Wanalyzer-out-of-bounds") if (ptr_laddr >= s_mmu_ctx.mem_regions[i].free_head && ptr_laddr < s_mmu_ctx.mem_regions[i].end) { @@ -638,7 +642,7 @@ esp_err_t esp_mmu_unmap(void *ptr) } ESP_COMPILER_DIAGNOSTIC_POP("-Wanalyzer-out-of-bounds") } - ESP_RETURN_ON_FALSE(region, ESP_ERR_NOT_FOUND, TAG, "munmap target pointer is outside external memory regions"); + ESP_GOTO_ON_FALSE(region, ESP_ERR_NOT_FOUND, err, TAG, "munmap target pointer is outside external memory regions"); bool found = false; mem_block_t *found_block = NULL; @@ -659,15 +663,23 @@ esp_err_t esp_mmu_unmap(void *ptr) } } - ESP_RETURN_ON_FALSE(found, ESP_ERR_NOT_FOUND, TAG, "munmap target pointer isn't mapped yet"); + if (found) { + TAILQ_REMOVE(®ion->mem_block_head, found_block, entries); + } + ESP_GOTO_ON_FALSE(found, ESP_ERR_NOT_FOUND, err, TAG, "munmap target pointer isn't mapped yet"); //do unmap s_do_unmapping(mem_block->vaddr_start, mem_block->size); - //remove the already unmapped block from the list - TAILQ_REMOVE(®ion->mem_block_head, found_block, entries); free(found_block); + _lock_release(&s_mmu_ctx.mutex); + return ESP_OK; + +err: + + _lock_release(&s_mmu_ctx.mutex); + return ret; } esp_err_t esp_mmu_map_dump_mapped_blocks(FILE* stream) diff --git a/components/esp_mm/include/esp_mmu_map.h b/components/esp_mm/include/esp_mmu_map.h index 2e64a0d842..e9efbb96b2 100644 --- a/components/esp_mm/include/esp_mmu_map.h +++ b/components/esp_mm/include/esp_mmu_map.h @@ -60,8 +60,6 @@ typedef uint32_t esp_paddr_t; /** * @brief Map a physical memory block to external virtual address block, with given capabilities. * - * @note This API does not guarantee thread safety - * * @param[in] paddr_start Start address of the physical memory block * @param[in] size Size to be mapped. Size will be rounded up by to the nearest multiple of MMU page size * @param[in] target Physical memory target you're going to map to, see `mmu_target_t` @@ -88,8 +86,6 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target, /** * @brief Unmap a previously mapped virtual memory block * - * @note This API does not guarantee thread safety - * * @param[in] ptr Start address of the virtual memory * * @return diff --git a/docs/en/api-reference/system/mm.rst b/docs/en/api-reference/system/mm.rst index febafbca3d..8f702216ff 100644 --- a/docs/en/api-reference/system/mm.rst +++ b/docs/en/api-reference/system/mm.rst @@ -169,7 +169,9 @@ SPI flash can be accessed by SPI1 (ESP-IDF ``esp_flash`` driver APIs), or by poi Thread Safety ============= -APIs in ``esp_mmu_map.h`` are not guaranteed to be thread-safe. +Following APIs in ``esp_mmu_map.h`` are not guaranteed to be thread-safe: + +- :cpp:func:`esp_mmu_map_dump_mapped_blocks` APIs in ``esp_cache.h`` are guaranteed to be thread-safe. diff --git a/docs/zh_CN/api-reference/system/mm.rst b/docs/zh_CN/api-reference/system/mm.rst index 32d5e459aa..b8cb03c004 100644 --- a/docs/zh_CN/api-reference/system/mm.rst +++ b/docs/zh_CN/api-reference/system/mm.rst @@ -169,7 +169,9 @@ SPI flash 可以通过 SPI1（ESP-IDF ``spi_flash`` 驱动 API）或指针进行 线程安全 ======== -``esp_mmu_map.h`` 中的 API 不能确保线程的安全性。 +``esp_mmu_map.h`` 中的以下 API 不能确保线程的安全性： + +- :cpp:func:`esp_mmu_map_dump_mapped_blocks` ``esp_cache.h`` 中的 API 能够确保线程的安全性。 From 6abf6f51963a23a6ec4479f4e47d98f94a8c9c01 Mon Sep 17 00:00:00 2001 From: armando Date: Mon, 17 Mar 2025 11:20:05 +0800 Subject: [PATCH 2/3] test(mmu): added esp_mmu_map concurrency test --- .../esp_mm/test_apps/mm/main/test_mmap.c | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/components/esp_mm/test_apps/mm/main/test_mmap.c b/components/esp_mm/test_apps/mm/main/test_mmap.c index e0ee721d1b..d62ef1db81 100644 --- a/components/esp_mm/test_apps/mm/main/test_mmap.c +++ b/components/esp_mm/test_apps/mm/main/test_mmap.c @@ -8,6 +8,9 @@ #include #include #include "inttypes.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" +#include "freertos/semphr.h" #include "esp_log.h" #include "esp_attr.h" #include "unity.h" @@ -71,6 +74,59 @@ TEST_CASE("Can find paddr caps by any paddr offset", "[mmu]") TEST_ESP_OK(esp_mmu_unmap(ptr0)); } +typedef struct { + SemaphoreHandle_t done; +} test_task_arg_t; + +void map_task(void *varg) +{ + esp_err_t ret = ESP_FAIL; + test_task_arg_t* args = (test_task_arg_t*) varg; + + const esp_partition_t *part = s_get_partition(); + srand(199); + int cnt = 0; + while (true) { + if (cnt >= 10000) { + break; + } + size_t i = rand() % part->size; + void *ptr0 = NULL; + size_t phys_addr = part->address + i; + size_t region_offset = phys_addr & (CONFIG_MMU_PAGE_SIZE - 1); + size_t mmap_addr = phys_addr & ~(CONFIG_MMU_PAGE_SIZE - 1); + ret = esp_mmu_map(mmap_addr, 1 + region_offset, MMU_TARGET_FLASH0, MMU_MEM_CAP_READ | MMU_MEM_CAP_8BIT, ESP_MMU_MMAP_FLAG_PADDR_SHARED, &ptr0); + if (ret == ESP_OK) { + esp_mmu_unmap(ptr0); + } + cnt++; + vTaskDelay(pdMS_TO_TICKS(1)); + } + + xSemaphoreGive(args->done); + vTaskDelete(NULL); +} + +TEST_CASE("Test mmap concurrency", "[mmu][manual][ignore]") +{ + test_task_arg_t args0 = { + .done = xSemaphoreCreateBinary(), + }; + + test_task_arg_t args1 = { + .done = xSemaphoreCreateBinary(), + }; + + xTaskCreate(map_task, "map0_task", 8096, &args0, 1, NULL); + xTaskCreate(map_task, "map1_task", 8096, &args1, 1, NULL); + + xSemaphoreTake(args0.done, portMAX_DELAY); + xSemaphoreTake(args1.done, portMAX_DELAY); + vTaskDelay(100); + vSemaphoreDelete(args0.done); + vSemaphoreDelete(args1.done); +} + #if CONFIG_SPIRAM #if !CONFIG_IDF_TARGET_ESP32 //ESP32 doesn't support using `esp_mmu_map` to map to PSRAM TEST_CASE("Can find paddr when mapping to psram", "[mmu]") From 1abe57c98738cc7e413abf047cb4d6c23fdb47e5 Mon Sep 17 00:00:00 2001 From: armando Date: Thu, 3 Apr 2025 10:31:50 +0800 Subject: [PATCH 3/3] test(system): increased 200B memory leak thresh due to mmu mmap mutex 200B to extend the thresh, real increase to the memory usage will be smaller --- .../test_apps/master/main/test_app_main.c | 2 +- .../esp_mm/test_apps/mmap_hw/main/test_app_main.c | 10 +++++----- .../esp_psram/test_apps/psram/main/test_app_main.c | 2 +- .../esp_system_unity_tests/main/test_app_main.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/components/esp_driver_spi/test_apps/master/main/test_app_main.c b/components/esp_driver_spi/test_apps/master/main/test_app_main.c index 1fb355f3d8..c212ec617d 100644 --- a/components/esp_driver_spi/test_apps/master/main/test_app_main.c +++ b/components/esp_driver_spi/test_apps/master/main/test_app_main.c @@ -10,7 +10,7 @@ #include "esp_newlib.h" // iterator to load partition tables in `test spi bus lock, with flash` will lead memory not free -#define TEST_MEMORY_LEAK_THRESHOLD (350) +#define TEST_MEMORY_LEAK_THRESHOLD (450) void setUp(void) { diff --git a/components/esp_mm/test_apps/mmap_hw/main/test_app_main.c b/components/esp_mm/test_apps/mmap_hw/main/test_app_main.c index 95fc6dcab2..4127c164dc 100644 --- a/components/esp_mm/test_apps/mmap_hw/main/test_app_main.c +++ b/components/esp_mm/test_apps/mmap_hw/main/test_app_main.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -13,11 +13,11 @@ * - traversing all vaddr range to check their attributes * * These tests need certain number of internal resources (heap memory), as they uses up the vaddr ranges - * On ESP32, it should be around 450 - * On ESP32S2, it should be around 600 - * On other chips, it should be around 400 + * On ESP32, it should be around 650 + * On ESP32S2, it should be around 800 + * On other chips, it should be around 600 */ -#define TEST_MEMORY_LEAK_THRESHOLD (-650) +#define TEST_MEMORY_LEAK_THRESHOLD (-800) static size_t before_free_8bit; static size_t before_free_32bit; diff --git a/components/esp_psram/test_apps/psram/main/test_app_main.c b/components/esp_psram/test_apps/psram/main/test_app_main.c index 8ca8bacf7e..1eaf39bad1 100644 --- a/components/esp_psram/test_apps/psram/main/test_app_main.c +++ b/components/esp_psram/test_apps/psram/main/test_app_main.c @@ -8,7 +8,7 @@ #include "unity_test_runner.h" #include "esp_heap_caps.h" -#define TEST_MEMORY_LEAK_THRESHOLD (-700) +#define TEST_MEMORY_LEAK_THRESHOLD (-750) static size_t before_free_8bit; static size_t before_free_32bit; diff --git a/components/esp_system/test_apps/esp_system_unity_tests/main/test_app_main.c b/components/esp_system/test_apps/esp_system_unity_tests/main/test_app_main.c index fdaa6101fc..a55e944f98 100644 --- a/components/esp_system/test_apps/esp_system_unity_tests/main/test_app_main.c +++ b/components/esp_system/test_apps/esp_system_unity_tests/main/test_app_main.c @@ -11,7 +11,7 @@ #include "freertos/task.h" // Some resources are lazy allocated, the threshold is left for that case -#define TEST_MEMORY_LEAK_THRESHOLD (-900) +#define TEST_MEMORY_LEAK_THRESHOLD (-1100) static size_t before_free_8bit; static size_t before_free_32bit;