From db7d272873d531ef53ca276ff5224cc47d9d82ac Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Sat, 19 Dec 2020 08:45:57 +1100 Subject: [PATCH 1/3] freertos: Add unit test for deleting task which may be blocking --- .../freertos/test/test_freertos_task_delete.c | 74 ++++++++++++++++++- 1 file changed, 71 insertions(+), 3 deletions(-) diff --git a/components/freertos/test/test_freertos_task_delete.c b/components/freertos/test/test_freertos_task_delete.c index a064cf71fa..1a8460ab57 100644 --- a/components/freertos/test/test_freertos_task_delete.c +++ b/components/freertos/test/test_freertos_task_delete.c @@ -2,9 +2,8 @@ * Test backported deletion behavior by creating tasks of various affinities and * check if the task memory is freed immediately under the correct conditions. * - * The behavior of vTaskDelete() has been backported form FreeRTOS v9.0.0. This - * results in the immediate freeing of task memory and the immediate execution - * of deletion callbacks under the following conditions... + * The behavior of vTaskDelete() results in the immediate freeing of task memory + * and the immediate execution of deletion callbacks under the following conditions... * - When deleting a task that is not currently running on either core * - When deleting a task that is pinned to the same core (with respect to * the core that calls vTaskDelete() @@ -16,6 +15,7 @@ #include "freertos/FreeRTOS.h" #include "freertos/task.h" +#include "freertos/semphr.h" #include "esp_heap_caps.h" #include "unity.h" @@ -84,3 +84,71 @@ TEST_CASE("FreeRTOS Delete Tasks", "[freertos]") } } + + +typedef struct { + SemaphoreHandle_t sem; + volatile bool deleted; // Check the deleted task doesn't keep running after being deleted +} tsk_blocks_param_t; + +/* Task blocks as often as possible + (two or more of these can share the same semaphore and "juggle" it around) +*/ +static void tsk_blocks_frequently(void *param) +{ + tsk_blocks_param_t *p = (tsk_blocks_param_t *)param; + SemaphoreHandle_t sem = p->sem; + srand(xTaskGetTickCount() ^ (int)xTaskGetCurrentTaskHandle()); + while (1) { + assert(!p->deleted); + esp_rom_delay_us(rand() % 10); + assert(!p->deleted); + xSemaphoreTake(sem, portMAX_DELAY); + assert(!p->deleted); + esp_rom_delay_us(rand() % 10); + assert(!p->deleted); + xSemaphoreGive(sem); + } +} + +TEST_CASE("FreeRTOS Delete Blocked Tasks", "[freertos]") +{ + TaskHandle_t blocking_tasks[portNUM_PROCESSORS + 1]; // one per CPU, plus one unpinned task + tsk_blocks_param_t params[portNUM_PROCESSORS + 1] = { 0 }; + + unsigned before = heap_caps_get_free_size(MALLOC_CAP_8BIT); + printf("Free memory at start %u\n", before); + + /* Any bugs will depend on relative timing of destroying the tasks, so create & delete many times. + + Stop early if it looks like some resources have not been properly cleaned up. + + (1000 iterations takes about 9 seconds on ESP32 dual core) + */ + for(unsigned iter = 0; iter < 1000; iter++) { + // Create everything + SemaphoreHandle_t sem = xSemaphoreCreateMutex(); + for(unsigned i = 0; i < portNUM_PROCESSORS + 1; i++) { + params[i].deleted = false; + params[i].sem = sem; + + TEST_ASSERT_EQUAL(pdTRUE, + xTaskCreatePinnedToCore(tsk_blocks_frequently, "tsk_block", 4096, ¶ms[i], + UNITY_FREERTOS_PRIORITY - 1, &blocking_tasks[i], + i < portNUM_PROCESSORS ? i : tskNO_AFFINITY)); + } + + vTaskDelay(5); // Let the tasks juggle the mutex for a bit + + for(unsigned i = 0; i < portNUM_PROCESSORS + 1; i++) { + vTaskDelete(blocking_tasks[i]); + params[i].deleted = true; + } + vTaskDelay(4); // Yield to the idle task for cleanup + + vSemaphoreDelete(sem); + + // Check we haven't leaked resources yet + TEST_ASSERT_GREATER_OR_EQUAL(before - 256, heap_caps_get_free_size(MALLOC_CAP_8BIT)); + } +} From f7b859ac841e56ebdd6485a95a9f464adae02095 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 23 Dec 2020 21:09:42 +1100 Subject: [PATCH 2/3] freertos: Fix race condition using vTaskDelete() cross-core causing resource leak Causes test added in parent commit to pass. This race happens if the deleted task is running on the other CPU, and is already spinning in a critical section waiting for xTaskQueueMutex because it's about to be blocked for a resource. The "deleted" task would end up blocked, possibly indefinitely, and never actually deleted or its resources cleaned up by the idle tasks. Details: vTaskDelete() adds the target task to the xTasksWaitingTermination list, expecting it to be yielded off CPU and then cleaned up later. However as soon as vTaskDelete() releases xTaskQueueMutex, the target task runs and moves itself to the xDelayedTaskList1. Because interrupts are already disabled on that CPU, the "yield" to the other CPU sent by the vTaskDelete() comes afterward so doesn't help. --- components/freertos/tasks.c | 41 ++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 0d1a6313b6..deb7774d01 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -1340,6 +1340,21 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode hence xYieldPending is used to latch that a context switch is required. */ portPRE_TASK_DELETE_HOOK( pxTCB, &xYieldPending[core] ); + + if (portNUM_PROCESSORS > 1 && pxTCB == pxCurrentTCB[ !core ]) + { + /* SMP case of deleting a task running on a different core. Same issue + as a task deleting itself, but we need to send a yield to this task now + before we release xTaskQueueMutex. + + Specifically there is a case where the other core may already be spinning on + xTaskQueueMutex waiting to go into a blocked state. A check is added in + prvAddCurrentTaskToDelayedList() to prevent it from removing itself from + xTasksWaitingTermination list in this case (instead it will immediately + release xTaskQueueMutex again and be yielded before the FreeRTOS function + returns.) */ + vPortYieldOtherCore( !core ); + } } else { @@ -1372,25 +1387,6 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode configASSERT( xTaskGetSchedulerState() != taskSCHEDULER_SUSPENDED ); portYIELD_WITHIN_API(); } - else if ( portNUM_PROCESSORS > 1 ) - { - /* Check if the deleted task is currently running on any other core - and force a yield to take it off. - - (this includes re-checking the core that curTCB was previously - running on, in case curTCB has migrated to a different core.) - */ - taskENTER_CRITICAL( &xTaskQueueMutex ); - for(BaseType_t i = 0; i < portNUM_PROCESSORS; i++) - { - if(pxTCB == pxCurrentTCB[ i ] ) - { - vPortYieldOtherCore( i ); - break; - } - } - taskEXIT_CRITICAL( &xTaskQueueMutex ); - } else { mtCOVERAGE_TEST_MARKER(); @@ -5677,6 +5673,13 @@ static void prvAddCurrentTaskToDelayedList( const portBASE_TYPE xCoreID, const T TickType_t xTimeToWake; const TickType_t xConstTickCount = xTickCount; + if (portNUM_PROCESSORS > 1 && listIS_CONTAINED_WITHIN(&xTasksWaitingTermination, &( pxCurrentTCB[xCoreID]->xStateListItem))) { + /* vTaskDelete() has been called on this task. This would have happened from the other core while this task was spinning on xTaskQueueMutex, + so don't move the running task to the delayed list - as soon as this core re-enables interrupts this task will + be suspended permanently */ + return; + } + #if( INCLUDE_xTaskAbortDelay == 1 ) { /* About to enter a delayed list, so ensure the ucDelayAborted flag is From 191e649257f36bbc14e795d6df07f3bf3ffc1fd1 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 24 Dec 2020 09:44:03 +1100 Subject: [PATCH 3/3] freertos: Add some comments about deleting tasks when using SMP Some cases are not immediately obvious, so document them in comments. --- components/freertos/tasks.c | 5 ++++- components/freertos/test/test_freertos_task_delete.c | 9 ++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index deb7774d01..94aa827038 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -1319,7 +1319,10 @@ static void prvAddNewTaskToReadyList( TCB_t *pxNewTCB, TaskFunction_t pxTaskCode uxTaskNumber++; if( pxTCB == curTCB || + /* in SMP, we also can't immediately delete the task active on the other core */ (portNUM_PROCESSORS > 1 && pxTCB == pxCurrentTCB[ !core ]) || + /* ... and we can't delete a non-running task pinned to the other core, as + FPU cleanup has to happen on the same core */ (portNUM_PROCESSORS > 1 && pxTCB->xCoreID == (!core)) ) { /* A task is deleting itself. This cannot complete within the @@ -5674,7 +5677,7 @@ TickType_t xTimeToWake; const TickType_t xConstTickCount = xTickCount; if (portNUM_PROCESSORS > 1 && listIS_CONTAINED_WITHIN(&xTasksWaitingTermination, &( pxCurrentTCB[xCoreID]->xStateListItem))) { - /* vTaskDelete() has been called on this task. This would have happened from the other core while this task was spinning on xTaskQueueMutex, + /* vTaskDelete() has been called to delete this task. This would have happened from the other core while this task was spinning on xTaskQueueMutex, so don't move the running task to the delayed list - as soon as this core re-enables interrupts this task will be suspended permanently */ return; diff --git a/components/freertos/test/test_freertos_task_delete.c b/components/freertos/test/test_freertos_task_delete.c index 1a8460ab57..eb7428deb0 100644 --- a/components/freertos/test/test_freertos_task_delete.c +++ b/components/freertos/test/test_freertos_task_delete.c @@ -3,12 +3,11 @@ * check if the task memory is freed immediately under the correct conditions. * * The behavior of vTaskDelete() results in the immediate freeing of task memory - * and the immediate execution of deletion callbacks under the following conditions... - * - When deleting a task that is not currently running on either core - * - When deleting a task that is pinned to the same core (with respect to - * the core that calls vTaskDelete() + * and the immediate execution of deletion callbacks for tasks which are not + * running, provided they are not pinned to the other core (due to FPU cleanup + * requirements). * - * If the two conditions are not met, freeing of task memory and execution of + * If the condition is not met, freeing of task memory and execution of * deletion callbacks will still be carried out by the Idle Task. */ #include