diff --git a/components/lwip/api/api_lib.c b/components/lwip/api/api_lib.c index e93a2a96a..62197f485 100755 --- a/components/lwip/api/api_lib.c +++ b/components/lwip/api/api_lib.c @@ -128,6 +128,9 @@ netconn_new_with_proto_and_callback(enum netconn_type t, u8_t proto, netconn_cal LWIP_ASSERT("conn has no op_completed", sys_sem_valid(&conn->op_completed)); sys_sem_free(&conn->op_completed); #endif /* !LWIP_NETCONN_SEM_PER_THREAD */ +#if ESP_THREAD_SAFE + sys_mbox_set_owner(&conn->recvmbox, NULL); +#endif sys_mbox_free(&conn->recvmbox); memp_free(MEMP_NETCONN, conn); return NULL; diff --git a/components/lwip/api/api_msg.c b/components/lwip/api/api_msg.c index c9970740b..ae31e92ed 100755 --- a/components/lwip/api/api_msg.c +++ b/components/lwip/api/api_msg.c @@ -552,6 +552,9 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err) tcp_err(pcb, NULL); /* remove reference from to the pcb from this netconn */ newconn->pcb.tcp = NULL; +#if ESP_THREAD_SAFE + sys_mbox_set_owner(&newconn->recvmbox, NULL); +#endif /* no need to drain since we know the recvmbox is empty. */ sys_mbox_free(&newconn->recvmbox); sys_mbox_set_invalid(&newconn->recvmbox); @@ -711,14 +714,14 @@ netconn_alloc(enum netconn_type t, netconn_callback callback) #endif #if ESP_THREAD_SAFE - conn->recvmbox_ref = conn->recvmbox; sys_mbox_set_owner(&conn->recvmbox, conn); -#if LWIP_TCP - sys_mbox_set_invalid(&conn->acceptmbox_ref); -#endif #endif #if LWIP_TCP +#if ESP_THREAD_SAFE + /* Init acceptmbox to NULL because sys_mbox_set_invalid is implemented as empty macro */ + conn->acceptmbox = NULL; +#endif sys_mbox_set_invalid(&conn->acceptmbox); #endif conn->state = NETCONN_NONE; @@ -761,24 +764,21 @@ void netconn_free(struct netconn *conn) { LWIP_ASSERT("PCB must be deallocated outside this function", conn->pcb.tcp == NULL); + +#if !ESP_THREAD_SAFE LWIP_ASSERT("recvmbox must be deallocated before calling this function", !sys_mbox_valid(&conn->recvmbox)); #if LWIP_TCP LWIP_ASSERT("acceptmbox must be deallocated before calling this function", !sys_mbox_valid(&conn->acceptmbox)); #endif /* LWIP_TCP */ - -#if ESP_THREAD_SAFE - if (conn->recvmbox_ref) { - sys_mbox_free(&conn->recvmbox_ref); - } +#else /* !ESP_THREAD_SAFE */ + sys_mbox_free(&conn->recvmbox); #if LWIP_TCP - if (conn->acceptmbox_ref) { - sys_mbox_free(&conn->acceptmbox_ref); - } -#endif + sys_mbox_free(&conn->acceptmbox); #endif +#endif /* !ESP_THREAD_SAFE */ #if !LWIP_NETCONN_SEM_PER_THREAD sys_sem_free(&conn->op_completed); @@ -1420,7 +1420,6 @@ lwip_netconn_do_listen(void *m) } if (msg->err == ERR_OK) { #if ESP_THREAD_SAFE - msg->conn->acceptmbox_ref = msg->conn->acceptmbox; sys_mbox_set_owner(&msg->conn->acceptmbox, msg->conn); #endif msg->conn->state = NETCONN_LISTEN; diff --git a/components/lwip/include/lwip/lwip/api.h b/components/lwip/include/lwip/lwip/api.h index fa9709122..5b6a21ecf 100755 --- a/components/lwip/include/lwip/lwip/api.h +++ b/components/lwip/include/lwip/lwip/api.h @@ -195,15 +195,6 @@ struct netconn { by the application thread */ sys_mbox_t acceptmbox; #endif /* LWIP_TCP */ - -#if ESP_THREAD_SAFE - /** point to the same mbox as recvmbox */ - sys_mbox_t recvmbox_ref; -#if LWIP_TCP - /** point to the same mbox as acceptmbox */ - sys_mbox_t acceptmbox_ref; -#endif -#endif /** only used for socket layer */ #if LWIP_SOCKET int socket; diff --git a/components/lwip/include/lwip/port/arch/sys_arch.h b/components/lwip/include/lwip/port/arch/sys_arch.h index f3a0625b8..3a3932734 100644 --- a/components/lwip/include/lwip/port/arch/sys_arch.h +++ b/components/lwip/include/lwip/port/arch/sys_arch.h @@ -62,7 +62,25 @@ typedef struct sys_mbox_s { #endif #define sys_mbox_valid( x ) ( ( ( *x ) == NULL) ? pdFALSE : pdTRUE ) -#define sys_mbox_set_invalid( x ) ( ( *x ) = NULL ) + +/* Define the sys_mbox_set_invalid() to empty to support lock-free mbox in ESP LWIP. + * + * The basic idea about the lock-free mbox is that the mbox should always be valid unless + * no socket APIs are using the socket and the socket is closed. ESP LWIP achieves this by + * following two changes to official LWIP: + * 1. Postpone the deallocation of mbox to netconn_free(), in other words, free the mbox when + * no one is using the socket. + * 2. Define the sys_mbox_set_invalid() to empty if the mbox is not actually freed. + + * The second change is necessary. Consider a common scenario: the application task calls + * recv() to receive packets from the socket, the sys_mbox_valid() returns true. Because there + * is no lock for the mbox, the LWIP CORE can call sys_mbox_set_invalid() to set the mbox at + * anytime and the thread-safe issue may happen. + * + * However, if the sys_mbox_set_invalid() is not called after sys_mbox_free(), e.g. in netconn_alloc(), + * we need to initialize the mbox to invalid explicitly since sys_mbox_set_invalid() now is empty. + */ +#define sys_mbox_set_invalid( x ) #define sys_sem_valid( x ) ( ( ( *x ) == NULL) ? pdFALSE : pdTRUE ) #define sys_sem_set_invalid( x ) ( ( *x ) = NULL ) diff --git a/components/lwip/port/freertos/sys_arch.c b/components/lwip/port/freertos/sys_arch.c index 34cb98ac2..e778374ff 100755 --- a/components/lwip/port/freertos/sys_arch.c +++ b/components/lwip/port/freertos/sys_arch.c @@ -343,7 +343,6 @@ sys_mbox_free(sys_mbox_t *mbox) uint32_t mbox_message_num = 0; if ( (NULL == mbox) || (NULL == *mbox) ) { - ESP_LOGW(TAG, "WARNING: free null mbox\n"); return; } @@ -381,12 +380,13 @@ sys_mbox_free(sys_mbox_t *mbox) /* For recvmbox or acceptmbox, free them in netconn_free() when all sockets' API are returned */ vQueueDelete((*mbox)->os_mbox); free(*mbox); + *mbox = NULL; } #else vQueueDelete((*mbox)->os_mbox); free(*mbox); -#endif *mbox = NULL; +#endif } /*-----------------------------------------------------------------------------------*/