From 4486d4cb10370095349d3f38e17b2652225b2e26 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 20 Jul 2017 16:34:45 +1000 Subject: [PATCH] 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 )