From f2952de3a58c3d5d705d84c17ee7a15b3b6d7327 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 13 Feb 2017 16:53:33 +1100 Subject: [PATCH 1/9] freertos spinlocks/portmux: Add combination unit tests & microbenchmarks --- components/freertos/test/test_spinlocks.c | 133 ++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 components/freertos/test/test_spinlocks.c diff --git a/components/freertos/test/test_spinlocks.c b/components/freertos/test/test_spinlocks.c new file mode 100644 index 000000000..2ddbb07f8 --- /dev/null +++ b/components/freertos/test/test_spinlocks.c @@ -0,0 +1,133 @@ +/* + Combined unit tests & benchmarking for spinlock "portMUX" functionality +*/ + +#include +#include +#include "rom/ets_sys.h" + +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" +#include "freertos/semphr.h" +#include "freertos/queue.h" +#include "freertos/xtensa_api.h" +#include "unity.h" +#include "soc/uart_reg.h" +#include "soc/dport_reg.h" +#include "soc/io_mux_reg.h" +#include "soc/cpu.h" + +#define REPEAT_OPS 10000 + +static uint32_t start, end; + +#define BENCHMARK_START() do { \ + RSR(CCOUNT, start); \ + } while(0) + +#define BENCHMARK_END(OPERATION) do { \ + RSR(CCOUNT, end); \ + printf("%s took %d cycles/op (%d cycles for %d ops)\n", \ + OPERATION, (end - start)/REPEAT_OPS, \ + (end - start), REPEAT_OPS); \ + } while(0) + +TEST_CASE("portMUX spinlocks (no contention)", "[freertos]") +{ + portMUX_TYPE mux = portMUX_INITIALIZER_UNLOCKED; + BENCHMARK_START(); + + for (int i = 0; i < REPEAT_OPS; i++) { + portENTER_CRITICAL(&mux); + portEXIT_CRITICAL(&mux); + } + BENCHMARK_END("no contention lock"); +} + +TEST_CASE("portMUX recursive locks (no contention)", "[freertos]") +{ + portMUX_TYPE mux = portMUX_INITIALIZER_UNLOCKED; + BENCHMARK_START(); + + const int RECURSE_COUNT = 25; + + for (int i = 0; i < REPEAT_OPS / RECURSE_COUNT; i++) { + for (int j = 0; j < RECURSE_COUNT; j++) { + portENTER_CRITICAL(&mux); + } + for (int j = 0; j < RECURSE_COUNT; j++) { + portEXIT_CRITICAL(&mux); + } + } + BENCHMARK_END("no contention recursive"); +} + +static volatile int shared_value; +static portMUX_TYPE shared_mux; +static xSemaphoreHandle done_sem; + +static void task_shared_value_increment(void *ignore) +{ + for (int i = 0; i < REPEAT_OPS; i++) { + portENTER_CRITICAL(&shared_mux); + shared_value++; + portEXIT_CRITICAL(&shared_mux); + } + xSemaphoreGive(done_sem); + vTaskDelete(NULL); +} + +TEST_CASE("portMUX cross-core locking", "[freertos]") +{ + done_sem = xSemaphoreCreateCounting(2, 0); + vPortCPUInitializeMutex(&shared_mux); + shared_value = 0; + + BENCHMARK_START(); + + xTaskCreatePinnedToCore(task_shared_value_increment, "INC0", 2048, NULL, UNITY_FREERTOS_PRIORITY + 1, NULL, UNITY_FREERTOS_CPU ? 0 : 1); + xTaskCreatePinnedToCore(task_shared_value_increment, "INC1", 2048, NULL, UNITY_FREERTOS_PRIORITY + 1, NULL, UNITY_FREERTOS_CPU); + + for(int i = 0; i < 2; i++) { + if(!xSemaphoreTake(done_sem, 10000/portTICK_PERIOD_MS)) { + TEST_FAIL_MESSAGE("done_sem not released by test task"); + } + } + + BENCHMARK_END("cross-core incrementing"); + vSemaphoreDelete(done_sem); + + TEST_ASSERT_EQUAL_INT(REPEAT_OPS * 2, shared_value); +} + +TEST_CASE("portMUX high contention", "[freertos]") +{ + const int TOTAL_TASKS = 8; /* half on each core */ + done_sem = xSemaphoreCreateCounting(TOTAL_TASKS, 0); + vPortCPUInitializeMutex(&shared_mux); + shared_value = 0; + + BENCHMARK_START(); + + for (int i = 0; i < TOTAL_TASKS / 2; i++) { + /* as each task has a higher priority than previous, expect + them to preempt the earlier created task, at least on the + other core (this core has the unity task, until that + blocks)... */ + xTaskCreatePinnedToCore(task_shared_value_increment, "INC0", 2048, NULL, tskIDLE_PRIORITY + 1 + i, NULL, UNITY_FREERTOS_CPU ? 0 : 1); + xTaskCreatePinnedToCore(task_shared_value_increment, "INC1", 2048, NULL, tskIDLE_PRIORITY + 1 + i, NULL, UNITY_FREERTOS_CPU); + } + + for(int i = 0; i < TOTAL_TASKS; i++) { + if(!xSemaphoreTake(done_sem, 10000/portTICK_PERIOD_MS)) { + TEST_FAIL_MESSAGE("done_sem not released by test task"); + } + } + + BENCHMARK_END("cross-core high contention"); + vSemaphoreDelete(done_sem); + + TEST_ASSERT_EQUAL_INT(REPEAT_OPS * TOTAL_TASKS, shared_value); +} + + From 4d42b2d100fc11551f3cee9dd2e7bf4ca57eca42 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 13 Feb 2017 14:46:37 +1100 Subject: [PATCH 2/9] freertos spinlock/portmux: Reduce spinlocking overhead Ref TW7117 Microbenchmarks in unit tests: (All numbers in cycles per benchmarked operation): Release mode No lock contention lock/unlock - 301 -> 167 (-45%) Recursive no contention lock/unlock - 289 -> 148 (-49%) Lock contention two CPUs (lock/unlock) 699 -> 400 (-43%) Debug mode No lock contention lock/unlock - 355 -> 203 (-43%) Recursive no contention lock/unlock - 345 -> 188 (-46%) Lock contention two CPUs (lock/unlock) 761 -> 483 (-36%) --- .../freertos/include/freertos/FreeRTOS.h | 15 +- .../freertos/include/freertos/portable.h | 2 +- .../freertos/include/freertos/portmacro.h | 33 ++-- .../include/freertos/xtensa_context.h | 10 +- components/freertos/port.c | 165 +++++++++++------- components/freertos/tasks.c | 37 ++-- 6 files changed, 152 insertions(+), 110 deletions(-) diff --git a/components/freertos/include/freertos/FreeRTOS.h b/components/freertos/include/freertos/FreeRTOS.h index 31b66fb85..d1e5d4eed 100644 --- a/components/freertos/include/freertos/FreeRTOS.h +++ b/components/freertos/include/freertos/FreeRTOS.h @@ -978,13 +978,14 @@ typedef struct xSTATIC_QUEUE uint8_t ucDummy9; #endif - struct { - volatile uint32_t ucDummy10; - #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - void *pvDummy8; - UBaseType_t uxDummy11; - #endif - } sDummy12; + struct { + volatile uint32_t ucDummy10; + uint32_t ucDummy11; + #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + void *pvDummy8; + UBaseType_t uxDummy12; + #endif + } sDummy1; } StaticQueue_t; typedef StaticQueue_t StaticSemaphore_t; diff --git a/components/freertos/include/freertos/portable.h b/components/freertos/include/freertos/portable.h index a628cf031..096e481e0 100644 --- a/components/freertos/include/freertos/portable.h +++ b/components/freertos/include/freertos/portable.h @@ -199,7 +199,7 @@ BaseType_t xPortInIsrContext(); /* Multi-core: get current core ID */ static inline uint32_t IRAM_ATTR xPortGetCoreID() { int id; - asm volatile( + asm ( "rsr.prid %0\n" " extui %0,%0,13,1" :"=r"(id)); diff --git a/components/freertos/include/freertos/portmacro.h b/components/freertos/include/freertos/portmacro.h index b74766283..63e4bda66 100644 --- a/components/freertos/include/freertos/portmacro.h +++ b/components/freertos/include/freertos/portmacro.h @@ -127,7 +127,8 @@ typedef unsigned portBASE_TYPE UBaseType_t; typedef struct { - volatile uint32_t mux; + volatile uint32_t owner; + uint32_t count; #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG const char *lastLockedFn; int lastLockedLine; @@ -143,24 +144,19 @@ typedef struct { * The magic number in the top 16 bits is there so we can detect uninitialized and corrupted muxes. */ -#define portMUX_MAGIC_VAL 0xB33F0000 #define portMUX_FREE_VAL 0xB33FFFFF -#define portMUX_MAGIC_MASK 0xFFFF0000 -#define portMUX_MAGIC_SHIFT 16 -#define portMUX_CNT_MASK 0x0000FF00 -#define portMUX_CNT_SHIFT 8 -#define portMUX_VAL_MASK 0x000000FF -#define portMUX_VAL_SHIFT 0 //Keep this in sync with the portMUX_TYPE struct definition please. #ifndef CONFIG_FREERTOS_PORTMUX_DEBUG -#define portMUX_INITIALIZER_UNLOCKED { \ - .mux = portMUX_MAGIC_VAL|portMUX_FREE_VAL \ +#define portMUX_INITIALIZER_UNLOCKED { \ + .owner = portMUX_FREE_VAL, \ + .count = 0, \ } #else -#define portMUX_INITIALIZER_UNLOCKED { \ - .mux = portMUX_MAGIC_VAL|portMUX_FREE_VAL, \ - .lastLockedFn = "(never locked)", \ +#define portMUX_INITIALIZER_UNLOCKED { \ + .owner = portMUX_FREE_VAL, \ + .count = 0, \ + .lastLockedFn = "(never locked)", \ .lastLockedLine = -1 \ } #endif @@ -173,8 +169,7 @@ typedef struct { #define portASSERT_IF_IN_ISR() vPortAssertIfInISR() void vPortAssertIfInISR(); - -#define portCRITICAL_NESTING_IN_TCB 1 +#define portCRITICAL_NESTING_IN_TCB 1 /* Modifications to portENTER_CRITICAL: @@ -204,6 +199,8 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux); #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *function, int line); portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *function, int line); +void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *function, int line); +portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *function, int line); void vTaskEnterCritical( portMUX_TYPE *mux, const char *function, int line ); void vTaskExitCritical( portMUX_TYPE *mux, const char *function, int line ); #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux, __FUNCTION__, __LINE__) @@ -215,6 +212,9 @@ void vTaskExitCritical( portMUX_TYPE *mux ); void vTaskEnterCritical( portMUX_TYPE *mux ); void vPortCPUAcquireMutex(portMUX_TYPE *mux); portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux); +void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux); +portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux); + #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux) #define portEXIT_CRITICAL(mux) vTaskExitCritical(mux) #define portENTER_CRITICAL_ISR(mux) vTaskEnterCritical(mux) @@ -243,9 +243,8 @@ static inline unsigned portENTER_CRITICAL_NESTED() { unsigned state = XTOS_SET_I * ESP32, though. (Would show up directly if it did because the magic wouldn't match.) */ static inline void uxPortCompareSet(volatile uint32_t *addr, uint32_t compare, uint32_t *set) { - __asm__ __volatile__( + __asm__ __volatile__ ( "WSR %2,SCOMPARE1 \n" - "ISYNC \n" "S32C1I %0, %1, 0 \n" :"=r"(*set) :"r"(addr), "r"(compare), "0"(*set) diff --git a/components/freertos/include/freertos/xtensa_context.h b/components/freertos/include/freertos/xtensa_context.h index b748a0d1a..58520f853 100644 --- a/components/freertos/include/freertos/xtensa_context.h +++ b/components/freertos/include/freertos/xtensa_context.h @@ -314,11 +314,10 @@ STRUCT_END(XtSolFrame) /* Macro to get the current core ID. Only uses the reg given as an argument. - Reading PRID on the ESP108 architecture gives us 0xCDCD on the PRO processor - and 0xABAB on the APP CPU. We distinguish between the two by simply checking - bit 1: it's 1 on the APP and 0 on the PRO processor. + Reading PRID on the ESP32 gives us 0xCDCD on the PRO processor (0) + and 0xABAB on the APP CPU (1). We distinguish between the two by simply checking + bit 13: it's 1 on the APP and 0 on the PRO processor. */ - #ifdef __ASSEMBLER__ .macro getcoreid reg rsr.prid \reg @@ -326,7 +325,8 @@ STRUCT_END(XtSolFrame) .endm #endif - +#define CORE_ID_PRO 0xCDCD +#define CORE_ID_APP 0xABAB /* ------------------------------------------------------------------------------- diff --git a/components/freertos/port.c b/components/freertos/port.c index df298fc4a..48669472a 100644 --- a/components/freertos/port.c +++ b/components/freertos/port.c @@ -97,6 +97,7 @@ #include "xtensa_rtos.h" #include "rom/ets_sys.h" +#include "soc/cpu.h" #include "FreeRTOS.h" #include "task.h" @@ -302,118 +303,154 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux) { mux->lastLockedFn="(never locked)"; mux->lastLockedLine=-1; #endif - mux->mux=portMUX_FREE_VAL; + mux->owner=portMUX_FREE_VAL; + mux->count=0; } +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG +void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *fnName, int line) { + unsigned int irqStatus = portENTER_CRITICAL_NESTED(); + vPortCPUAcquireMutexIntsDisabled(mux, fnName, line); + portEXIT_CRITICAL_NESTED(irqStatus); +} +#else +void vPortCPUAcquireMutex(portMUX_TYPE *mux) { + unsigned int irqStatus = portENTER_CRITICAL_NESTED(); + vPortCPUAcquireMutexIntsDisabled(mux); + portEXIT_CRITICAL_NESTED(irqStatus); +} +#endif + +/* XOR one core ID with this value to get the other core ID */ +#define CORE_ID_XOR_SWAP (CORE_ID_PRO ^ CORE_ID_APP) + /* * For kernel use: Acquire a per-CPU mux. Spinlocks, so don't hold on to these muxes for too long. */ #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *fnName, int line) { +void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { #else -void vPortCPUAcquireMutex(portMUX_TYPE *mux) { +void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { #endif #if !CONFIG_FREERTOS_UNICORE uint32_t res; - uint32_t recCnt; - unsigned int irqStatus; + portBASE_TYPE coreID, otherCoreID; #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - uint32_t cnt=(1<<16); - if ( (mux->mux & portMUX_MAGIC_MASK) != portMUX_MAGIC_VAL ) { - ets_printf("ERROR: vPortCPUAcquireMutex: mux %p is uninitialized (0x%X)! Called from %s line %d.\n", mux, mux->mux, fnName, line); - mux->mux=portMUX_FREE_VAL; + uint32_t timeout=(1<<16); + uint32_t owner = mux->owner; + if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { + ets_printf("ERROR: vPortCPUAcquireMutex: mux %p is uninitialized (0x%X)! Called from %s line %d.\n", mux, owner, fnName, line); + mux->owner=portMUX_FREE_VAL; } #endif - irqStatus=portENTER_CRITICAL_NESTED(); + /* Spin until we own the core */ + + RSR(PRID, coreID); + /* Note: coreID is the full 32 bit core ID (CORE_ID_PRO/CORE_ID_APP), + not the 0/1 value returned by xPortGetCoreID() + */ + otherCoreID = CORE_ID_XOR_SWAP ^ coreID; do { - //Lock mux if it's currently unlocked - res=(xPortGetCoreID()<mux, portMUX_FREE_VAL, &res); - //If it wasn't free and we're the owner of the lock, we are locking recursively. - if ( (res != portMUX_FREE_VAL) && (((res&portMUX_VAL_MASK)>>portMUX_VAL_SHIFT) == xPortGetCoreID()) ) { - //Mux was already locked by us. Just bump the recurse count by one. - recCnt=(res&portMUX_CNT_MASK)>>portMUX_CNT_SHIFT; - recCnt++; -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE - ets_printf("Recursive lock: recCnt=%d last non-recursive lock %s line %d, curr %s line %d\n", recCnt, mux->lastLockedFn, mux->lastLockedLine, fnName, line); -#endif - mux->mux=portMUX_MAGIC_VAL|(recCnt<owner should be one of portMUX_FREE_VAL, CORE_ID_PRO, + CORE_ID_APP: + + - If portMUX_FREE_VAL, we want to atomically set to 'coreID'. + - If "our" coreID, we can drop through immediately. + - If "otherCoreID", we spin here. + */ + res = coreID; + uxPortCompareSet(&mux->owner, portMUX_FREE_VAL, &res); + #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - cnt--; - if (cnt==0) { + timeout--; + if (timeout == 0) { ets_printf("Timeout on mux! last non-recursive lock %s line %d, curr %s line %d\n", mux->lastLockedFn, mux->lastLockedLine, fnName, line); - ets_printf("Mux value %X\n", mux->mux); + ets_printf("Owner 0x%x count %d\n", mux->owner, mux->count); } #endif - } while (res!=portMUX_FREE_VAL); + } while (res == otherCoreID); + + assert(res == coreID || res == portMUX_FREE_VAL); /* any other value implies memory corruption */ + assert((res == portMUX_FREE_VAL) == (mux->count == 0)); /* we're first to lock iff count is zero */ + + /* now we own it, we can increment the refcount */ + mux->count++; + #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG if (res==portMUX_FREE_VAL) { //initial lock mux->lastLockedFn=fnName; mux->lastLockedLine=line; + } else { + ets_printf("Recursive lock: count=%d last non-recursive lock %s line %d, curr %s line %d\n", mux->count-1, + mux->lastLockedFn, mux->lastLockedLine, fnName, line); } #endif - portEXIT_CRITICAL_NESTED(irqStatus); #endif } +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG +portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *fnName, int line) { + unsigned int irqStatus = portENTER_CRITICAL_NESTED(); + portBASE_TYPE res = vPortCPUReleaseMutexIntsDisabled(mux, fnName, line); + portEXIT_CRITICAL_NESTED(irqStatus); + return res; +} +#else +portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { + unsigned int irqStatus = portENTER_CRITICAL_NESTED(); + portBASE_TYPE res = vPortCPUReleaseMutexIntsDisabled(mux); + portEXIT_CRITICAL_NESTED(irqStatus); + return res; +} +#endif + /* * For kernel use: Release a per-CPU mux. Returns true if everything is OK, false if mux * was already unlocked or is locked by a different core. */ #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *fnName, int line) { +portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { #else -portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { +portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { #endif #if !CONFIG_FREERTOS_UNICORE - uint32_t res=0; - uint32_t recCnt; - unsigned int irqStatus; - portBASE_TYPE ret=pdTRUE; -// ets_printf("Unlock %p\n", mux); - irqStatus=portENTER_CRITICAL_NESTED(); + portBASE_TYPE res; + portBASE_TYPE coreID; #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG const char *lastLockedFn=mux->lastLockedFn; int lastLockedLine=mux->lastLockedLine; mux->lastLockedFn=fnName; mux->lastLockedLine=line; - if ( (mux->mux & portMUX_MAGIC_MASK) != portMUX_MAGIC_VAL ) ets_printf("ERROR: vPortCPUReleaseMutex: mux %p is uninitialized (0x%X)!\n", mux, mux->mux); + uint32_t owner = mux->owner; + if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { + ets_printf("ERROR: vPortCPUReleaseMutex: mux %p is uninitialized (0x%x)!\n", mux, mux->owner); + } #endif - //Unlock mux if it's currently locked with a recurse count of 0 - res=portMUX_FREE_VAL; - uxPortCompareSet(&mux->mux, (xPortGetCoreID()<>portMUX_VAL_SHIFT) == xPortGetCoreID() ) { - //Lock is valid, we can return safely. Just need to check if it's a recursive lock; if so we need to decrease the refcount. - if ( ((res&portMUX_CNT_MASK)>>portMUX_CNT_SHIFT)!=0) { - //We locked this, but the reccount isn't zero. Decrease refcount and continue. - recCnt=(res&portMUX_CNT_MASK)>>portMUX_CNT_SHIFT; - recCnt--; -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE - ets_printf("Recursive unlock: recCnt=%d last locked %s line %d, curr %s line %d\n", recCnt, lastLockedFn, lastLockedLine, fnName, line); -#endif - mux->mux=portMUX_MAGIC_VAL|(recCnt<owner); + #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + if (!res) { ets_printf("ERROR: vPortCPUReleaseMutex: mux %p was already unlocked!\n", mux); ets_printf("Last non-recursive unlock %s line %d, curr unlock %s line %d\n", lastLockedFn, lastLockedLine, fnName, line); -#endif - ret=pdFALSE; - } else { -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - ets_printf("ERROR: vPortCPUReleaseMutex: mux %p wasn't locked by this core (%d) but by core %d (ret=%x, mux=%x).\n", mux, xPortGetCoreID(), ((res&portMUX_VAL_MASK)>>portMUX_VAL_SHIFT), res, mux->mux); - ets_printf("Last non-recursive lock %s line %d\n", lastLockedFn, lastLockedLine); - ets_printf("Called by %s line %d\n", fnName, line); -#endif - ret=pdFALSE; } - portEXIT_CRITICAL_NESTED(irqStatus); - return ret; +#endif + + mux->count--; + if(mux->count == 0) { + mux->owner = portMUX_FREE_VAL; + } +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE + else { + ets_printf("Recursive unlock: count=%d last locked %s line %d, curr %s line %d\n", mux->count, lastLockedFn, lastLockedLine, fnName, line); + } +#endif + + return res; + #else //!CONFIG_FREERTOS_UNICORE return 0; #endif diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 4e1a0c0e2..74dec7fbd 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -4111,7 +4111,8 @@ For ESP32 FreeRTOS, vTaskEnterCritical implements both portENTER_CRITICAL and po #endif { BaseType_t oldInterruptLevel=0; - if( xSchedulerRunning != pdFALSE ) + BaseType_t schedulerRunning = xSchedulerRunning; + if( schedulerRunning != pdFALSE ) { //Interrupts may already be disabled (because we're doing this recursively) but we can't get the interrupt level after //vPortCPUAquireMutex, because it also may mess with interrupts. Get it here first, then later figure out if we're nesting @@ -4119,26 +4120,27 @@ For ESP32 FreeRTOS, vTaskEnterCritical implements both portENTER_CRITICAL and po oldInterruptLevel=portENTER_CRITICAL_NESTED(); } #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - vPortCPUAcquireMutex( mux, function, line ); + vPortCPUAcquireMutexIntsDisabled( mux, function, line ); #else - vPortCPUAcquireMutex( mux ); + vPortCPUAcquireMutexIntsDisabled( mux ); #endif - if( xSchedulerRunning != pdFALSE ) + if( schedulerRunning != pdFALSE ) { - ( pxCurrentTCB[ xPortGetCoreID() ]->uxCriticalNesting )++; - - if( xSchedulerRunning != pdFALSE && pxCurrentTCB[ xPortGetCoreID() ]->uxCriticalNesting == 1 ) + TCB_t *tcb = pxCurrentTCB[xPortGetCoreID()]; + BaseType_t newNesting = tcb->uxCriticalNesting + 1; + tcb->uxCriticalNesting = newNesting; + if( newNesting == 1 ) { //This is the first time we get called. Save original interrupt level. - pxCurrentTCB[ xPortGetCoreID() ]->uxOldInterruptState=oldInterruptLevel; + tcb->uxOldInterruptState = oldInterruptLevel; } /* Original FreeRTOS comment, saved for reference: This is not the interrupt safe version of the enter critical function so assert() if it is being called from an interrupt context. Only API functions that end in "FromISR" can be used in an - interrupt. Only assert if the critical nesting count is 1 to + interrupt. Only assert if the critical nesting count is 1 to protect against recursive calls if the assert function also uses a critical section. */ @@ -4149,7 +4151,7 @@ For ESP32 FreeRTOS, vTaskEnterCritical implements both portENTER_CRITICAL and po both from ISR as well as non-ISR code, thus we re-organized vTaskEnterCritical to also work in ISRs. */ #if 0 - if( pxCurrentTCB[ xPortGetCoreID() ]->uxCriticalNesting == 1 ) + if( newNesting == 1 ) { portASSERT_IF_IN_ISR(); } @@ -4178,19 +4180,22 @@ For ESP32 FreeRTOS, vTaskExitCritical implements both portEXIT_CRITICAL and port #endif { #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - vPortCPUReleaseMutex( mux, function, line ); + vPortCPUReleaseMutexIntsDisabled( mux, function, line ); #else - vPortCPUReleaseMutex( mux ); + vPortCPUReleaseMutexIntsDisabled( mux ); #endif if( xSchedulerRunning != pdFALSE ) { - if( pxCurrentTCB[ xPortGetCoreID() ]->uxCriticalNesting > 0U ) + TCB_t *tcb = pxCurrentTCB[xPortGetCoreID()]; + BaseType_t nesting = tcb->uxCriticalNesting; + if( nesting > 0U ) { - ( pxCurrentTCB[ xPortGetCoreID() ]->uxCriticalNesting )--; + nesting--; + tcb->uxCriticalNesting = nesting; - if( pxCurrentTCB[ xPortGetCoreID() ]->uxCriticalNesting == 0U ) + if( nesting == 0U ) { - portEXIT_CRITICAL_NESTED(pxCurrentTCB[ xPortGetCoreID() ]->uxOldInterruptState); + portEXIT_CRITICAL_NESTED(tcb->uxOldInterruptState); } else { From db58a2732ba1d3ea4975b633d6f17e84242490b3 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 16 Feb 2017 16:03:02 +1100 Subject: [PATCH 3/9] freertos: vPortCPUReleaseMutex() no longer returns a value Unlocking a never-locked mutex is an assertion failure in debug mode. In release mode, this further improves performance: No-Contention -> 153 cycles Recursion No-Contention -> 138 cycles Contention -> 378 cycles --- .../freertos/include/freertos/portmacro.h | 6 +-- components/freertos/port.c | 52 +++++++++---------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/components/freertos/include/freertos/portmacro.h b/components/freertos/include/freertos/portmacro.h index 63e4bda66..f55eeefe0 100644 --- a/components/freertos/include/freertos/portmacro.h +++ b/components/freertos/include/freertos/portmacro.h @@ -127,7 +127,7 @@ typedef unsigned portBASE_TYPE UBaseType_t; typedef struct { - volatile uint32_t owner; + uint32_t owner; uint32_t count; #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG const char *lastLockedFn; @@ -211,9 +211,9 @@ void vTaskExitCritical( portMUX_TYPE *mux, const char *function, int line ); void vTaskExitCritical( portMUX_TYPE *mux ); void vTaskEnterCritical( portMUX_TYPE *mux ); void vPortCPUAcquireMutex(portMUX_TYPE *mux); -portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux); +void vPortCPUReleaseMutex(portMUX_TYPE *mux); void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux); -portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux); +void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux); #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux) #define portEXIT_CRITICAL(mux) vTaskExitCritical(mux) diff --git a/components/freertos/port.c b/components/freertos/port.c index 48669472a..fda5ed6cf 100644 --- a/components/freertos/port.c +++ b/components/freertos/port.c @@ -308,6 +308,9 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux) { } +/* + * For kernel use: Acquire a per-CPU mux. Spinlocks, so don't hold on to these muxes for too long. + */ #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *fnName, int line) { unsigned int irqStatus = portENTER_CRITICAL_NESTED(); @@ -325,9 +328,6 @@ void vPortCPUAcquireMutex(portMUX_TYPE *mux) { /* XOR one core ID with this value to get the other core ID */ #define CORE_ID_XOR_SWAP (CORE_ID_PRO ^ CORE_ID_APP) -/* - * For kernel use: Acquire a per-CPU mux. Spinlocks, so don't hold on to these muxes for too long. - */ #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { #else @@ -372,12 +372,14 @@ void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { #endif } while (res == otherCoreID); - assert(res == coreID || res == portMUX_FREE_VAL); /* any other value implies memory corruption */ + assert(res == coreID || res == portMUX_FREE_VAL); /* any other value implies memory corruption or uninitialized mux */ assert((res == portMUX_FREE_VAL) == (mux->count == 0)); /* we're first to lock iff count is zero */ + assert(mux->count < 0xFF); /* Bad count value implies memory corruption */ /* now we own it, we can increment the refcount */ mux->count++; + #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG if (res==portMUX_FREE_VAL) { //initial lock mux->lastLockedFn=fnName; @@ -390,33 +392,31 @@ void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { #endif } +/* + * For kernel use: Release a per-CPU mux + * + * Mux must be already locked by this core + */ #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *fnName, int line) { +void vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *fnName, int line) { unsigned int irqStatus = portENTER_CRITICAL_NESTED(); - portBASE_TYPE res = vPortCPUReleaseMutexIntsDisabled(mux, fnName, line); + vPortCPUReleaseMutexIntsDisabled(mux, fnName, line); portEXIT_CRITICAL_NESTED(irqStatus); - return res; } #else -portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { +void vPortCPUReleaseMutex(portMUX_TYPE *mux) { unsigned int irqStatus = portENTER_CRITICAL_NESTED(); - portBASE_TYPE res = vPortCPUReleaseMutexIntsDisabled(mux); + vPortCPUReleaseMutexIntsDisabled(mux); portEXIT_CRITICAL_NESTED(irqStatus); - return res; } #endif -/* - * For kernel use: Release a per-CPU mux. Returns true if everything is OK, false if mux - * was already unlocked or is locked by a different core. - */ #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { +void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { #else -portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { +void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { #endif #if !CONFIG_FREERTOS_UNICORE - portBASE_TYPE res; portBASE_TYPE coreID; #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG const char *lastLockedFn=mux->lastLockedFn; @@ -425,20 +425,25 @@ portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { mux->lastLockedLine=line; uint32_t owner = mux->owner; if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { - ets_printf("ERROR: vPortCPUReleaseMutex: mux %p is uninitialized (0x%x)!\n", mux, mux->owner); + ets_printf("ERROR: vPortCPUReleaseMutex: mux %p is invalid (0x%x)!\n", mux, mux->owner); } #endif +#if CONFIG_FREERTOS_PORTMUX_DEBUG || !defined(NDEBUG) RSR(PRID, coreID); - res = (coreID == mux->owner); +#endif #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - if (!res) { + if (coreID != mux->owner) { ets_printf("ERROR: vPortCPUReleaseMutex: mux %p was already unlocked!\n", mux); ets_printf("Last non-recursive unlock %s line %d, curr unlock %s line %d\n", lastLockedFn, lastLockedLine, fnName, line); } #endif + assert(coreID == mux->owner); // This is a mutex we didn't lock, or it's corrupt + assert(mux->count > 0); // Indicates memory corruption + assert(mux->count < 0x100); // Indicates memory corruption + mux->count--; if(mux->count == 0) { mux->owner = portMUX_FREE_VAL; @@ -448,12 +453,7 @@ portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { ets_printf("Recursive unlock: count=%d last locked %s line %d, curr %s line %d\n", mux->count, lastLockedFn, lastLockedLine, fnName, line); } #endif - - return res; - -#else //!CONFIG_FREERTOS_UNICORE - return 0; -#endif +#endif //!CONFIG_FREERTOS_UNICORE } #if CONFIG_FREERTOS_BREAK_ON_SCHEDULER_START_JTAG From 5c996a1b292db0411ebb3c3e6b8106c58aeb7d62 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 16 Feb 2017 16:56:06 +1100 Subject: [PATCH 4/9] freertos: Inline vPortCPUAcquireMutex/vPortCPUReleaseMutex into implementations Further improves performance: No contention -> 134 cycles Recursion -> 117 cycles Contention -> 323 cycles --- .../freertos/include/freertos/portmacro.h | 4 - components/freertos/port.c | 113 +------------- components/freertos/portmux_impl.h | 147 ++++++++++++++++++ components/freertos/tasks.c | 1 + 4 files changed, 149 insertions(+), 116 deletions(-) create mode 100644 components/freertos/portmux_impl.h diff --git a/components/freertos/include/freertos/portmacro.h b/components/freertos/include/freertos/portmacro.h index f55eeefe0..336737de9 100644 --- a/components/freertos/include/freertos/portmacro.h +++ b/components/freertos/include/freertos/portmacro.h @@ -199,8 +199,6 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux); #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *function, int line); portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *function, int line); -void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *function, int line); -portBASE_TYPE vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *function, int line); void vTaskEnterCritical( portMUX_TYPE *mux, const char *function, int line ); void vTaskExitCritical( portMUX_TYPE *mux, const char *function, int line ); #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux, __FUNCTION__, __LINE__) @@ -212,8 +210,6 @@ void vTaskExitCritical( portMUX_TYPE *mux ); void vTaskEnterCritical( portMUX_TYPE *mux ); void vPortCPUAcquireMutex(portMUX_TYPE *mux); void vPortCPUReleaseMutex(portMUX_TYPE *mux); -void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux); -void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux); #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux) #define portEXIT_CRITICAL(mux) vTaskExitCritical(mux) diff --git a/components/freertos/port.c b/components/freertos/port.c index fda5ed6cf..3d0fd192e 100644 --- a/components/freertos/port.c +++ b/components/freertos/port.c @@ -307,6 +307,7 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux) { mux->count=0; } +#include "portmux_impl.h" /* * For kernel use: Acquire a per-CPU mux. Spinlocks, so don't hold on to these muxes for too long. @@ -325,72 +326,6 @@ void vPortCPUAcquireMutex(portMUX_TYPE *mux) { } #endif -/* XOR one core ID with this value to get the other core ID */ -#define CORE_ID_XOR_SWAP (CORE_ID_PRO ^ CORE_ID_APP) - -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { -#else -void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { -#endif -#if !CONFIG_FREERTOS_UNICORE - uint32_t res; - portBASE_TYPE coreID, otherCoreID; -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - uint32_t timeout=(1<<16); - uint32_t owner = mux->owner; - if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { - ets_printf("ERROR: vPortCPUAcquireMutex: mux %p is uninitialized (0x%X)! Called from %s line %d.\n", mux, owner, fnName, line); - mux->owner=portMUX_FREE_VAL; - } -#endif - - /* Spin until we own the core */ - - RSR(PRID, coreID); - /* Note: coreID is the full 32 bit core ID (CORE_ID_PRO/CORE_ID_APP), - not the 0/1 value returned by xPortGetCoreID() - */ - otherCoreID = CORE_ID_XOR_SWAP ^ coreID; - do { - /* mux->owner should be one of portMUX_FREE_VAL, CORE_ID_PRO, - CORE_ID_APP: - - - If portMUX_FREE_VAL, we want to atomically set to 'coreID'. - - If "our" coreID, we can drop through immediately. - - If "otherCoreID", we spin here. - */ - res = coreID; - uxPortCompareSet(&mux->owner, portMUX_FREE_VAL, &res); - -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - timeout--; - if (timeout == 0) { - ets_printf("Timeout on mux! last non-recursive lock %s line %d, curr %s line %d\n", mux->lastLockedFn, mux->lastLockedLine, fnName, line); - ets_printf("Owner 0x%x count %d\n", mux->owner, mux->count); - } -#endif - } while (res == otherCoreID); - - assert(res == coreID || res == portMUX_FREE_VAL); /* any other value implies memory corruption or uninitialized mux */ - assert((res == portMUX_FREE_VAL) == (mux->count == 0)); /* we're first to lock iff count is zero */ - assert(mux->count < 0xFF); /* Bad count value implies memory corruption */ - - /* now we own it, we can increment the refcount */ - mux->count++; - - -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - if (res==portMUX_FREE_VAL) { //initial lock - mux->lastLockedFn=fnName; - mux->lastLockedLine=line; - } else { - ets_printf("Recursive lock: count=%d last non-recursive lock %s line %d, curr %s line %d\n", mux->count-1, - mux->lastLockedFn, mux->lastLockedLine, fnName, line); - } -#endif -#endif -} /* * For kernel use: Release a per-CPU mux @@ -411,58 +346,12 @@ void vPortCPUReleaseMutex(portMUX_TYPE *mux) { } #endif -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { -#else -void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { -#endif -#if !CONFIG_FREERTOS_UNICORE - portBASE_TYPE coreID; -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - const char *lastLockedFn=mux->lastLockedFn; - int lastLockedLine=mux->lastLockedLine; - mux->lastLockedFn=fnName; - mux->lastLockedLine=line; - uint32_t owner = mux->owner; - if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { - ets_printf("ERROR: vPortCPUReleaseMutex: mux %p is invalid (0x%x)!\n", mux, mux->owner); - } -#endif - -#if CONFIG_FREERTOS_PORTMUX_DEBUG || !defined(NDEBUG) - RSR(PRID, coreID); -#endif - -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - if (coreID != mux->owner) { - ets_printf("ERROR: vPortCPUReleaseMutex: mux %p was already unlocked!\n", mux); - ets_printf("Last non-recursive unlock %s line %d, curr unlock %s line %d\n", lastLockedFn, lastLockedLine, fnName, line); - } -#endif - - assert(coreID == mux->owner); // This is a mutex we didn't lock, or it's corrupt - assert(mux->count > 0); // Indicates memory corruption - assert(mux->count < 0x100); // Indicates memory corruption - - mux->count--; - if(mux->count == 0) { - mux->owner = portMUX_FREE_VAL; - } -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE - else { - ets_printf("Recursive unlock: count=%d last locked %s line %d, curr %s line %d\n", mux->count, lastLockedFn, lastLockedLine, fnName, line); - } -#endif -#endif //!CONFIG_FREERTOS_UNICORE -} - #if CONFIG_FREERTOS_BREAK_ON_SCHEDULER_START_JTAG void vPortFirstTaskHook(TaskFunction_t function) { esp_set_breakpoint_if_jtag(function); } #endif - void vPortSetStackWatchpoint( void* pxStackStart ) { //Set watchpoint 1 to watch the last 32 bytes of the stack. //Unfortunately, the Xtensa watchpoints can't set a watchpoint on a random [base - base+n] region because diff --git a/components/freertos/portmux_impl.h b/components/freertos/portmux_impl.h new file mode 100644 index 000000000..fece3aa38 --- /dev/null +++ b/components/freertos/portmux_impl.h @@ -0,0 +1,147 @@ +/* + Copyright (C) 2016-2017 Espressif Shanghai PTE LTD + Copyright (C) 2015 Real Time Engineers Ltd. + + All rights reserved + + FreeRTOS is free software; you can redistribute it and/or modify it under + the terms of the GNU General Public License (version 2) as published by the + Free Software Foundation >>!AND MODIFIED BY!<< the FreeRTOS exception. + + *************************************************************************** + >>! NOTE: The modification to the GPL is included to allow you to !<< + >>! distribute a combined work that includes FreeRTOS without being !<< + >>! obliged to provide the source code for proprietary components !<< + >>! outside of the FreeRTOS kernel. !<< + *************************************************************************** + + FreeRTOS is distributed in the hope that it will be useful, but WITHOUT ANY + WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + FOR A PARTICULAR PURPOSE. Full license text is available on the following + link: http://www.freertos.org/a00114.html +*/ + +/* This header exists for performance reasons, in order to inline the + implementation of vPortCPUAcquireMutexIntsDisabled and + vPortCPUReleaseMutexIntsDisabled into the + vTaskEnterCritical/vTaskExitCritical functions as well as the + vPortCPUAcquireMutex/vPortCPUReleaseMutex implementations. + + Normally this kind of performance hack is over the top, but these + functions get used a great deal by FreeRTOS internals. + + It should be #included by freertos port.c or tasks.c, in esp-idf. +*/ +#include "soc/cpu.h" + +/* XOR one core ID with this value to get the other core ID */ +#define CORE_ID_XOR_SWAP (CORE_ID_PRO ^ CORE_ID_APP) + +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG +static inline void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { +#else +static inline void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { +#endif +#if !CONFIG_FREERTOS_UNICORE + uint32_t res; + portBASE_TYPE coreID, otherCoreID; +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + uint32_t timeout=(1<<16); + uint32_t owner = mux->owner; + if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { + ets_printf("ERROR: vPortCPUAcquireMutex: mux %p is uninitialized (0x%X)! Called from %s line %d.\n", mux, owner, fnName, line); + mux->owner=portMUX_FREE_VAL; + } +#endif + + /* Spin until we own the core */ + + RSR(PRID, coreID); + /* Note: coreID is the full 32 bit core ID (CORE_ID_PRO/CORE_ID_APP), + not the 0/1 value returned by xPortGetCoreID() + */ + otherCoreID = CORE_ID_XOR_SWAP ^ coreID; + do { + /* mux->owner should be one of portMUX_FREE_VAL, CORE_ID_PRO, + CORE_ID_APP: + + - If portMUX_FREE_VAL, we want to atomically set to 'coreID'. + - If "our" coreID, we can drop through immediately. + - If "otherCoreID", we spin here. + */ + res = coreID; + uxPortCompareSet(&mux->owner, portMUX_FREE_VAL, &res); + +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + timeout--; + if (timeout == 0) { + ets_printf("Timeout on mux! last non-recursive lock %s line %d, curr %s line %d\n", mux->lastLockedFn, mux->lastLockedLine, fnName, line); + ets_printf("Owner 0x%x count %d\n", mux->owner, mux->count); + } +#endif + } while (res == otherCoreID); + + assert(res == coreID || res == portMUX_FREE_VAL); /* any other value implies memory corruption or uninitialized mux */ + assert((res == portMUX_FREE_VAL) == (mux->count == 0)); /* we're first to lock iff count is zero */ + assert(mux->count < 0xFF); /* Bad count value implies memory corruption */ + + /* now we own it, we can increment the refcount */ + mux->count++; + + +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + if (res==portMUX_FREE_VAL) { //initial lock + mux->lastLockedFn=fnName; + mux->lastLockedLine=line; + } else { + ets_printf("Recursive lock: count=%d last non-recursive lock %s line %d, curr %s line %d\n", mux->count-1, + mux->lastLockedFn, mux->lastLockedLine, fnName, line); + } +#endif +#endif +} + +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG +static inline void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { +#else +static inline void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { +#endif +#if !CONFIG_FREERTOS_UNICORE + portBASE_TYPE coreID; +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + const char *lastLockedFn=mux->lastLockedFn; + int lastLockedLine=mux->lastLockedLine; + mux->lastLockedFn=fnName; + mux->lastLockedLine=line; + uint32_t owner = mux->owner; + if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { + ets_printf("ERROR: vPortCPUReleaseMutex: mux %p is invalid (0x%x)!\n", mux, mux->owner); + } +#endif + +#if CONFIG_FREERTOS_PORTMUX_DEBUG || !defined(NDEBUG) + RSR(PRID, coreID); +#endif + +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + if (coreID != mux->owner) { + ets_printf("ERROR: vPortCPUReleaseMutex: mux %p was already unlocked!\n", mux); + ets_printf("Last non-recursive unlock %s line %d, curr unlock %s line %d\n", lastLockedFn, lastLockedLine, fnName, line); + } +#endif + + assert(coreID == mux->owner); // This is a mutex we didn't lock, or it's corrupt + assert(mux->count > 0); // Indicates memory corruption + assert(mux->count < 0x100); // Indicates memory corruption + + mux->count--; + if(mux->count == 0) { + mux->owner = portMUX_FREE_VAL; + } +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE + else { + ets_printf("Recursive unlock: count=%d last locked %s line %d, curr %s line %d\n", mux->count, lastLockedFn, lastLockedLine, fnName, line); + } +#endif +#endif //!CONFIG_FREERTOS_UNICORE +} diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 74dec7fbd..7101c5a18 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -4103,6 +4103,7 @@ For ESP32 FreeRTOS, vTaskEnterCritical implements both portENTER_CRITICAL and po #if ( portCRITICAL_NESTING_IN_TCB == 1 ) +#include "portmux_impl.h" #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vTaskEnterCritical( portMUX_TYPE *mux, const char *function, int line ) From 397c0bfb4bfb1f6e6bb90283fc3bb4695e36bd7e Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 26 Jun 2017 14:09:02 +1000 Subject: [PATCH 5/9] freertos scheduler test: Free timer group interrupt handle when test finishes --- components/freertos/test/test_suspend_scheduler.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/components/freertos/test/test_suspend_scheduler.c b/components/freertos/test/test_suspend_scheduler.c index 3d2783b4f..d3c8ed86a 100644 --- a/components/freertos/test/test_suspend_scheduler.c +++ b/components/freertos/test/test_suspend_scheduler.c @@ -49,6 +49,7 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" isr_count = 0; isr_semaphore = xSemaphoreCreateMutex(); TaskHandle_t counter_task; + intr_handle_t isr_handle = NULL; xTaskCreatePinnedToCore(counter_task_fn, "counter", 2048, NULL, UNITY_FREERTOS_PRIORITY + 1, @@ -69,7 +70,7 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" timer_set_counter_value(TIMER_GROUP_0, TIMER_0, 0); timer_set_alarm_value(TIMER_GROUP_0, TIMER_0, 1000); timer_enable_intr(TIMER_GROUP_0, TIMER_0); - timer_isr_register(TIMER_GROUP_0, TIMER_0, timer_group0_isr, NULL, 0, NULL); + timer_isr_register(TIMER_GROUP_0, TIMER_0, timer_group0_isr, NULL, 0, &isr_handle); timer_start(TIMER_GROUP_0, TIMER_0); vTaskDelay(5); @@ -101,6 +102,9 @@ TEST_CASE("Handle pending context switch while scheduler disabled", "[freertos]" TEST_ASSERT_NOT_EQUAL(task_count, no_sched_task); } + esp_intr_free(isr_handle); + timer_disable_intr(TIMER_GROUP_0, TIMER_0); + vTaskDelete(counter_task); vSemaphoreDelete(isr_semaphore); } From 4486d4cb10370095349d3f38e17b2652225b2e26 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 20 Jul 2017 16:34:45 +1000 Subject: [PATCH 6/9] portmux: Add vPortCPUAcquireMutexTimeout() function Refactor app_trace locking to use this function. --- components/app_trace/app_trace_util.c | 84 ++++--------------- .../app_trace/include/esp_app_trace_util.h | 10 +-- .../freertos/include/freertos/portmacro.h | 17 ++++ components/freertos/port.c | 19 ++++- components/freertos/portmux_impl.h | 52 ++++++++---- components/freertos/tasks.c | 4 +- 6 files changed, 95 insertions(+), 91 deletions(-) diff --git a/components/app_trace/app_trace_util.c b/components/app_trace/app_trace_util.c index 078bd0d7f..e268c81ab 100644 --- a/components/app_trace/app_trace_util.c +++ b/components/app_trace/app_trace_util.c @@ -22,6 +22,7 @@ // TODO: get actual clock from PLL config #define ESP_APPTRACE_CPUTICKS2US(_t_) ((_t_)/(XT_CLOCK_FREQ/1000000)) +#define ESP_APPTRACE_US2CPUTICKS(_t_) ((_t_)*(XT_CLOCK_FREQ/1000000)) esp_err_t esp_apptrace_tmo_check(esp_apptrace_tmo_t *tmo) { @@ -45,81 +46,30 @@ esp_err_t esp_apptrace_tmo_check(esp_apptrace_tmo_t *tmo) esp_err_t esp_apptrace_lock_take(esp_apptrace_lock_t *lock, esp_apptrace_tmo_t *tmo) { - uint32_t res; -#if CONFIG_SYSVIEW_ENABLE - uint32_t recCnt; -#endif + lock->int_state = portENTER_CRITICAL_NESTED(); - while (1) { - res = (xPortGetCoreID() << portMUX_VAL_SHIFT) | portMUX_MAGIC_VAL; - // first disable IRQs on this CPU, this will prevent current task from been - // preempted by higher prio tasks, otherwise deadlock can happen: - // when lower prio task took mux and then preempted by higher prio one which also tries to - // get mux with INFINITE timeout - unsigned int irq_stat = portENTER_CRITICAL_NESTED(); - // Now try to lock mux - uxPortCompareSet(&lock->portmux.mux, portMUX_FREE_VAL, &res); - if (res == portMUX_FREE_VAL) { - // do not enable IRQs, we will held them disabled until mux is unlocked - // we do not need to flush cache region for mux->irq_stat because it is used - // to hold and restore IRQ state only for CPU which took mux, other CPUs will not use this value - lock->irq_stat = irq_stat; - break; - } -#if CONFIG_SYSVIEW_ENABLE - else if (((res & portMUX_VAL_MASK) >> portMUX_VAL_SHIFT) == xPortGetCoreID()) { - recCnt = (res & portMUX_CNT_MASK) >> portMUX_CNT_SHIFT; - recCnt++; - // ets_printf("Recursive lock: recCnt=%d\n", recCnt); - lock->portmux.mux = portMUX_MAGIC_VAL | (recCnt << portMUX_CNT_SHIFT) | (xPortGetCoreID() << portMUX_VAL_SHIFT); - break; - } -#endif - // if mux is locked by other task/ISR enable IRQs and let other guys work - portEXIT_CRITICAL_NESTED(irq_stat); + unsigned now = ESP_APPTRACE_CPUTICKS2US(portGET_RUN_TIME_COUNTER_VALUE()); // us + unsigned end = tmo->start + tmo->tmo; + if (now > end) { + goto timeout; + } + unsigned remaining = end - now; // us - int err = esp_apptrace_tmo_check(tmo); - if (err != ESP_OK) { - return err; - } + bool success = vPortCPUAcquireMutexTimeout(&lock->mux, ESP_APPTRACE_US2CPUTICKS(remaining)); + if (success) { + return ESP_OK; } - return ESP_OK; + timeout: + portEXIT_CRITICAL_NESTED(lock->int_state); + return ESP_ERR_TIMEOUT; } esp_err_t esp_apptrace_lock_give(esp_apptrace_lock_t *lock) { - esp_err_t ret = ESP_OK; - uint32_t res = 0; - unsigned int irq_stat; -#if CONFIG_SYSVIEW_ENABLE - uint32_t recCnt; -#endif - res = portMUX_FREE_VAL; - - // first of all save a copy of IRQ status for this locker because uxPortCompareSet will unlock mux and tasks/ISRs - // from other core can overwrite mux->irq_stat - irq_stat = lock->irq_stat; - uxPortCompareSet(&lock->portmux.mux, (xPortGetCoreID() << portMUX_VAL_SHIFT) | portMUX_MAGIC_VAL, &res); - - if ( ((res & portMUX_VAL_MASK) >> portMUX_VAL_SHIFT) == xPortGetCoreID() ) { -#if CONFIG_SYSVIEW_ENABLE - //Lock is valid, we can return safely. Just need to check if it's a recursive lock; if so we need to decrease the refcount. - if ( ((res & portMUX_CNT_MASK) >> portMUX_CNT_SHIFT) != 0) { - //We locked this, but the reccount isn't zero. Decrease refcount and continue. - recCnt = (res & portMUX_CNT_MASK) >> portMUX_CNT_SHIFT; - recCnt--; - lock->portmux.mux = portMUX_MAGIC_VAL | (recCnt << portMUX_CNT_SHIFT) | (xPortGetCoreID() << portMUX_VAL_SHIFT); - } -#endif - } else if ( res == portMUX_FREE_VAL ) { - ret = ESP_FAIL; // should never get here - } else { - ret = ESP_FAIL; // should never get here - } - // restore local interrupts - portEXIT_CRITICAL_NESTED(irq_stat); - return ret; + vPortCPUReleaseMutex(&lock->mux); + portEXIT_CRITICAL_NESTED(lock->int_state); + return ESP_OK; } /////////////////////////////////////////////////////////////////////////////// diff --git a/components/app_trace/include/esp_app_trace_util.h b/components/app_trace/include/esp_app_trace_util.h index 979337869..e689d4502 100644 --- a/components/app_trace/include/esp_app_trace_util.h +++ b/components/app_trace/include/esp_app_trace_util.h @@ -59,8 +59,8 @@ static inline uint32_t esp_apptrace_tmo_remaining_us(esp_apptrace_tmo_t *tmo) /** Tracing module synchronization lock */ typedef struct { - volatile unsigned int irq_stat; ///< local (on 1 CPU) IRQ state - portMUX_TYPE portmux; ///< mux for synchronization + portMUX_TYPE mux; + unsigned int_state; } esp_apptrace_lock_t; /** @@ -70,8 +70,8 @@ typedef struct { */ static inline void esp_apptrace_lock_init(esp_apptrace_lock_t *lock) { - lock->portmux.mux = portMUX_FREE_VAL; - lock->irq_stat = 0; + vPortCPUInitializeMutex(&lock->mux); + lock->int_state = 0; } /** @@ -163,4 +163,4 @@ uint32_t esp_apptrace_rb_read_size_get(esp_apptrace_rb_t *rb); */ uint32_t esp_apptrace_rb_write_size_get(esp_apptrace_rb_t *rb); -#endif //ESP_APP_TRACE_UTIL_H_ \ No newline at end of file +#endif //ESP_APP_TRACE_UTIL_H_ diff --git a/components/freertos/include/freertos/portmacro.h b/components/freertos/include/freertos/portmacro.h index 336737de9..d398ba5da 100644 --- a/components/freertos/include/freertos/portmacro.h +++ b/components/freertos/include/freertos/portmacro.h @@ -146,6 +146,10 @@ typedef struct { #define portMUX_FREE_VAL 0xB33FFFFF +/* Special constants for vPortCPUAcquireMutexTimeout() */ +#define portMUX_NO_TIMEOUT (-1) /* When passed for 'timeout_cycles', spin forever if necessary */ +#define portMUX_TRY_LOCK 0 /* Try to acquire the spinlock a single time only */ + //Keep this in sync with the portMUX_TYPE struct definition please. #ifndef CONFIG_FREERTOS_PORTMUX_DEBUG #define portMUX_INITIALIZER_UNLOCKED { \ @@ -198,7 +202,10 @@ behaviour; please keep this in mind if you need any compatibility with other Fre void vPortCPUInitializeMutex(portMUX_TYPE *mux); #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *function, int line); +bool vPortCPUAcquireMutexTimeout(portMUX_TYPE *mux, int timeout_cycles, const char *function, int line); portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *function, int line); + + void vTaskEnterCritical( portMUX_TYPE *mux, const char *function, int line ); void vTaskExitCritical( portMUX_TYPE *mux, const char *function, int line ); #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux, __FUNCTION__, __LINE__) @@ -209,6 +216,16 @@ void vTaskExitCritical( portMUX_TYPE *mux, const char *function, int line ); void vTaskExitCritical( portMUX_TYPE *mux ); void vTaskEnterCritical( portMUX_TYPE *mux ); void vPortCPUAcquireMutex(portMUX_TYPE *mux); + +/** @brief Acquire a portmux spinlock with a timeout + * + * @param mux Pointer to portmux to acquire. + * @param timeout_cycles Timeout to spin, in CPU cycles. Pass portMUX_NO_TIMEOUT to wait forever, + * portMUX_TRY_LOCK to try a single time to acquire the lock. + * + * @return true if mutex is successfully acquired, false on timeout. + */ +bool vPortCPUAcquireMutexTimeout(portMUX_TYPE *mux, int timeout_cycles); void vPortCPUReleaseMutex(portMUX_TYPE *mux); #define portENTER_CRITICAL(mux) vTaskEnterCritical(mux) diff --git a/components/freertos/port.c b/components/freertos/port.c index 3d0fd192e..c217dc44a 100644 --- a/components/freertos/port.c +++ b/components/freertos/port.c @@ -315,15 +315,30 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux) { #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *fnName, int line) { unsigned int irqStatus = portENTER_CRITICAL_NESTED(); - vPortCPUAcquireMutexIntsDisabled(mux, fnName, line); + vPortCPUAcquireMutexIntsDisabled(mux, portMUX_NO_TIMEOUT, fnName, line); portEXIT_CRITICAL_NESTED(irqStatus); } + +bool vPortCPUAcquireMutexTimeout(portMUX_TYPE *mux, uint32_t timeout_cycles, const char *fnName, int line) { + unsigned int irqStatus = portENTER_CRITICAL_NESTED(); + bool result = vPortCPUAcquireMutexIntsDisabled(mux, timeout_cycles, fnName, line); + portEXIT_CRITICAL_NESTED(irqStatus); + return result; +} + #else void vPortCPUAcquireMutex(portMUX_TYPE *mux) { unsigned int irqStatus = portENTER_CRITICAL_NESTED(); - vPortCPUAcquireMutexIntsDisabled(mux); + vPortCPUAcquireMutexIntsDisabled(mux, portMUX_NO_TIMEOUT); portEXIT_CRITICAL_NESTED(irqStatus); } + +bool vPortCPUAcquireMutexTimeout(portMUX_TYPE *mux, int timeout_cycles) { + unsigned int irqStatus = portENTER_CRITICAL_NESTED(); + bool result = vPortCPUAcquireMutexIntsDisabled(mux, timeout_cycles); + portEXIT_CRITICAL_NESTED(irqStatus); + return result; +} #endif diff --git a/components/freertos/portmux_impl.h b/components/freertos/portmux_impl.h index fece3aa38..7bb6bcc9d 100644 --- a/components/freertos/portmux_impl.h +++ b/components/freertos/portmux_impl.h @@ -24,11 +24,12 @@ /* This header exists for performance reasons, in order to inline the implementation of vPortCPUAcquireMutexIntsDisabled and vPortCPUReleaseMutexIntsDisabled into the - vTaskEnterCritical/vTaskExitCritical functions as well as the + vTaskEnterCritical/vTaskExitCritical functions in task.c as well as the vPortCPUAcquireMutex/vPortCPUReleaseMutex implementations. - Normally this kind of performance hack is over the top, but these - functions get used a great deal by FreeRTOS internals. + Normally this kind of performance hack is over the top, but + vTaskEnterCritical/vTaskExitCritical is called a great + deal by FreeRTOS internals. It should be #included by freertos port.c or tasks.c, in esp-idf. */ @@ -37,16 +38,28 @@ /* XOR one core ID with this value to get the other core ID */ #define CORE_ID_XOR_SWAP (CORE_ID_PRO ^ CORE_ID_APP) +static inline bool __attribute__((always_inline)) #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -static inline void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { +vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, int timeout_cycles, const char *fnName, int line) { #else -static inline void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { +vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux, int timeout_cycles) { #endif #if !CONFIG_FREERTOS_UNICORE uint32_t res; portBASE_TYPE coreID, otherCoreID; + uint32_t ccount_start; + bool set_timeout = timeout_cycles > portMUX_NO_TIMEOUT; +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + if (!set_timeout) { + timeout_cycles = 10000; // Always set a timeout in debug mode + set_timeout = true; + } +#endif + if (set_timeout) { // Timeout + RSR(CCOUNT, ccount_start); + } + #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - uint32_t timeout=(1<<16); uint32_t owner = mux->owner; if (owner != portMUX_FREE_VAL && owner != CORE_ID_PRO && owner != CORE_ID_APP) { ets_printf("ERROR: vPortCPUAcquireMutex: mux %p is uninitialized (0x%X)! Called from %s line %d.\n", mux, owner, fnName, line); @@ -72,14 +85,22 @@ static inline void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { res = coreID; uxPortCompareSet(&mux->owner, portMUX_FREE_VAL, &res); -#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - timeout--; - if (timeout == 0) { - ets_printf("Timeout on mux! last non-recursive lock %s line %d, curr %s line %d\n", mux->lastLockedFn, mux->lastLockedLine, fnName, line); - ets_printf("Owner 0x%x count %d\n", mux->owner, mux->count); + if (res != otherCoreID) { + break; // mux->owner is "our" coreID } + + if (set_timeout) { + uint32_t ccount_now; + RSR(CCOUNT, ccount_now); + if (ccount_now - ccount_start > (unsigned)timeout_cycles) { +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG + ets_printf("Timeout on mux! last non-recursive lock %s line %d, curr %s line %d\n", mux->lastLockedFn, mux->lastLockedLine, fnName, line); + ets_printf("Owner 0x%x count %d\n", mux->owner, mux->count); #endif - } while (res == otherCoreID); + return false; + } + } + } while (1); assert(res == coreID || res == portMUX_FREE_VAL); /* any other value implies memory corruption or uninitialized mux */ assert((res == portMUX_FREE_VAL) == (mux->count == 0)); /* we're first to lock iff count is zero */ @@ -97,12 +118,13 @@ static inline void vPortCPUAcquireMutexIntsDisabled(portMUX_TYPE *mux) { ets_printf("Recursive lock: count=%d last non-recursive lock %s line %d, curr %s line %d\n", mux->count-1, mux->lastLockedFn, mux->lastLockedLine, fnName, line); } -#endif -#endif +#endif /* CONFIG_FREERTOS_PORTMUX_DEBUG */ +#endif /* CONFIG_FREERTOS_UNICORE */ + return true; } #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG -static inline void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { + static inline void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux, const char *fnName, int line) { #else static inline void vPortCPUReleaseMutexIntsDisabled(portMUX_TYPE *mux) { #endif diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 7101c5a18..396ad0cd7 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -4121,9 +4121,9 @@ For ESP32 FreeRTOS, vTaskEnterCritical implements both portENTER_CRITICAL and po oldInterruptLevel=portENTER_CRITICAL_NESTED(); } #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG - vPortCPUAcquireMutexIntsDisabled( mux, function, line ); + vPortCPUAcquireMutexIntsDisabled( mux, portMUX_NO_TIMEOUT, function, line ); #else - vPortCPUAcquireMutexIntsDisabled( mux ); + vPortCPUAcquireMutexIntsDisabled( mux, portMUX_NO_TIMEOUT ); #endif if( schedulerRunning != pdFALSE ) From a19aaf2072e3320f9d99d1b19efcf020623f5bcd Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 31 Aug 2017 10:49:42 +1000 Subject: [PATCH 7/9] esp32: Update wifi lib to use new spinlock implementation --- components/esp32/lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/esp32/lib b/components/esp32/lib index 64b0ff419..fc2118052 160000 --- a/components/esp32/lib +++ b/components/esp32/lib @@ -1 +1 @@ -Subproject commit 64b0ff4199614a8a5066fe3a05d446acb52575e6 +Subproject commit fc2118052ec3863e228840d15df58bc66ff0ce57 From b3c6748a0bce80283f13a23da0e0d85b5575e10c Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 4 Sep 2017 20:39:35 +0800 Subject: [PATCH 8/9] ci: add extra unit test job --- .gitlab-ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9e68170ce..a480e580b 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -538,6 +538,12 @@ UT_001_06: - ESP32_IDF - UT_T1_SPIMODE +UT_001_07: + <<: *unit_test_template + tags: + - ESP32_IDF + - UT_T1_1 + IT_001_01: <<: *test_template tags: From cf29dd47a9bcde7f91e7ef3fcb8f912bafedd22a Mon Sep 17 00:00:00 2001 From: Alexey Gerenkov Date: Sun, 3 Sep 2017 03:44:21 +0300 Subject: [PATCH 9/9] apptrace lock acquire function was re-designed to minimize waiting time with disabled IRQs --- components/app_trace/app_trace_util.c | 40 +++++++++++-------- .../Config/SEGGER_SYSVIEW_Config_FreeRTOS.c | 2 +- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/components/app_trace/app_trace_util.c b/components/app_trace/app_trace_util.c index e268c81ab..524a861ab 100644 --- a/components/app_trace/app_trace_util.c +++ b/components/app_trace/app_trace_util.c @@ -46,29 +46,37 @@ esp_err_t esp_apptrace_tmo_check(esp_apptrace_tmo_t *tmo) esp_err_t esp_apptrace_lock_take(esp_apptrace_lock_t *lock, esp_apptrace_tmo_t *tmo) { - lock->int_state = portENTER_CRITICAL_NESTED(); + int res; - unsigned now = ESP_APPTRACE_CPUTICKS2US(portGET_RUN_TIME_COUNTER_VALUE()); // us - unsigned end = tmo->start + tmo->tmo; - if (now > end) { - goto timeout; + while (1) { + // do not overwrite lock->int_state before we actually acquired the mux + unsigned int_state = portENTER_CRITICAL_NESTED(); + // FIXME: if mux is busy it is not good idea to loop during the whole tmo with disabled IRQs. + // So we check mux state using zero tmo, restore IRQs and let others tasks/IRQs to run on this CPU + // while we are doing our own tmo check. + bool success = vPortCPUAcquireMutexTimeout(&lock->mux, 0); + if (success) { + lock->int_state = int_state; + return ESP_OK; + } + portEXIT_CRITICAL_NESTED(int_state); + // we can be preempted from this place till the next call (above) to portENTER_CRITICAL_NESTED() + res = esp_apptrace_tmo_check(tmo); + if (res != ESP_OK) { + break; + } } - unsigned remaining = end - now; // us - - bool success = vPortCPUAcquireMutexTimeout(&lock->mux, ESP_APPTRACE_US2CPUTICKS(remaining)); - if (success) { - return ESP_OK; - } - - timeout: - portEXIT_CRITICAL_NESTED(lock->int_state); - return ESP_ERR_TIMEOUT; + return res; } esp_err_t esp_apptrace_lock_give(esp_apptrace_lock_t *lock) { + // save lock's irq state value for this CPU + unsigned int_state = lock->int_state; + // after call to the following func we can not be sure that lock->int_state + // is not overwritten by other CPU who has acquired the mux just after we released it. See esp_apptrace_lock_take(). vPortCPUReleaseMutex(&lock->mux); - portEXIT_CRITICAL_NESTED(lock->int_state); + portEXIT_CRITICAL_NESTED(int_state); return ESP_OK; } diff --git a/components/app_trace/sys_view/Sample/Config/SEGGER_SYSVIEW_Config_FreeRTOS.c b/components/app_trace/sys_view/Sample/Config/SEGGER_SYSVIEW_Config_FreeRTOS.c index 52bc27d06..87d76f32a 100644 --- a/components/app_trace/sys_view/Sample/Config/SEGGER_SYSVIEW_Config_FreeRTOS.c +++ b/components/app_trace/sys_view/Sample/Config/SEGGER_SYSVIEW_Config_FreeRTOS.c @@ -115,7 +115,7 @@ static timer_group_t s_ts_timer_group; // everything is fine, so for multi-core env we have to wait on underlying lock forever #define SEGGER_LOCK_WAIT_TMO ESP_APPTRACE_TMO_INFINITE -static esp_apptrace_lock_t s_sys_view_lock = {.irq_stat = 0, .portmux = portMUX_INITIALIZER_UNLOCKED}; +static esp_apptrace_lock_t s_sys_view_lock = {.mux = portMUX_INITIALIZER_UNLOCKED, .int_state = 0}; static const char * const s_isr_names[] = { [0] = "WIFI_MAC",