From 1625a3aae210b605686fb202d3b66fa6a91486af Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 29 Jun 2022 00:58:08 +0800 Subject: [PATCH 1/2] freertos: Fix event group task list race condition FreeRTOS synchronization primitives (e.g., queues, eventgroups) use various event lists (i.e., task lists) to track what tasks are blocked on a current primitive. Usually these event lists are accessed via one of the event lists functions (such as vTask[PlaceOn|RemoveFrom]UnorderedEventList()), which in turn ensure that the global task list spinlock (xTaskQueueMutex) is taken when accessing these lists. However, some functions in event_groups.c manually traverse their event lists. Thus if a tick interrupt occurs on another core during traversal and that tick interrupt unblocks a task on the event list being traversed, the event list will be corrupted. This commit modifies the following event_groups.c functions so that they take the global task list lock before traversing their event list. - xEventGroupSetBits() - vEventGroupDelete() --- .../freertos/FreeRTOS-Kernel/event_groups.c | 16 ++++++++++++++++ .../FreeRTOS-Kernel/include/freertos/task.h | 19 +++++++++++++++++++ components/freertos/FreeRTOS-Kernel/tasks.c | 14 ++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/components/freertos/FreeRTOS-Kernel/event_groups.c b/components/freertos/FreeRTOS-Kernel/event_groups.c index 1e2f7f7322..0d35f33244 100644 --- a/components/freertos/FreeRTOS-Kernel/event_groups.c +++ b/components/freertos/FreeRTOS-Kernel/event_groups.c @@ -607,6 +607,10 @@ EventBits_t xEventGroupSetBits( EventGroupHandle_t xEventGroup, pxListEnd = listGET_END_MARKER( pxList ); /*lint !e826 !e740 !e9087 The mini list structure is used as the list end to save RAM. This is checked and valid. */ #ifdef ESP_PLATFORM // IDF-3755 taskENTER_CRITICAL(); + /* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list. + * Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk + * the task list's being changed while be are traversing it. */ + vTaskTakeEventListLock(); #else vTaskSuspendAll(); #endif // ESP_PLATFORM @@ -682,6 +686,8 @@ EventBits_t xEventGroupSetBits( EventGroupHandle_t xEventGroup, pxEventBits->uxEventBits &= ~uxBitsToClear; } #ifdef ESP_PLATFORM // IDF-3755 + /* Release the previously held task list spinlock, then release the event group spinlock. */ + vTaskReleaseEventListLock(); taskEXIT_CRITICAL(); #else ( void ) xTaskResumeAll(); @@ -700,6 +706,12 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup ) // IDF-3755 taskENTER_CRITICAL(); +#ifdef ESP_PLATFORM + /* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list. + * Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk + * the task list's being changed while be are traversing it. */ + vTaskTakeEventListLock(); +#endif { while( listCURRENT_LIST_LENGTH( pxTasksWaitingForBits ) > ( UBaseType_t ) 0 ) { @@ -709,6 +721,10 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup ) vTaskRemoveFromUnorderedEventList( pxTasksWaitingForBits->xListEnd.pxNext, eventUNBLOCKED_DUE_TO_BIT_SET ); } } +#ifdef ESP_PLATFORM + /* Release the previously held task list spinlock. */ + vTaskReleaseEventListLock(); +#endif taskEXIT_CRITICAL(); #if ( ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) && ( configSUPPORT_STATIC_ALLOCATION == 0 ) ) diff --git a/components/freertos/FreeRTOS-Kernel/include/freertos/task.h b/components/freertos/FreeRTOS-Kernel/include/freertos/task.h index f07774d1b0..0fd3791a88 100644 --- a/components/freertos/FreeRTOS-Kernel/include/freertos/task.h +++ b/components/freertos/FreeRTOS-Kernel/include/freertos/task.h @@ -3369,6 +3369,25 @@ void vTaskPlaceOnEventListRestricted( List_t * const pxEventList, TickType_t xTicksToWait, const BaseType_t xWaitIndefinitely ) PRIVILEGED_FUNCTION; +#ifdef ESP_PLATFORM +/* + * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE. IT IS AN + * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER. + * + * This function is a wrapper to take the "xTaskQueueMutex" spinlock of tasks.c. + * This lock is taken whenver any of the task lists or event lists are + * accessed/modified, such as when adding/removing tasks to/from the delayed + * task list or various event lists. + * + * This functions is meant to be called by xEventGroupSetBits() and + * vEventGroupDelete() as both those functions will access event lists (instead + * of delegating the entire responsibility to one of vTask...EventList() + * functions). + */ +void vTaskTakeEventListLock( void ); +void vTaskReleaseEventListLock( void ); +#endif // ESP_PLATFORM + /* * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE. IT IS AN * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER. diff --git a/components/freertos/FreeRTOS-Kernel/tasks.c b/components/freertos/FreeRTOS-Kernel/tasks.c index a1998c0a20..66069a681e 100644 --- a/components/freertos/FreeRTOS-Kernel/tasks.c +++ b/components/freertos/FreeRTOS-Kernel/tasks.c @@ -3723,6 +3723,20 @@ BaseType_t xTaskRemoveFromEventList( const List_t * const pxEventList ) } /*-----------------------------------------------------------*/ +#ifdef ESP_PLATFORM +void vTaskTakeEventListLock( void ) +{ + /* We call the tasks.c critical section macro to take xTaskQueueMutex */ + taskENTER_CRITICAL(); +} + +void vTaskReleaseEventListLock( void ) +{ + /* We call the tasks.c critical section macro to release xTaskQueueMutex */ + taskEXIT_CRITICAL(); +} +#endif // ESP_PLATFORM + void vTaskRemoveFromUnorderedEventList( ListItem_t * pxEventListItem, const TickType_t xItemValue ) { From 16e739a09e507fb0e43907029a213025ae705a75 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 29 Jun 2022 01:10:57 +0800 Subject: [PATCH 2/2] freertos: Fix flakey event group unit test The "FreeRTOS Event Groups" main task will only wait a single tick for the created tasks to set their response bits. This short delay may not be sufficent if the tick frequency is high. This commit updates the test so that - the main task waits indefinitely for all the response bits to be set. - created tasks are cleand up by the main task --- .../event_groups/test_freertos_eventgroups.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/components/freertos/test/integration/event_groups/test_freertos_eventgroups.c b/components/freertos/test/integration/event_groups/test_freertos_eventgroups.c index ffd9d6d000..a4521ec91e 100644 --- a/components/freertos/test/integration/event_groups/test_freertos_eventgroups.c +++ b/components/freertos/test/integration/event_groups/test_freertos_eventgroups.c @@ -13,6 +13,7 @@ #include "freertos/event_groups.h" #include "driver/gptimer.h" #include "unity.h" +#include "unity_test_utils.h" #define BIT_CALL (1 << 0) #define BIT_RESPONSE(TASK) (1 << (TASK+1)) @@ -40,7 +41,8 @@ static void task_event_group_call_response(void *param) printf("Task %d done\n", task_num); xSemaphoreGive(done_sem); - vTaskDelete(NULL); + // Wait to be deleted + vTaskSuspend(NULL); } TEST_CASE("FreeRTOS Event Groups", "[freertos]") @@ -48,6 +50,8 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") eg = xEventGroupCreate(); done_sem = xSemaphoreCreateCounting(NUM_TASKS, 0); + TaskHandle_t task_handles[NUM_TASKS]; + /* Note: task_event_group_call_response all have higher priority than this task, so on this core they will always preempt this task. @@ -55,7 +59,7 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") or they get out of sync. */ for (int c = 0; c < NUM_TASKS; c++) { - xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, NULL, c % portNUM_PROCESSORS); + xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, &task_handles[c], c % portNUM_PROCESSORS); } /* Tasks all start instantly, but this task will resume running at the same time as the higher priority tasks on the @@ -66,15 +70,19 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") /* signal all tasks with "CALL" bit... */ xEventGroupSetBits(eg, BIT_CALL); - /* Only wait for 1 tick, the wakeup should be immediate... */ - TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 1)); + /* Wait until all tasks have set their respective response bits */ + TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, portMAX_DELAY)); } - /* Ensure all tasks cleaned up correctly */ + /* Ensure all tasks have suspend themselves */ for (int c = 0; c < NUM_TASKS; c++) { TEST_ASSERT( xSemaphoreTake(done_sem, 100 / portTICK_PERIOD_MS) ); } + for (int c = 0; c < NUM_TASKS; c++) { + unity_utils_task_delete(task_handles[c]); + } + vSemaphoreDelete(done_sem); vEventGroupDelete(eg); }