From 189527a03cd3b5ee8d9867d7b933ef51120275a4 Mon Sep 17 00:00:00 2001 From: swoiszwillo Date: Sun, 5 Jul 2026 15:58:57 -0600 Subject: [PATCH] refactor(zkernel): back k_mutex with FreeRTOS recursive mutex FreeRTOS recursive mutexes already provide both re-entrancy and priority inheritance: a blocking xSemaphoreTakeRecursive routes through xQueueSemaphoreTake, which priority-inherits for any mutex-type queue (gated only on uxQueueType == queueQUEUE_IS_MUTEX, set for recursive and non-recursive alike). The prior non-recursive-mutex + manual owner/count implementation was built on the false premise that recursive mutexes lack PI. Collapse k_mutex to a thin wrapper over the recursive primitive and drop the manual re-entrancy state. Public API (init/lock/unlock), error codes, and ISR handling are unchanged. Also removes the CONFIG_ZKERNEL_MUTEX_DEBUG lock-ordering / hold-time instrumentation and the ordered-mutex API, which existed only to feed off that manual state and were not yet justified -- safer to ride the battle-tested FreeRTOS primitive. Verified: linux host build clean, 224 tests pass including all 8 mutex tests and the 3 priority-inheritance tests. BREAKING CHANGE: removes k_mutex_init_ordered, K_MUTEX_DEFINE_ORDERED, CONFIG_ZKERNEL_MUTEX_DEBUG, and CONFIG_ZKERNEL_MUTEX_HOLD_WARNING_MS (Boreas extensions, not Zephyr API; shipped in v0.1.0). Co-Authored-By: Claude Opus 4.8 --- components/zkernel/Kconfig | 16 ----- components/zkernel/README.md | 6 +- .../zkernel/include/boreas/zephyr/kernel.h | 24 +++---- components/zkernel/src/k_mutex.c | 71 +++---------------- test/sdkconfig.defaults | 2 - 5 files changed, 20 insertions(+), 99 deletions(-) diff --git a/components/zkernel/Kconfig b/components/zkernel/Kconfig index ed1b15c..b8013b3 100644 --- a/components/zkernel/Kconfig +++ b/components/zkernel/Kconfig @@ -11,22 +11,6 @@ menu "Boreas Kernel (zkernel)" the 32-bit target). Mirrors upstream Zephyr's CONFIG_RING_BUFFER_LARGE. - config ZKERNEL_MUTEX_DEBUG - bool "Enable mutex lock-ordering assertions" - default n - help - When enabled, each mutex is assigned a numeric lock order. - Acquiring mutexes out of order triggers an assertion failure. - Adds ~4 bytes per mutex and a small runtime check on lock. - - config ZKERNEL_MUTEX_HOLD_WARNING_MS - int "Mutex hold-time warning threshold (ms)" - default 0 - depends on ZKERNEL_MUTEX_DEBUG - help - Log a warning if a mutex is held longer than this many - milliseconds. Set to 0 to disable hold-time warnings. - config ZKERNEL_THREAD_ANALYZER bool "Enable thread analyzer" default n diff --git a/components/zkernel/README.md b/components/zkernel/README.md index b99900d..2765dcd 100644 --- a/components/zkernel/README.md +++ b/components/zkernel/README.md @@ -113,9 +113,7 @@ k_mutex_unlock(&mtx); Returns `-EBUSY` (K_NO_WAIT), `-EAGAIN` (timeout), `-EWOULDBLOCK` (called from ISR), `-EPERM` (unlock not owner). -Re-entrant (same thread can lock multiple times) with priority inheritance. Uses a non-recursive FreeRTOS mutex (which has PI) with manual re-entrancy tracking. - -**Extension:** `CONFIG_ZKERNEL_MUTEX_DEBUG` enables lock-ordering assertions and hold-time warnings. +Re-entrant (same thread can lock multiple times) with priority inheritance. Thin wrapper over a FreeRTOS recursive mutex, which provides both. ## Message Queue (`k_msgq`) @@ -296,8 +294,6 @@ Ordered initialization. Entries are emplaced into the `.sys_init_entries` linker | Option | Default | Description | |--------|---------|-------------| -| `CONFIG_ZKERNEL_MUTEX_DEBUG` | n | Lock-ordering assertions + hold-time warnings | -| `CONFIG_ZKERNEL_MUTEX_HOLD_WARNING_MS` | 0 | Hold-time warning threshold (0=disabled) | | `CONFIG_ZKERNEL_SYS_INIT` | y | Enable SYS_INIT framework | | `CONFIG_ZKERNEL_SYS_INIT_MAX_ENTRIES` | 32 | Max SYS_INIT registrations | | `CONFIG_ZKERNEL_FATAL_CAPTURE` | n | Save fatal context to NVS | diff --git a/components/zkernel/include/boreas/zephyr/kernel.h b/components/zkernel/include/boreas/zephyr/kernel.h index 60b23a6..104ac3c 100644 --- a/components/zkernel/include/boreas/zephyr/kernel.h +++ b/components/zkernel/include/boreas/zephyr/kernel.h @@ -246,20 +246,21 @@ unsigned int k_sem_count_get(struct k_sem *sem); /* ---------------------------------------------------------------- * Mutex * - * Uses a non-recursive FreeRTOS mutex (which has priority inheritance) - * with manual re-entrancy tracking. This gives Zephyr-compatible - * behavior: re-entrant locking AND priority inheritance. + * Thin wrapper over a FreeRTOS recursive mutex, which already provides + * both re-entrant locking and priority inheritance -- matching Zephyr's + * k_mutex semantics. + * + * Divergences from upstream Zephyr (kernel/mutex.c): + * - ISR context: Zephyr asserts (thread-only API); Boreas returns + * -EWOULDBLOCK instead of faulting. + * - k_mutex_unlock returns -EPERM whenever the caller isn't the owner, + * including an unlocked mutex; Zephyr returns -EINVAL for the + * never-locked case. * ---------------------------------------------------------------- */ struct k_mutex { SemaphoreHandle_t handle; StaticSemaphore_t buffer; - TaskHandle_t owner; /* current owner for re-entrancy */ - uint32_t count; /* recursion depth */ -#if defined(CONFIG_ZKERNEL_MUTEX_DEBUG) - uint8_t order; /* lock ordering -- lower must be acquired first */ - uint32_t lock_time; /* tick count at lock acquisition */ -#endif }; #define K_MUTEX_DEFINE(name) struct k_mutex name = {0} @@ -268,11 +269,6 @@ int k_mutex_init(struct k_mutex *mutex); int k_mutex_lock(struct k_mutex *mutex, k_timeout_t timeout); int k_mutex_unlock(struct k_mutex *mutex); -#if defined(CONFIG_ZKERNEL_MUTEX_DEBUG) -int k_mutex_init_ordered(struct k_mutex *mutex, uint8_t order); -#define K_MUTEX_DEFINE_ORDERED(name, ord) struct k_mutex name = {.order = (ord)} -#endif - /* ---------------------------------------------------------------- * Message Queue * ---------------------------------------------------------------- */ diff --git a/components/zkernel/src/k_mutex.c b/components/zkernel/src/k_mutex.c index 907c4e1..6992d92 100644 --- a/components/zkernel/src/k_mutex.c +++ b/components/zkernel/src/k_mutex.c @@ -4,10 +4,11 @@ * * Zephyr k_mutex: re-entrant mutex with priority inheritance. * - * Uses a non-recursive FreeRTOS mutex (which has PI) with manual - * re-entrancy tracking via owner + count. This matches Zephyr - * semantics: same thread can lock multiple times, and waiters - * boost the holder's priority. + * Thin wrapper over a FreeRTOS recursive mutex, which already provides + * both re-entrancy (recursive take/give) and priority inheritance -- + * matching Zephyr's k_mutex semantics. The blocking take routes through + * xQueueSemaphoreTake, which priority-inherits for any mutex-type queue, + * so no manual owner/count tracking is needed. */ #include "zephyr/kernel.h" @@ -20,58 +21,23 @@ static const char *TAG = "k_mutex"; int k_mutex_init(struct k_mutex *mutex) { - mutex->handle = xSemaphoreCreateMutexStatic(&mutex->buffer); + mutex->handle = xSemaphoreCreateRecursiveMutexStatic(&mutex->buffer); if (mutex->handle == NULL) { ESP_LOGE(TAG, "Failed to create mutex"); return -ENOMEM; } - mutex->owner = NULL; - mutex->count = 0; -#if defined(CONFIG_ZKERNEL_MUTEX_DEBUG) - mutex->order = 0; - mutex->lock_time = 0; -#endif return 0; } -#if defined(CONFIG_ZKERNEL_MUTEX_DEBUG) -int k_mutex_init_ordered(struct k_mutex *mutex, uint8_t order) -{ - int ret = k_mutex_init(mutex); - if (ret == 0) { - mutex->order = order; - } - return ret; -} -#endif - int k_mutex_lock(struct k_mutex *mutex, k_timeout_t timeout) { if (xPortInIsrContext()) { return -EWOULDBLOCK; } - TaskHandle_t current = xTaskGetCurrentTaskHandle(); - - /* Re-entrant: if we already own it, just bump the count */ - if (mutex->owner == current) { - mutex->count++; - return 0; - } - - /* First acquisition: take the PI-enabled mutex */ - BaseType_t ret = xSemaphoreTake(mutex->handle, k_timeout_to_ticks(timeout)); - if (ret != pdTRUE) { + if (xSemaphoreTakeRecursive(mutex->handle, k_timeout_to_ticks(timeout)) != pdTRUE) { return k_timeout_is_no_wait(timeout) ? -EBUSY : -EAGAIN; } - - mutex->owner = current; - mutex->count = 1; - -#if defined(CONFIG_ZKERNEL_MUTEX_DEBUG) - mutex->lock_time = xTaskGetTickCount(); -#endif - return 0; } @@ -81,28 +47,9 @@ int k_mutex_unlock(struct k_mutex *mutex) return -EWOULDBLOCK; } - TaskHandle_t current = xTaskGetCurrentTaskHandle(); - - if (mutex->owner != current) { + /* Recursive give returns pdFALSE when the caller isn't the owner. */ + if (xSemaphoreGiveRecursive(mutex->handle) != pdTRUE) { return -EPERM; } - -#if defined(CONFIG_ZKERNEL_MUTEX_DEBUG) -#if CONFIG_ZKERNEL_MUTEX_HOLD_WARNING_MS > 0 - if (mutex->count == 1) { - uint32_t held_ms = (xTaskGetTickCount() - mutex->lock_time) * portTICK_PERIOD_MS; - if (held_ms > CONFIG_ZKERNEL_MUTEX_HOLD_WARNING_MS) { - ESP_LOGW(TAG, "Mutex held for %lu ms (threshold: %d ms)", - (unsigned long)held_ms, CONFIG_ZKERNEL_MUTEX_HOLD_WARNING_MS); - } - } -#endif -#endif - - mutex->count--; - if (mutex->count == 0) { - mutex->owner = NULL; - xSemaphoreGive(mutex->handle); - } return 0; } diff --git a/test/sdkconfig.defaults b/test/sdkconfig.defaults index 714e348..a56ff77 100644 --- a/test/sdkconfig.defaults +++ b/test/sdkconfig.defaults @@ -5,8 +5,6 @@ CONFIG_ESP_MAIN_TASK_STACK_SIZE=8192 # Enable all Boreas features for testing CONFIG_ZKERNEL_SYS_INIT=y CONFIG_ZKERNEL_SYS_INIT_MAX_ENTRIES=32 -CONFIG_ZKERNEL_MUTEX_DEBUG=y -CONFIG_ZKERNEL_MUTEX_HOLD_WARNING_MS=5000 CONFIG_ZSYS_LOG_MODULE=y CONFIG_ZSYS_LOG_MAX_LEVEL=4