From e0b1ccf6cee9fa4847348ae32e5c6e158375da92 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Mon, 13 Mar 2023 23:09:18 +0800 Subject: [PATCH] freertos: Fix flakey task snapshot tests The task snapshot tests use esp_cpu_stall() to stall the other CPU before manually walking the task lists. However, it is possible that the other CPU was also accessing the task lists when esp_cpu_stall() is called, leading to flakey tests This commit fixes the test by using a 2-way handshake instead of esp_cpu_stall(). --- .../include/freertos/task_snapshot.h | 6 +- .../integration/tasks/test_tasks_snapshot.c | 59 +++++++++++++++---- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/components/freertos/esp_additions/include/freertos/task_snapshot.h b/components/freertos/esp_additions/include/freertos/task_snapshot.h index 07849a8cd1..cd5d6195e5 100644 --- a/components/freertos/esp_additions/include/freertos/task_snapshot.h +++ b/components/freertos/esp_additions/include/freertos/task_snapshot.h @@ -38,7 +38,7 @@ typedef struct xTASK_SNAPSHOT * - This function is only available when CONFIG_FREERTOS_ENABLE_TASK_SNAPSHOT is set to 1. * * @note This function should only be called when FreeRTOS is no longer running (e.g., during a panic) as this function - * does not acquire any locks. + * does not acquire any locks. * @param pxTask Handle of the previous task (or NULL on the first call of this function) * @return TaskHandle_t Handle of the next task (or NULL when all tasks have been iterated over) */ @@ -51,7 +51,7 @@ TaskHandle_t pxTaskGetNext( TaskHandle_t pxTask ); * - This function is only available when CONFIG_FREERTOS_ENABLE_TASK_SNAPSHOT is set to 1. * * @note This function should only be called when FreeRTOS is no longer running (e.g., during a panic) as this function - * does not acquire any locks. + * does not acquire any locks. * @param[in] pxTask Task's handle * @param[out] pxTaskSnapshot Snapshot of the task * @return pdTRUE if operation was successful else pdFALSE @@ -65,7 +65,7 @@ BaseType_t vTaskGetSnapshot( TaskHandle_t pxTask, TaskSnapshot_t *pxTaskSnapshot * - This function is only available when CONFIG_FREERTOS_ENABLE_TASK_SNAPSHOT is set to 1. * * @note This function should only be called when FreeRTOS is no longer running (e.g., during a panic) as this function - * does not acquire any locks. + * does not acquire any locks. * @param[out] pxTaskSnapshotArray Array of TaskSnapshot_t structures filled by this function * @param[in] uxArrayLength Length of the provided array * @param[out] pxTCBSize Size of the a task's TCB structure diff --git a/components/freertos/test/integration/tasks/test_tasks_snapshot.c b/components/freertos/test/integration/tasks/test_tasks_snapshot.c index 5b8ff6a0ce..e67fdcba2c 100644 --- a/components/freertos/test/integration/tasks/test_tasks_snapshot.c +++ b/components/freertos/test/integration/tasks/test_tasks_snapshot.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -8,10 +8,12 @@ #if CONFIG_FREERTOS_ENABLE_TASK_SNAPSHOT #include +#include #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "freertos/task_snapshot.h" #include "esp_cpu.h" +#include "esp_rom_sys.h" #include "unity.h" #include "sdkconfig.h" @@ -19,10 +21,34 @@ #define NUM_TASKS_PER_LIST 2 #define TASK_PRIORITY (configMAX_PRIORITIES - 2) +static volatile bool start_ready_flag = false; +static volatile bool start_resp_flag = false; +static volatile bool stop_flag = false; + static void ready_task(void *arg) { while (1) { - ; +#if !CONFIG_FREERTOS_UNICORE + taskDISABLE_INTERRUPTS(); + /* + The main task (on core 0) has a higher priority than this task. Thus, by the time this flag is set, this task + is guaranteed to running on core 1. + */ + if (start_ready_flag) { // Is the test starting? + start_resp_flag = true; // Indicate we are ready to start + while (!stop_flag) { + ; // Wait until test completes + } + + // Reset the flags + start_ready_flag = false; + start_resp_flag = false; + stop_flag = false; + } + taskENABLE_INTERRUPTS(); + // Short non-blocking delay to allow INT WDT to feed + esp_rom_delay_us(1000); +#endif // !CONFIG_FREERTOS_UNICORE } } @@ -60,13 +86,25 @@ static void setup(TaskHandle_t *task_list, int *num_tasks_ret, UBaseType_t *old_ // Short delay to allow tasks to spin up vTaskDelay(10); - // Stop preemption on this core, and stall the other core + /* + Task snapshot functions are only meant to be called when FreeRTOS is no longer running (e.g., during a panic), + thus assumes that nothing else (interrupt or the other core) will access the task lists simultaneously. However, for + unit tests, FreeRTOS is still running. So we must guarantee thread safe access another way. + + - For single core, simply disabling interrupts will guarantee that the current task is the only thread to access the + task lists + - For SMP, we also need to stop the other core from accessing the task lists. Thus, we use a 2-way handshake to put + the other core into a while loop with interrupts disabled. + */ + // Disable interrupts so that the current core isn't preempted. taskDISABLE_INTERRUPTS(); #if !CONFIG_FREERTOS_UNICORE - esp_cpu_stall(!xPortGetCoreID()); -#endif - - + // For SMP, we need to do a 2-way handshake to ensure the other core spins with interrupts disabled. + start_ready_flag = true; + while (!start_resp_flag) { + ; + } +#endif // !CONFIG_FREERTOS_UNICORE } static void check_snapshots(TaskHandle_t *task_list, int num_tasks, TaskSnapshot_t *task_snapshots, UBaseType_t num_snapshots) @@ -86,10 +124,11 @@ static void check_snapshots(TaskHandle_t *task_list, int num_tasks, TaskSnapshot static void teardown(TaskHandle_t *task_list, int num_tasks, UBaseType_t old_priority) { - // Resume other cores and allow preemption #if !CONFIG_FREERTOS_UNICORE - esp_cpu_unstall(!xPortGetCoreID()); -#endif + // Signal to the other core that it can exit the loop + stop_flag = true; +#endif // !CONFIG_FREERTOS_UNICORE + // Reenable interrupts on the current core. taskENABLE_INTERRUPTS(); for (int i = 0; i < num_tasks; i++) {