From ecc0c386e69948939c2af37eb042cdbe250e5603 Mon Sep 17 00:00:00 2001 From: Jimmy Wennlund Date: Fri, 29 Nov 2024 23:29:42 +0100 Subject: [PATCH 1/2] fix(lwip): Avoid dual heap allocations of semaphores by LwIP This avoids one pointer indirection. --- components/esp_netif/lwip/esp_netif_lwip.c | 8 ++-- .../port/freertos/include/arch/sys_arch.h | 9 ++--- components/lwip/port/freertos/sys_arch.c | 38 ++++++++++--------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/components/esp_netif/lwip/esp_netif_lwip.c b/components/esp_netif/lwip/esp_netif_lwip.c index bb26777d6b1d..cbdbb3040c97 100644 --- a/components/esp_netif/lwip/esp_netif_lwip.c +++ b/components/esp_netif/lwip/esp_netif_lwip.c @@ -207,8 +207,8 @@ static void netif_unset_garp_flag(struct netif *netif) #endif // CONFIG_LWIP_GARP_TMR_INTERVAL #if !LWIP_TCPIP_CORE_LOCKING -static sys_sem_t api_sync_sem = NULL; -static sys_sem_t api_lock_sem = NULL; +static sys_sem_t api_sync_sem = {0}; +static sys_sem_t api_lock_sem = {0}; #endif /** @@ -588,14 +588,14 @@ esp_err_t esp_netif_init(void) } #if !LWIP_TCPIP_CORE_LOCKING - if (!api_sync_sem) { + if (!sys_sem_valid(&api_sync_sem)) { if (ERR_OK != sys_sem_new(&api_sync_sem, 0)) { ESP_LOGE(TAG, "esp netif api sync sem init fail"); return ESP_FAIL; } } - if (!api_lock_sem) { + if (!sys_sem_valid(&api_lock_sem)) { if (ERR_OK != sys_sem_new(&api_lock_sem, 1)) { ESP_LOGE(TAG, "esp netif api lock sem init fail"); return ESP_FAIL; diff --git a/components/lwip/port/freertos/include/arch/sys_arch.h b/components/lwip/port/freertos/include/arch/sys_arch.h index 408f9e799037..f97a69524dc0 100644 --- a/components/lwip/port/freertos/include/arch/sys_arch.h +++ b/components/lwip/port/freertos/include/arch/sys_arch.h @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: BSD-3-Clause * - * SPDX-FileContributor: 2018-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2018-2025 Espressif Systems (Shanghai) CO LTD */ #ifndef __SYS_ARCH_H__ #define __SYS_ARCH_H__ @@ -18,7 +18,7 @@ extern "C" { #endif -typedef SemaphoreHandle_t sys_sem_t; +typedef StaticSemaphore_t sys_sem_t; typedef SemaphoreHandle_t sys_mutex_t; typedef TaskHandle_t sys_thread_t; @@ -45,9 +45,8 @@ void sys_delay_ms(uint32_t ms); #define sys_mbox_valid(mbox) (*(mbox) != NULL) #define sys_mbox_set_invalid(mbox) (*(mbox) = NULL) -#define sys_sem_valid_val(sema) ((sema) != NULL) -#define sys_sem_valid(sema) (((sema) != NULL) && sys_sem_valid_val(*(sema))) -#define sys_sem_set_invalid(sema) ((*(sema)) = NULL) +#define sys_sem_valid(sema) ( ( memcmp((sema), &(StaticSemaphore_t){0}, sizeof(StaticSemaphore_t)) == 0) ? pdFALSE : pdTRUE ) +#define sys_sem_set_invalid(sema) memset((sema), 0, sizeof(StaticSemaphore_t)) void sys_delay_ms(uint32_t ms); sys_sem_t* sys_thread_sem_init(void); diff --git a/components/lwip/port/freertos/sys_arch.c b/components/lwip/port/freertos/sys_arch.c index 02c5674492c2..9fd60a93b39b 100644 --- a/components/lwip/port/freertos/sys_arch.c +++ b/components/lwip/port/freertos/sys_arch.c @@ -108,14 +108,14 @@ sys_sem_new(sys_sem_t *sem, u8_t count) LWIP_ASSERT("initial_count invalid (neither 0 nor 1)", (count == 0) || (count == 1)); - *sem = xSemaphoreCreateBinary(); - if (*sem == NULL) { - LWIP_DEBUGF(SYS_DEBUG, ("sys_sem_new: out of mem\r\n")); + QueueHandle_t res = xSemaphoreCreateBinaryStatic(sem); + if ((sys_sem_t *)res != sem) { + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_sem_new: out of mem\r\n")); return ERR_MEM; } if (count == 1) { - BaseType_t ret = xSemaphoreGive(*sem); + BaseType_t ret = xSemaphoreGive(sem); LWIP_ASSERT("sys_sem_new: initial give failed", ret == pdTRUE); (void)ret; } @@ -131,7 +131,7 @@ sys_sem_new(sys_sem_t *sem, u8_t count) void sys_sem_signal(sys_sem_t *sem) { - BaseType_t ret = xSemaphoreGive(*sem); + BaseType_t ret = xSemaphoreGive(sem); /* queue full is OK, this is a signal only... */ LWIP_ASSERT("sys_sem_signal: sane return value", (ret == pdTRUE) || (ret == errQUEUE_FULL)); @@ -144,7 +144,7 @@ int sys_sem_signal_isr(sys_sem_t *sem) { BaseType_t woken = pdFALSE; - xSemaphoreGiveFromISR(*sem, &woken); + xSemaphoreGiveFromISR(sem, &woken); return woken == pdTRUE; } @@ -162,7 +162,7 @@ sys_arch_sem_wait(sys_sem_t *sem, u32_t timeout) if (!timeout) { /* wait infinite */ - ret = xSemaphoreTake(*sem, portMAX_DELAY); + ret = xSemaphoreTake((QueueHandle_t)sem, portMAX_DELAY); LWIP_ASSERT("taking semaphore failed", ret == pdTRUE); } else { /* Round up the number of ticks. @@ -172,7 +172,7 @@ sys_arch_sem_wait(sys_sem_t *sem, u32_t timeout) * 1 tick before triggering a timeout. Thus, we need to pass 2 ticks as a timeout * to `xSemaphoreTake`. */ TickType_t timeout_ticks = ((timeout + portTICK_PERIOD_MS - 1) / portTICK_PERIOD_MS) + 1; - ret = xSemaphoreTake(*sem, timeout_ticks); + ret = xSemaphoreTake((QueueHandle_t)sem, timeout_ticks); if (ret == errQUEUE_EMPTY) { /* timed out */ return SYS_ARCH_TIMEOUT; @@ -191,8 +191,8 @@ sys_arch_sem_wait(sys_sem_t *sem, u32_t timeout) void sys_sem_free(sys_sem_t *sem) { - vSemaphoreDelete(*sem); - *sem = NULL; + vSemaphoreDelete((QueueHandle_t)sem); + memset(sem, 0, sizeof(*sem)); } /** @@ -490,13 +490,13 @@ sys_thread_sem_free(void* data) // destructor for TLS semaphore { sys_sem_t *sem = (sys_sem_t*)(data); - if (sem && *sem){ - LWIP_DEBUGF(SYS_DEBUG, ("sem del, sem=%p\n", *sem)); - vSemaphoreDelete(*sem); + if (sem){ + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem del, sem=%p\n", sem)); + vSemaphoreDelete((QueueHandle_t)sem); } if (sem) { - LWIP_DEBUGF(SYS_DEBUG, ("sem pointer del, sem_p=%p\n", sem)); + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem pointer del, sem_p=%p\n", sem)); mem_free(sem); } } @@ -504,17 +504,19 @@ sys_thread_sem_free(void* data) // destructor for TLS semaphore sys_sem_t* sys_thread_sem_init(void) { - sys_sem_t *sem = (sys_sem_t*)mem_malloc(sizeof(sys_sem_t*)); + sys_sem_t *sem = (sys_sem_t*)mem_malloc(sizeof(sys_sem_t)); if (!sem){ ESP_LOGE(TAG, "thread_sem_init: out of memory"); return 0; } - *sem = xSemaphoreCreateBinary(); - if (!(*sem)){ + // If successful, create a binary semaphore the pointer to the semaphore is returned + QueueHandle_t res = xSemaphoreCreateBinaryStatic(sem); + + if (res != (QueueHandle_t)sem){ mem_free(sem); - ESP_LOGE(TAG, "thread_sem_init: out of memory"); + ESP_LOGE(TAG, "thread_sem_init: unable to create semaphore %p != %p", res, sem); return 0; } From b4104104a9b9b7ad1acfc613d03f0573aaf692cb Mon Sep 17 00:00:00 2001 From: Jimmy Wennlund Date: Sat, 30 Nov 2024 22:59:36 +0100 Subject: [PATCH 2/2] fix(lwip): Use static queue for LwIP mbox This avoids duplicated malloc, on a congested code path. Avoids additional heap overhead, increases speed and lowers memory usage. --- .../port/freertos/include/arch/sys_arch.h | 3 +- components/lwip/port/freertos/sys_arch.c | 28 +++++++++---------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/components/lwip/port/freertos/include/arch/sys_arch.h b/components/lwip/port/freertos/include/arch/sys_arch.h index f97a69524dc0..fd892d42b3eb 100644 --- a/components/lwip/port/freertos/include/arch/sys_arch.h +++ b/components/lwip/port/freertos/include/arch/sys_arch.h @@ -23,7 +23,8 @@ typedef SemaphoreHandle_t sys_mutex_t; typedef TaskHandle_t sys_thread_t; typedef struct sys_mbox_s { - QueueHandle_t os_mbox; + StaticQueue_t os_mbox; + uint8_t buffer[0]; }* sys_mbox_t; /** This is returned by _fromisr() sys functions to tell the outermost function diff --git a/components/lwip/port/freertos/sys_arch.c b/components/lwip/port/freertos/sys_arch.c index 9fd60a93b39b..035817ec5deb 100644 --- a/components/lwip/port/freertos/sys_arch.c +++ b/components/lwip/port/freertos/sys_arch.c @@ -205,21 +205,21 @@ sys_sem_free(sys_sem_t *sem) err_t sys_mbox_new(sys_mbox_t *mbox, int size) { - *mbox = mem_malloc(sizeof(struct sys_mbox_s)); + *mbox = (sys_mbox_t)mem_malloc(sizeof(struct sys_mbox_s) + (size * sizeof(void *))); if (*mbox == NULL){ LWIP_DEBUGF(SYS_DEBUG, ("fail to new *mbox\n")); return ERR_MEM; } - (*mbox)->os_mbox = xQueueCreate(size, sizeof(void *)); + QueueHandle_t res = xQueueCreateStatic(size, sizeof(void *), (*mbox)->buffer, &(*mbox)->os_mbox); - if ((*mbox)->os_mbox == NULL) { + if ((QueueHandle_t)&(*mbox)->os_mbox != res) { LWIP_DEBUGF(SYS_DEBUG, ("fail to new (*mbox)->os_mbox\n")); - mem_free(*mbox); + free(*mbox); return ERR_MEM; } - LWIP_DEBUGF(SYS_DEBUG, ("new *mbox ok mbox=%p os_mbox=%p\n", *mbox, (*mbox)->os_mbox)); + LWIP_DEBUGF(SYS_DEBUG, ("new *mbox ok mbox=%p os_mbox=%p\n", *mbox, (QueueHandle_t)&(*mbox)->os_mbox)); return ERR_OK; } @@ -232,7 +232,7 @@ sys_mbox_new(sys_mbox_t *mbox, int size) void sys_mbox_post(sys_mbox_t *mbox, void *msg) { - BaseType_t ret = xQueueSendToBack((*mbox)->os_mbox, &msg, portMAX_DELAY); + BaseType_t ret = xQueueSendToBack((QueueHandle_t)&(*mbox)->os_mbox, &msg, portMAX_DELAY); LWIP_ASSERT("mbox post failed", ret == pdTRUE); (void)ret; } @@ -249,10 +249,10 @@ sys_mbox_trypost(sys_mbox_t *mbox, void *msg) { err_t xReturn; - if (xQueueSend((*mbox)->os_mbox, &msg, 0) == pdTRUE) { + if (xQueueSend((QueueHandle_t)&(*mbox)->os_mbox, &msg, 0) == pdTRUE) { xReturn = ERR_OK; } else { - LWIP_DEBUGF(SYS_DEBUG, ("trypost mbox=%p fail\n", (*mbox)->os_mbox)); + LWIP_DEBUGF(SYS_DEBUG, ("trypost mbox=%p fail\n", (QueueHandle_t)&(*mbox)->os_mbox)); xReturn = ERR_MEM; } @@ -274,7 +274,7 @@ sys_mbox_trypost_fromisr(sys_mbox_t *mbox, void *msg) BaseType_t ret; BaseType_t xHigherPriorityTaskWoken = pdFALSE; - ret = xQueueSendFromISR((*mbox)->os_mbox, &msg, &xHigherPriorityTaskWoken); + ret = xQueueSendFromISR((QueueHandle_t)&(*mbox)->os_mbox, &msg, &xHigherPriorityTaskWoken); if (ret == pdTRUE) { if (xHigherPriorityTaskWoken == pdTRUE) { return ERR_NEED_SCHED; @@ -306,11 +306,11 @@ sys_arch_mbox_fetch(sys_mbox_t *mbox, void **msg, u32_t timeout) if (timeout == 0) { /* wait infinite */ - ret = xQueueReceive((*mbox)->os_mbox, &(*msg), portMAX_DELAY); + ret = xQueueReceive((QueueHandle_t)&(*mbox)->os_mbox, &(*msg), portMAX_DELAY); LWIP_ASSERT("mbox fetch failed", ret == pdTRUE); } else { TickType_t timeout_ticks = timeout / portTICK_PERIOD_MS; - ret = xQueueReceive((*mbox)->os_mbox, &(*msg), timeout_ticks); + ret = xQueueReceive((QueueHandle_t)&(*mbox)->os_mbox, &(*msg), timeout_ticks); if (ret == errQUEUE_EMPTY) { /* timed out */ *msg = NULL; @@ -338,7 +338,7 @@ sys_arch_mbox_tryfetch(sys_mbox_t *mbox, void **msg) if (msg == NULL) { msg = &msg_dummy; } - ret = xQueueReceive((*mbox)->os_mbox, &(*msg), 0); + ret = xQueueReceive((QueueHandle_t)&(*mbox)->os_mbox, &(*msg), 0); if (ret == errQUEUE_EMPTY) { *msg = NULL; return SYS_MBOX_EMPTY; @@ -359,10 +359,10 @@ sys_mbox_free(sys_mbox_t *mbox) if ((NULL == mbox) || (NULL == *mbox)) { return; } - UBaseType_t msgs_waiting = uxQueueMessagesWaiting((*mbox)->os_mbox); + UBaseType_t msgs_waiting = uxQueueMessagesWaiting((QueueHandle_t)&(*mbox)->os_mbox); LWIP_ASSERT("mbox quence not empty", msgs_waiting == 0); - vQueueDelete((*mbox)->os_mbox); + vQueueDelete((QueueHandle_t)&(*mbox)->os_mbox); mem_free(*mbox); *mbox = NULL;