From 3e4fcf66ef396fc0587af898721f3920674e86c0 Mon Sep 17 00:00:00 2001 From: Sudeep Mohanty Date: Tue, 23 Jul 2024 16:19:24 +0200 Subject: [PATCH] fix(freertos): Fixed memory leak issue in vTaskDeleteWithCaps() vTaskDeleteWithCaps() leaked memory when a task uses the API to delete itself. This commit adds a fix to avoid the memory leak. Closes https://github.com/espressif/esp-idf/issues/14222 --- .../linux/include/freertos/portmacro.h | 8 +- .../linux/include/freertos/portmacro_idf.h | 13 +- .../freertos/esp_additions/idf_additions.c | 131 ++++++++++++++++-- .../include/freertos/idf_additions.h | 5 + .../freertos/misc/test_idf_additions.c | 65 ++++++++- 5 files changed, 207 insertions(+), 15 deletions(-) diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/linux/include/freertos/portmacro.h b/components/freertos/FreeRTOS-Kernel-SMP/portable/linux/include/freertos/portmacro.h index f4ee05e14e9..02cbdedada7 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/linux/include/freertos/portmacro.h +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/linux/include/freertos/portmacro.h @@ -244,6 +244,12 @@ static inline BaseType_t xPortInIsrContext(void) return xPortCheckIfInISR(); } +static inline void vPortAssertIfInISR(void) +{ + /* Assert if the interrupt nesting count is > 0 */ + configASSERT(xPortInIsrContext() == 0); +} + // xPortInterruptedFromISRContext() is only used in panic handler and core dump, // both probably not relevant on POSIX sim. //BaseType_t xPortInterruptedFromISRContext(void); @@ -301,7 +307,7 @@ extern void vPortCancelThread( void *pxTaskToDelete ); * are always a full memory barrier. ISRs are emulated as signals * which also imply a full memory barrier. * - * Thus, only a compilier barrier is needed to prevent the compiler + * Thus, only a compiler barrier is needed to prevent the compiler * reordering. */ #define portMEMORY_BARRIER() __asm volatile( "" ::: "memory" ) diff --git a/components/freertos/FreeRTOS-Kernel/portable/linux/include/freertos/portmacro_idf.h b/components/freertos/FreeRTOS-Kernel/portable/linux/include/freertos/portmacro_idf.h index a4b75c066a5..bff384a77db 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/linux/include/freertos/portmacro_idf.h +++ b/components/freertos/FreeRTOS-Kernel/portable/linux/include/freertos/portmacro_idf.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -104,6 +104,17 @@ static inline BaseType_t xPortInIsrContext(void) return xPortCheckIfInISR(); } +static inline void vPortAssertIfInISR(void) +{ + /* Assert if the interrupt nesting count is > 0 */ + configASSERT(xPortInIsrContext() == 0); +} + +/** + * @brief Assert if in ISR context + */ +#define portASSERT_IF_IN_ISR() vPortAssertIfInISR() + #if CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP /* If enabled, users must provide an implementation of vPortCleanUpTCB() */ extern void vPortCleanUpTCB ( void *pxTCB ); diff --git a/components/freertos/esp_additions/idf_additions.c b/components/freertos/esp_additions/idf_additions.c index 4bd428981f6..ee02e838cc4 100644 --- a/components/freertos/esp_additions/idf_additions.c +++ b/components/freertos/esp_additions/idf_additions.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -21,6 +21,8 @@ #include "freertos/timers.h" #include "freertos/idf_additions.h" #include "esp_heap_caps.h" +#include "esp_log.h" +#include "freertos/portmacro.h" /* -------------------------------------------- Creation With Memory Caps ------------------------------------------- */ @@ -81,21 +83,128 @@ #if ( configSUPPORT_STATIC_ALLOCATION == 1 ) + static void prvTaskDeleteWithCapsTask( void * pvParameters ) + { + TaskHandle_t xTaskToDelete = ( TaskHandle_t ) pvParameters; + + /* The task to be deleted must not be running */ + configASSERT( eRunning != eTaskGetState( xTaskToDelete ) ); + + /* Delete the WithCaps task */ + vTaskDeleteWithCaps( xTaskToDelete ); + + /* Delete the temporary clean up task */ + vTaskDelete( NULL ); + } + void vTaskDeleteWithCaps( TaskHandle_t xTaskToDelete ) { - BaseType_t xResult; - StaticTask_t * pxTaskBuffer; - StackType_t * puxStackBuffer; + /* THIS FUNCTION SHOULD NOT BE CALLED FROM AN INTERRUPT CONTEXT. */ + /*TODO: Update it to use portASSERT_IF_IN_ISR() instead. (IDF-10540) */ + vPortAssertIfInISR(); - xResult = xTaskGetStaticBuffers( xTaskToDelete, &puxStackBuffer, &pxTaskBuffer ); - configASSERT( xResult == pdTRUE ); + TaskHandle_t xCurrentTaskHandle = xTaskGetCurrentTaskHandle(); + configASSERT( xCurrentTaskHandle != NULL ); - /* Delete the task */ - vTaskDelete( xTaskToDelete ); + if( ( xTaskToDelete == NULL ) || ( xTaskToDelete == xCurrentTaskHandle ) ) + { + /* The WithCaps task is deleting itself. While, the task can put itself on the + * xTasksWaitingTermination list via the vTaskDelete() call, the idle + * task will not free the task TCB and stack memories we created statically + * during xTaskCreateWithCaps() or xTaskCreatePinnedToCoreWithCaps(). This + * task will never be rescheduled once it is on the xTasksWaitingTermination + * list and will not be able to clear the memories. Therefore, it will leak memory. + * + * To avoid this, we create a new "temporary clean up" task to delete the current task. + * This task is created at the priority of the task to be deleted with the same core + * affitinty. Its limited purpose is to delete the self-deleting task created WithCaps. + * + * This approach has the following problems - + * 1. Once a WithCaps task deletes itself via vTaskDeleteWithCaps(), it may end up in the + * suspended tasks lists for a short time before being deleted. This can give an incorrect + * picture about the system state. + * + * 2. This approach is wasteful and can be error prone. The temporary clean up task will need + * system resources to get scheduled and cleanup the WithCaps task. It can be a problem if the system + * has several self-deleting WithCaps tasks. + * + * TODO: A better approach could be either - + * + * 1. Delegate memory management to the application/user. This way the kernel needn't bother about freeing + * the memory (like other static memory task creation APIs like xTaskCreateStatic()) (IDF-10521) + * + * 2. Have a post deletion hook/callback from the IDLE task to notify higher layers when it is safe to + * perform activities such as clearing up the TCB and stack memories. (IDF-10522) */ + if( xTaskCreatePinnedToCore( ( TaskFunction_t ) prvTaskDeleteWithCapsTask, "prvTaskDeleteWithCapsTask", configMINIMAL_STACK_SIZE, xCurrentTaskHandle, uxTaskPriorityGet( xTaskToDelete ), NULL, xPortGetCoreID() ) != pdFAIL ) + { + /* Although the current task should get preemted immediately when prvTaskDeleteWithCapsTask is created, + * for safety, we suspend the current task and wait for prvTaskDeleteWithCapsTask to delete it. */ + vTaskSuspend( xTaskToDelete ); + + /* Should never reach here */ + ESP_LOGE( "freertos_additions", "%s: Failed to suspend the task to be deleted", __func__ ); + abort(); + } + else + { + /* Failed to create the task to delete the current task. */ + ESP_LOGE( "freertos_additions", "%s: Failed to create the task to delete the current task", __func__ ); + abort(); + } + } - /* Free the memory buffers */ - heap_caps_free( puxStackBuffer ); - vPortFree( pxTaskBuffer ); + #if ( configNUM_CORES > 1 ) + else if( eRunning == eTaskGetState( xTaskToDelete ) ) + { + /* The WithCaps task is running on another core. + * We suspend the task first and then delete it. */ + vTaskSuspend( xTaskToDelete ); + + /* Wait for the task to be suspended */ + while( eRunning == eTaskGetState( xTaskToDelete ) ) + { + portYIELD_WITHIN_API(); + } + + BaseType_t xResult; + StaticTask_t * pxTaskBuffer; + StackType_t * puxStackBuffer; + + xResult = xTaskGetStaticBuffers( xTaskToDelete, &puxStackBuffer, &pxTaskBuffer ); + configASSERT( xResult == pdTRUE ); + configASSERT( puxStackBuffer != NULL ); + configASSERT( pxTaskBuffer != NULL ); + + /* Delete the task */ + vTaskDelete( xTaskToDelete ); + + /* Free the memory buffers */ + heap_caps_free( puxStackBuffer ); + vPortFree( pxTaskBuffer ); + } + #endif /* if ( configNUM_CORES > 1 ) */ + else + { + /* The WithCaps task is not running and is being deleted + * from another task's context. */ + configASSERT( eRunning != eTaskGetState( xTaskToDelete ) ); + + BaseType_t xResult; + StaticTask_t * pxTaskBuffer; + StackType_t * puxStackBuffer; + + xResult = xTaskGetStaticBuffers( xTaskToDelete, &puxStackBuffer, &pxTaskBuffer ); + configASSERT( xResult == pdTRUE ); + configASSERT( puxStackBuffer != NULL ); + configASSERT( pxTaskBuffer != NULL ); + + /* We can delete the task and free the memory buffers. */ + vTaskDelete( xTaskToDelete ); + + /* Free the memory buffers */ + heap_caps_free( puxStackBuffer ); + vPortFree( pxTaskBuffer ); + } /* if( ( xTaskToDelete == NULL ) || ( xTaskToDelete == xCurrentTaskHandle ) ) */ } #endif /* if ( configSUPPORT_STATIC_ALLOCATION == 1 ) */ diff --git a/components/freertos/esp_additions/include/freertos/idf_additions.h b/components/freertos/esp_additions/include/freertos/idf_additions.h index b9fb9c47a08..5e50dd1ba3c 100644 --- a/components/freertos/esp_additions/include/freertos/idf_additions.h +++ b/components/freertos/esp_additions/include/freertos/idf_additions.h @@ -339,6 +339,11 @@ uint8_t * pxTaskGetStackStart( TaskHandle_t xTask ); * @brief Deletes a task previously created using xTaskCreateWithCaps() or * xTaskCreatePinnedToCoreWithCaps() * + * @note It is recommended to use this API to delete tasks from another task's + * context, rather than self-deletion. When the task is being deleted, it is vital + * to ensure that it is not running on another core. This API must not be called + * from an interrupt context. + * * @param xTaskToDelete A handle to the task to be deleted */ void vTaskDeleteWithCaps( TaskHandle_t xTaskToDelete ); diff --git a/components/freertos/test_apps/freertos/misc/test_idf_additions.c b/components/freertos/test_apps/freertos/misc/test_idf_additions.c index aea2ecb2f8e..ced4941398c 100644 --- a/components/freertos/test_apps/freertos/misc/test_idf_additions.c +++ b/components/freertos/test_apps/freertos/misc/test_idf_additions.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -43,7 +43,17 @@ static void task_with_caps(void *arg) vTaskSuspend(NULL); } -TEST_CASE("IDF additions: Task creation with memory caps", "[freertos]") +static void task_with_caps_self_delete(void *arg) +{ + /* Wait for the unity task to indicate that this task should delete itself */ + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + + /* Although it is not recommended to self-delete a task with memory caps but this + * is done intentionally to test for memory leaks */ + vTaskDeleteWithCaps(NULL); +} + +TEST_CASE("IDF additions: Task creation with memory caps and deletion from another task", "[freertos]") { TaskHandle_t task_handle = NULL; StackType_t *puxStackBuffer; @@ -62,6 +72,57 @@ TEST_CASE("IDF additions: Task creation with memory caps", "[freertos]") vTaskDeleteWithCaps(task_handle); } +TEST_CASE("IDF additions: Task creation with memory caps and self deletion", "[freertos]") +{ + TaskHandle_t task_handle = NULL; + StackType_t *puxStackBuffer; + StaticTask_t *pxTaskBuffer; + + // Create a task with caps + TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCoreWithCaps(task_with_caps_self_delete, "task", 4096, (void *)xTaskGetCurrentTaskHandle(), UNITY_FREERTOS_PRIORITY + 1, &task_handle, UNITY_FREERTOS_CPU, OBJECT_MEMORY_CAPS)); + TEST_ASSERT_NOT_EQUAL(NULL, task_handle); + // Get the task's memory + TEST_ASSERT_EQUAL(pdTRUE, xTaskGetStaticBuffers(task_handle, &puxStackBuffer, &pxTaskBuffer)); + TEST_ASSERT(esp_ptr_in_dram(puxStackBuffer)); + TEST_ASSERT(esp_ptr_in_dram(pxTaskBuffer)); + // Notify the task to delete itself + xTaskNotifyGive(task_handle); +} + +#if ( CONFIG_FREERTOS_NUMBER_OF_CORES > 1 ) + +static void task_with_caps_running_on_other_core(void *arg) +{ + /* Notify the unity task that this task is running on the other core */ + xTaskNotifyGive((TaskHandle_t)arg); + + /* We make sure that this task is running on the other core */ + while (1) { + ; + } +} + +TEST_CASE("IDF additions: Task creation with memory caps and deletion from another core", "[freertos]") +{ + TaskHandle_t task_handle = NULL; + StackType_t *puxStackBuffer; + StaticTask_t *pxTaskBuffer; + + // Create a task with caps on the other core + TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCoreWithCaps(task_with_caps_running_on_other_core, "task", 4096, (void *)xTaskGetCurrentTaskHandle(), UNITY_FREERTOS_PRIORITY + 1, &task_handle, !UNITY_FREERTOS_CPU, OBJECT_MEMORY_CAPS)); + TEST_ASSERT_NOT_EQUAL(NULL, task_handle); + // Get the task's memory + TEST_ASSERT_EQUAL(pdTRUE, xTaskGetStaticBuffers(task_handle, &puxStackBuffer, &pxTaskBuffer)); + TEST_ASSERT(esp_ptr_in_dram(puxStackBuffer)); + TEST_ASSERT(esp_ptr_in_dram(pxTaskBuffer)); + // Wait for the created task to start running + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + // Delete the task from another core + vTaskDeleteWithCaps(task_handle); +} + +#endif // CONFIG_FREERTOS_NUMBER_OF_CORES > 1 + TEST_CASE("IDF additions: Queue creation with memory caps", "[freertos]") { QueueHandle_t queue_handle;