From 8825c8dda90f4f2dcc2bdb670c58b80eded54800 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Tue, 29 Aug 2023 02:04:51 +0800 Subject: [PATCH] refactor(freertos/idf): Move critical section API to IDF addition headers - The following IDF API additions are moved to freertos_tasks_c_additions.h (implementation) and freertos_idf_additions_priv.h (declaration) as APIs are private. This reduces the source code difference from upstream. - prvENTER_CRITICAL_OR_SUSPEND_ALL() - prvEXIT_CRITICAL_OR_RESUME_ALL() - prvENTER_CRITICAL_OR_MASK_ISR() - prvEXIT_CRITICAL_OR_UNMASK_ISR() - vTaskTakeKernelLock() - vTaskReleaseKernelLock() - Rename vTask[Take/Release]KernelLock() to prv[Take/Release]KernelLock() to indicate that the this API is private. --- .../freertos/FreeRTOS-Kernel/event_groups.c | 16 ++-- .../FreeRTOS-Kernel/include/freertos/task.h | 48 +----------- components/freertos/FreeRTOS-Kernel/queue.c | 2 + .../freertos/FreeRTOS-Kernel/stream_buffer.c | 2 + components/freertos/FreeRTOS-Kernel/tasks.c | 18 +---- components/freertos/FreeRTOS-Kernel/timers.c | 2 + .../freertos_tasks_c_additions.h | 28 +++++++ .../esp_private/freertos_idf_additions_priv.h | 73 +++++++++++++++++++ components/freertos/linker.lf | 3 - components/freertos/linker_common.lf | 4 + 10 files changed, 124 insertions(+), 72 deletions(-) diff --git a/components/freertos/FreeRTOS-Kernel/event_groups.c b/components/freertos/FreeRTOS-Kernel/event_groups.c index 8b1b184b17..019b85e3b2 100644 --- a/components/freertos/FreeRTOS-Kernel/event_groups.c +++ b/components/freertos/FreeRTOS-Kernel/event_groups.c @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: MIT * - * SPDX-FileContributor: 2016-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2016-2023 Espressif Systems (Shanghai) CO LTD */ /* @@ -45,6 +45,8 @@ #include "task.h" #include "timers.h" #include "event_groups.h" +/* Include private IDF API additions for critical thread safety macros */ +#include "esp_private/freertos_idf_additions_priv.h" /* Lint e961, e750 and e9021 are suppressed as a MISRA exception justified * because the MPU ports require MPU_WRAPPERS_INCLUDED_FROM_API_FILE to be defined @@ -566,8 +568,8 @@ EventBits_t xEventGroupSetBits( EventGroupHandle_t xEventGroup, #if ( configNUM_CORES > 1 ) /* We are about to traverse a task list which is a kernel data structure. - * Thus we need to call vTaskTakeKernelLock() to take the kernel lock. */ - vTaskTakeKernelLock(); + * Thus we need to call prvTakeKernelLock() to take the kernel lock. */ + prvTakeKernelLock(); #endif /* configNUM_CORES > 1 */ { traceEVENT_GROUP_SET_BITS( xEventGroup, uxBitsToSet ); @@ -642,7 +644,7 @@ EventBits_t xEventGroupSetBits( EventGroupHandle_t xEventGroup, } #if ( configNUM_CORES > 1 ) /* Release the previously taken kernel lock. */ - vTaskReleaseKernelLock(); + prvReleaseKernelLock(); #endif /* configNUM_CORES > 1 */ ( void ) prvEXIT_CRITICAL_OR_RESUME_ALL( &( pxEventBits->xEventGroupLock ) ); @@ -659,8 +661,8 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup ) #if ( configNUM_CORES > 1 ) /* We are about to traverse a task list which is a kernel data structure. - * Thus we need to call vTaskTakeKernelLock() to take the kernel lock. */ - vTaskTakeKernelLock(); + * Thus we need to call prvTakeKernelLock() to take the kernel lock. */ + prvTakeKernelLock(); #endif /* configNUM_CORES > 1 */ { traceEVENT_GROUP_DELETE( xEventGroup ); @@ -675,7 +677,7 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup ) } #if ( configNUM_CORES > 1 ) /* Release the previously taken kernel lock. */ - vTaskReleaseKernelLock(); + prvReleaseKernelLock(); #endif /* configNUM_CORES > 1 */ prvEXIT_CRITICAL_OR_RESUME_ALL( &( pxEventBits->xEventGroupLock ) ); diff --git a/components/freertos/FreeRTOS-Kernel/include/freertos/task.h b/components/freertos/FreeRTOS-Kernel/include/freertos/task.h index a50fb78d07..c316608ee8 100644 --- a/components/freertos/FreeRTOS-Kernel/include/freertos/task.h +++ b/components/freertos/FreeRTOS-Kernel/include/freertos/task.h @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: MIT * - * SPDX-FileContributor: 2016-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2016-2023 Espressif Systems (Shanghai) CO LTD */ /* @@ -3439,32 +3439,6 @@ BaseType_t xTaskCatchUpTicks( TickType_t xTicksToCatchUp ) PRIVILEGED_FUNCTION; *----------------------------------------------------------*/ /** @cond !DOC_EXCLUDE_HEADER_SECTION */ -/* - * Various convenience macros for critical sections and scheduler suspension - * called by other FreeRTOS sources and not meant to be called by the - * application. The behavior of each macro depends on whether FreeRTOS is - * currently configured for SMP or single core. - */ -#if ( configNUM_CORES > 1 ) - #define prvENTER_CRITICAL_OR_SUSPEND_ALL( x ) taskENTER_CRITICAL( ( x ) ) - #define prvEXIT_CRITICAL_OR_RESUME_ALL( x ) ( { taskEXIT_CRITICAL( ( x ) ); pdFALSE; } ) - #define prvENTER_CRITICAL_OR_MASK_ISR( pxLock, uxInterruptStatus ) \ - taskENTER_CRITICAL_ISR( ( pxLock ) ); \ - ( void ) ( uxInterruptStatus ); - #define prvEXIT_CRITICAL_OR_UNMASK_ISR( pxLock, uxInterruptStatus ) \ - taskEXIT_CRITICAL_ISR( ( pxLock ) ); \ - ( void ) ( uxInterruptStatus ); -#else /* configNUM_CORES > 1 */ - #define prvENTER_CRITICAL_OR_SUSPEND_ALL( x ) ( { vTaskSuspendAll(); ( void ) ( x ); } ) - #define prvEXIT_CRITICAL_OR_RESUME_ALL( x ) xTaskResumeAll() - #define prvENTER_CRITICAL_OR_MASK_ISR( pxLock, uxInterruptStatus ) \ - ( uxInterruptStatus ) = portSET_INTERRUPT_MASK_FROM_ISR(); \ - ( void ) ( pxLock ); - #define prvEXIT_CRITICAL_OR_UNMASK_ISR( pxLock, uxInterruptStatus ) \ - portCLEAR_INTERRUPT_MASK_FROM_ISR( ( uxInterruptStatus ) ); \ - ( void ) ( pxLock ); -#endif /* configNUM_CORES > 1 */ - /* * Return the handle of the task running on a certain CPU. Because of * the nature of SMP processing, there is no guarantee that this @@ -3577,26 +3551,6 @@ void vTaskPlaceOnEventListRestricted( List_t * const pxEventList, TickType_t xTicksToWait, const BaseType_t xWaitIndefinitely ) PRIVILEGED_FUNCTION; -#if ( configNUM_CORES > 1 ) - -/* - * 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 "xKernelLock" spinlock of tasks.c. - * This lock is taken whenver any of the kernel's data structures 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 vTaskTakeKernelLock( void ); - void vTaskReleaseKernelLock( void ); -#endif /* configNUM_CORES > 1 */ - /* * 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/queue.c b/components/freertos/FreeRTOS-Kernel/queue.c index 69da28f732..1be5631a0b 100644 --- a/components/freertos/FreeRTOS-Kernel/queue.c +++ b/components/freertos/FreeRTOS-Kernel/queue.c @@ -43,6 +43,8 @@ #include "FreeRTOS.h" #include "task.h" #include "queue.h" +/* Include private IDF API additions for critical thread safety macros */ +#include "esp_private/freertos_idf_additions_priv.h" #if ( configUSE_CO_ROUTINES == 1 ) #include "croutine.h" diff --git a/components/freertos/FreeRTOS-Kernel/stream_buffer.c b/components/freertos/FreeRTOS-Kernel/stream_buffer.c index d67f091cba..0973bdbbf1 100644 --- a/components/freertos/FreeRTOS-Kernel/stream_buffer.c +++ b/components/freertos/FreeRTOS-Kernel/stream_buffer.c @@ -45,6 +45,8 @@ #include "FreeRTOS.h" #include "task.h" #include "stream_buffer.h" +/* Include private IDF API additions for critical thread safety macros */ +#include "esp_private/freertos_idf_additions_priv.h" #if ( configUSE_TASK_NOTIFICATIONS != 1 ) #error configUSE_TASK_NOTIFICATIONS must be set to 1 to build stream_buffer.c diff --git a/components/freertos/FreeRTOS-Kernel/tasks.c b/components/freertos/FreeRTOS-Kernel/tasks.c index 9487213e2a..51fc9b47b6 100644 --- a/components/freertos/FreeRTOS-Kernel/tasks.c +++ b/components/freertos/FreeRTOS-Kernel/tasks.c @@ -46,6 +46,8 @@ #include "task.h" #include "timers.h" #include "stack_macros.h" +/* Include private IDF API additions for critical thread safety macros */ +#include "esp_private/freertos_idf_additions_priv.h" #ifdef ESP_PLATFORM #undef _REENT_INIT_PTR @@ -4008,20 +4010,6 @@ BaseType_t xTaskRemoveFromEventList( const List_t * const pxEventList ) } /*-----------------------------------------------------------*/ -#if ( configNUM_CORES > 1 ) - void vTaskTakeKernelLock( void ) - { - /* We call the tasks.c critical section macro to take xKernelLock */ - taskENTER_CRITICAL( &xKernelLock ); - } - - void vTaskReleaseKernelLock( void ) - { - /* We call the tasks.c critical section macro to release xKernelLock */ - taskEXIT_CRITICAL( &xKernelLock ); - } -#endif /* configNUM_CORES > 1 */ - void vTaskRemoveFromUnorderedEventList( ListItem_t * pxEventListItem, const TickType_t xItemValue ) { @@ -4032,7 +4020,7 @@ void vTaskRemoveFromUnorderedEventList( ListItem_t * pxEventListItem, /* THIS FUNCTION MUST BE CALLED WITH THE KERNEL LOCK ALREADY TAKEN. * It is used by the event flags implementation, thus those functions - * should call vTaskTakeKernelLock() before calling this function. */ + * should call prvTakeKernelLock() before calling this function. */ #else /* configNUM_CORES > 1 */ /* THIS FUNCTION MUST BE CALLED WITH THE SCHEDULER SUSPENDED. It is used by diff --git a/components/freertos/FreeRTOS-Kernel/timers.c b/components/freertos/FreeRTOS-Kernel/timers.c index d68ab1e371..7e9db6968d 100644 --- a/components/freertos/FreeRTOS-Kernel/timers.c +++ b/components/freertos/FreeRTOS-Kernel/timers.c @@ -44,6 +44,8 @@ #include "task.h" #include "queue.h" #include "timers.h" +/* Include private IDF API additions for critical thread safety macros */ +#include "esp_private/freertos_idf_additions_priv.h" #if ( INCLUDE_xTimerPendFunctionCall == 1 ) && ( configUSE_TIMERS == 0 ) #error configUSE_TIMERS must be set to 1 to make the xTimerPendFunctionCall() function available. diff --git a/components/freertos/esp_additions/freertos_tasks_c_additions.h b/components/freertos/esp_additions/freertos_tasks_c_additions.h index 9a3beeb6b7..02aeb66949 100644 --- a/components/freertos/esp_additions/freertos_tasks_c_additions.h +++ b/components/freertos/esp_additions/freertos_tasks_c_additions.h @@ -33,6 +33,34 @@ _Static_assert( offsetof( StaticTask_t, pxDummy8 ) == offsetof( TCB_t, pxEndOfSt /* ------------------------------------------------- Kernel Control ------------------------------------------------- */ +#if ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) + +/* + * Wrapper function to take "xKerneLock" + */ + void prvTakeKernelLock( void ) + { + /* We call the tasks.c critical section macro to take xKernelLock */ + taskENTER_CRITICAL( &xKernelLock ); + } + +#endif /* ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) */ +/*----------------------------------------------------------*/ + +#if ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) + +/* + * Wrapper function to release "xKerneLock" + */ + void prvReleaseKernelLock( void ) + { + /* We call the tasks.c critical section macro to release xKernelLock */ + taskEXIT_CRITICAL( &xKernelLock ); + } + +#endif /* ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) */ +/*----------------------------------------------------------*/ + #if ( CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) /* diff --git a/components/freertos/esp_additions/include/esp_private/freertos_idf_additions_priv.h b/components/freertos/esp_additions/include/esp_private/freertos_idf_additions_priv.h index 0318f166d1..7791d81996 100644 --- a/components/freertos/esp_additions/include/esp_private/freertos_idf_additions_priv.h +++ b/components/freertos/esp_additions/include/esp_private/freertos_idf_additions_priv.h @@ -24,6 +24,79 @@ * KERNEL CONTROL (PRIVATE) *----------------------------------------------------------------------------*/ +/* + * The following macros are convenience macros used to account for different + * thread safety behavior between Vanilla FreeRTOS (i.e., single-core) and ESP-IDF + * FreeRTOS (i.e., multi-core SMP). + * + * For thread saftey... + * + * - Vanilla FreeRTOS will use the following for thread safety (depending on situation) + * - `vTaskSuspendAll()`/`xTaskResumeAll()` for non-deterministic operations + * - Critical sections or disabling interrupts for deterministic operations + * - ESP-IDF FreeRTOS will always use critical sections (determinism is not supported) + * + * [refactor-todo]: Define these locally in each kernel source file (IDF-8161) + */ +#if ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) + + #define prvENTER_CRITICAL_OR_SUSPEND_ALL( x ) taskENTER_CRITICAL( ( x ) ) + #define prvEXIT_CRITICAL_OR_RESUME_ALL( x ) ( { taskEXIT_CRITICAL( ( x ) ); pdFALSE; } ) + #define prvENTER_CRITICAL_OR_MASK_ISR( pxLock, uxInterruptStatus ) \ + { \ + taskENTER_CRITICAL_ISR( ( pxLock ) ); \ + ( void ) ( uxInterruptStatus ); \ + } + #define prvEXIT_CRITICAL_OR_UNMASK_ISR( pxLock, uxInterruptStatus ) \ + { \ + taskEXIT_CRITICAL_ISR( ( pxLock ) ); \ + ( void ) ( uxInterruptStatus ); \ + } + +#elif ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES == 1 ) ) + + #define prvENTER_CRITICAL_OR_SUSPEND_ALL( x ) ( { vTaskSuspendAll(); ( void ) ( x ); } ) + #define prvEXIT_CRITICAL_OR_RESUME_ALL( x ) xTaskResumeAll() + #define prvENTER_CRITICAL_OR_MASK_ISR( pxLock, uxInterruptStatus ) \ + { \ + ( uxInterruptStatus ) = portSET_INTERRUPT_MASK_FROM_ISR(); \ + ( void ) ( pxLock ); \ + } + #define prvEXIT_CRITICAL_OR_UNMASK_ISR( pxLock, uxInterruptStatus ) \ + { \ + portCLEAR_INTERRUPT_MASK_FROM_ISR( ( uxInterruptStatus ) ); \ + ( void ) ( pxLock ); \ + } + +#endif /* ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES == 1 ) ) */ + +/* + * In ESP-IDF FreeRTOS (i.e., multi-core SMP) uses spinlocks to protect different + * groups of data. This function is a wrapper to take the "xKernelLock" spinlock + * of tasks.c. + * + * This lock is taken whenever any of the kernel's data structures are + * accessed/modified, such as when adding/removing tasks to/from the delayed + * task list or various event lists. + * + * In more cases, kernel data structures are not accessed by functions outside + * tasks.c. Thus, all accesses of the kernel data structures inside tasks.c will + * handle the taking/releasing of the "xKerneLock". + * + * This functions is meant to be called by xEventGroupSetBits() and + * vEventGroupDelete() as both those functions will directly access event lists + * (which are kernel data structures). Thus, a wrapper function must be provided + * to take/release the "xKernelLock" from outside tasks.c. + * + * [refactor-todo]: Extern this locally in event groups (IDF-8161) + */ +#if ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) + + void prvTakeKernelLock( void ); + void prvReleaseKernelLock( void ); + +#endif /* ( !CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) */ + #if ( CONFIG_FREERTOS_SMP && ( configNUM_CORES > 1 ) ) /** diff --git a/components/freertos/linker.lf b/components/freertos/linker.lf index f23fac8bfe..23ff389917 100644 --- a/components/freertos/linker.lf +++ b/components/freertos/linker.lf @@ -142,9 +142,6 @@ entries: tasks:vTaskPlaceOnEventList (default) tasks:vTaskPlaceOnUnorderedEventList (default) tasks:vTaskPlaceOnEventListRestricted (default) - if FREERTOS_UNICORE = n: - tasks:vTaskTakeKernelLock (default) - tasks:vTaskReleaseKernelLock (default) tasks:vTaskRemoveFromUnorderedEventList (default) tasks:vTaskSetTimeOutState (default) tasks:vTaskInternalSetTimeOutState (default) diff --git a/components/freertos/linker_common.lf b/components/freertos/linker_common.lf index 0756332bbb..ee61f1d03c 100644 --- a/components/freertos/linker_common.lf +++ b/components/freertos/linker_common.lf @@ -17,6 +17,10 @@ entries: # to always keep it in IRAM # ------------------------------------------------------------------------------------------------------------------ if FREERTOS_PLACE_FUNCTIONS_INTO_FLASH = y: + # Kernel Control + if FREERTOS_SMP = n && FREERTOS_UNICORE = n: + tasks:prvTakeKernelLock (default) + tasks:prvReleaseKernelLock (default) # Task Creation if FREERTOS_SMP = y: tasks:xTaskCreatePinnedToCore (default)