From b75c784a202451c3b1d96c17ac86a1a7d2dab97c Mon Sep 17 00:00:00 2001 From: Jon Shallow Date: Fri, 19 Jul 2024 12:43:30 +0100 Subject: [PATCH] Constrained Stack: No need for individual locks If a proxy reads in a packet via coap_read_endpoint, which then invokes an ongoing proxy TCP connection, while waiting for the CSM response of the new session, coap_read_endpoint would get invoked again if there is any data to read on any of the listening endpoints.. With a Constrained Stack, the read in data is protected by mutex m_read_enpoint - which is still locked when trying to wait for the CSM. As the code is either single threaded, or protected by global_lock if multi-threaded, there is no need to maintain mutexes such as m_read_endpoint. This PR removes these not needed mutexes. --- include/coap3/coap_mutex_internal.h | 11 ----- src/coap_debug.c | 19 +-------- src/coap_mbedtls.c | 12 +----- src/coap_net.c | 64 +---------------------------- src/coap_subscribe.c | 12 +----- 5 files changed, 6 insertions(+), 112 deletions(-) diff --git a/include/coap3/coap_mutex_internal.h b/include/coap3/coap_mutex_internal.h index ec91f6c1b8..665fe49977 100644 --- a/include/coap3/coap_mutex_internal.h +++ b/include/coap3/coap_mutex_internal.h @@ -150,15 +150,4 @@ typedef int coap_mutex_t; #endif /* !WITH_CONTIKI && !WITH_LWIP && !RIOT_VERSION && !HAVE_PTHREAD_H && !HAVE_PTHREAD_MUTEX_LOCK */ -#if COAP_CONSTRAINED_STACK - -extern coap_mutex_t m_show_pdu; -extern coap_mutex_t m_log_impl; -extern coap_mutex_t m_dtls_recv; -extern coap_mutex_t m_read_session; -extern coap_mutex_t m_read_endpoint; -extern coap_mutex_t m_persist_add; - -#endif /* COAP_CONSTRAINED_STACK */ - #endif /* COAP_MUTEX_INTERNAL_H_ */ diff --git a/src/coap_debug.c b/src/coap_debug.c index ead4af98b7..0e3d93f797 100644 --- a/src/coap_debug.c +++ b/src/coap_debug.c @@ -778,7 +778,7 @@ void coap_show_pdu(coap_log_t level, const coap_pdu_t *pdu) { #if COAP_CONSTRAINED_STACK /* Proxy-Uri: can be 1034 bytes long */ - /* buf and outbuf protected by mutex m_show_pdu */ + /* buf and outbuf can be protected by global_lock if needed */ static unsigned char buf[min(COAP_DEBUG_BUF_SIZE, 1035)]; static char outbuf[COAP_DEBUG_BUF_SIZE]; #else /* ! COAP_CONSTRAINED_STACK */ @@ -803,10 +803,6 @@ coap_show_pdu(coap_log_t level, const coap_pdu_t *pdu) { if (level > coap_get_log_level()) return; -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_show_pdu); -#endif /* COAP_CONSTRAINED_STACK */ - if (!pdu->session || COAP_PROTO_NOT_RELIABLE(pdu->session->proto)) { snprintf(outbuf, sizeof(outbuf), "v:%d t:%s c:%s i:%04x {", COAP_DEFAULT_VERSION, msg_type_string(pdu->type), @@ -1118,10 +1114,6 @@ coap_show_pdu(coap_log_t level, const coap_pdu_t *pdu) { outbuflen--; snprintf(&outbuf[outbuflen], sizeof(outbuf)-outbuflen, "\n"); COAP_DO_SHOW_OUTPUT_LINE; - -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_show_pdu); -#endif /* COAP_CONSTRAINED_STACK */ } void @@ -1273,7 +1265,7 @@ coap_log_impl(coap_log_t level, const char *format, ...) { if (log_handler) { #if COAP_CONSTRAINED_STACK - /* message protected by mutex m_log_impl */ + /* message can be protected by global_lock if needed */ static char message[COAP_DEBUG_BUF_SIZE]; #else /* ! COAP_CONSTRAINED_STACK */ char message[COAP_DEBUG_BUF_SIZE]; @@ -1281,10 +1273,6 @@ coap_log_impl(coap_log_t level, const char *format, ...) { va_list ap; va_start(ap, format); -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_log_impl); -#endif /* COAP_CONSTRAINED_STACK */ - #ifdef RIOT_VERSION flash_vsnprintf(message, sizeof(message), format, ap); #else /* !RIOT_VERSION */ @@ -1292,9 +1280,6 @@ coap_log_impl(coap_log_t level, const char *format, ...) { #endif /* !RIOT_VERSION */ va_end(ap); log_handler(level, message); -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_log_impl); -#endif /* COAP_CONSTRAINED_STACK */ } else { char timebuf[32]; coap_tick_t now; diff --git a/src/coap_mbedtls.c b/src/coap_mbedtls.c index dd7a510f3d..977e82687b 100644 --- a/src/coap_mbedtls.c +++ b/src/coap_mbedtls.c @@ -2299,16 +2299,12 @@ coap_dtls_receive(coap_session_t *c_session, if (m_env->established) { #if COAP_CONSTRAINED_STACK - /* pdu protected by mutex m_dtls_recv */ + /* pdu can be protected by global_lock if needed */ static uint8_t pdu[COAP_RXBUFFER_SIZE]; #else /* ! COAP_CONSTRAINED_STACK */ uint8_t pdu[COAP_RXBUFFER_SIZE]; #endif /* ! COAP_CONSTRAINED_STACK */ -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_dtls_recv); -#endif /* COAP_CONSTRAINED_STACK */ - if (c_session->state == COAP_SESSION_STATE_HANDSHAKE) { coap_handle_event_lkd(c_session->context, COAP_EVENT_DTLS_CONNECTED, c_session); @@ -2318,9 +2314,6 @@ coap_dtls_receive(coap_session_t *c_session, ret = mbedtls_ssl_read(&m_env->ssl, pdu, sizeof(pdu)); if (ret > 0) { ret = coap_handle_dgram(c_session->context, c_session, pdu, (size_t)ret); -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_dtls_recv); -#endif /* COAP_CONSTRAINED_STACK */ goto finish; } switch (ret) { @@ -2337,9 +2330,6 @@ coap_dtls_receive(coap_session_t *c_session, -ret, get_error_string(ret), data_len); break; } -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_dtls_recv); -#endif /* COAP_CONSTRAINED_STACK */ ret = -1; } else { ret = do_mbedtls_handshake(c_session, m_env); diff --git a/src/coap_net.c b/src/coap_net.c index 9d3df82446..9c65c6ef92 100644 --- a/src/coap_net.c +++ b/src/coap_net.c @@ -2002,7 +2002,7 @@ coap_write_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now void coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) { #if COAP_CONSTRAINED_STACK - /* payload and packet protected by mutex m_read_session */ + /* payload and packet can be protected by global_lock if needed */ static unsigned char payload[COAP_RXBUFFER_SIZE]; static coap_packet_t s_packet; #else /* ! COAP_CONSTRAINED_STACK */ @@ -2011,10 +2011,6 @@ coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) #endif /* ! COAP_CONSTRAINED_STACK */ coap_packet_t *packet = &s_packet; -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ - assert(session->sock.flags & (COAP_SOCKET_CONNECTED | COAP_SOCKET_MULTICAST)); packet->length = sizeof(payload); @@ -2052,9 +2048,6 @@ coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) /* Need max space incase PDU is updated with updated token etc. */ pdu = coap_pdu_init(0, 0, 0, coap_session_max_pdu_rcv_size(session)); if (!pdu) { -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ return; } @@ -2062,15 +2055,9 @@ coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) coap_handle_event_lkd(session->context, COAP_EVENT_BAD_PACKET, session); coap_log_warn("discard malformed PDU\n"); coap_delete_pdu(pdu); -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ return; } -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ coap_dispatch(ctx, session, pdu); coap_delete_pdu(pdu); return; @@ -2102,13 +2089,7 @@ coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) if (n == len) { if (coap_pdu_parse_header(session->partial_pdu, session->proto) && coap_pdu_parse_opt(session->partial_pdu)) { -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ coap_dispatch(ctx, session, session->partial_pdu); -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ } coap_delete_pdu(session->partial_pdu); session->partial_pdu = NULL; @@ -2154,13 +2135,7 @@ coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) session->partial_read = hdr_size + tok_ext_bytes; if (size == 0) { if (coap_pdu_parse_header(session->partial_pdu, session->proto)) { -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ coap_dispatch(ctx, session, session->partial_pdu); -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ } coap_delete_pdu(session->partial_pdu); session->partial_pdu = NULL; @@ -2185,9 +2160,6 @@ coap_read_session(coap_context_t *ctx, coap_session_t *session, coap_tick_t now) coap_session_disconnected_lkd(session, COAP_NACK_NOT_DELIVERABLE); #endif /* !COAP_DISABLE_TCP */ } -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_session); -#endif /* COAP_CONSTRAINED_STACK */ } #if COAP_SERVER_SUPPORT @@ -2196,7 +2168,7 @@ coap_read_endpoint(coap_context_t *ctx, coap_endpoint_t *endpoint, coap_tick_t n ssize_t bytes_read = -1; int result = -1; /* the value to be returned */ #if COAP_CONSTRAINED_STACK - /* payload and e_packet protected by mutex m_read_endpoint */ + /* payload and e_packet can be protected by global_lock if needed */ static unsigned char payload[COAP_RXBUFFER_SIZE]; static coap_packet_t e_packet; #else /* ! COAP_CONSTRAINED_STACK */ @@ -2208,10 +2180,6 @@ coap_read_endpoint(coap_context_t *ctx, coap_endpoint_t *endpoint, coap_tick_t n assert(COAP_PROTO_NOT_RELIABLE(endpoint->proto)); assert(endpoint->sock.flags & COAP_SOCKET_BOUND); -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_read_endpoint); -#endif /* COAP_CONSTRAINED_STACK */ - /* Need to do this as there may be holes in addr_info */ memset(&packet->addr_info, 0, sizeof(packet->addr_info)); packet->length = sizeof(payload); @@ -2234,9 +2202,6 @@ coap_read_endpoint(coap_context_t *ctx, coap_endpoint_t *endpoint, coap_tick_t n coap_session_new_dtls_session(session, now); } } -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_read_endpoint); -#endif /* COAP_CONSTRAINED_STACK */ return result; } @@ -4392,14 +4357,6 @@ coap_check_async(coap_context_t *context, coap_tick_t now) { int coap_started = 0; -#if COAP_CONSTRAINED_STACK -coap_mutex_t m_show_pdu; -coap_mutex_t m_log_impl; -coap_mutex_t m_dtls_recv; -coap_mutex_t m_read_session; -coap_mutex_t m_read_endpoint; -coap_mutex_t m_persist_add; -#endif /* COAP_CONSTRAINED_STACK */ #if COAP_THREAD_SAFE /* * Global lock for multi-thread support @@ -4418,15 +4375,6 @@ coap_startup(void) { return; coap_started = 1; -#if COAP_CONSTRAINED_STACK - coap_mutex_init(&m_show_pdu); - coap_mutex_init(&m_log_impl); - coap_mutex_init(&m_dtls_recv); - coap_mutex_init(&m_read_session); - coap_mutex_init(&m_read_endpoint); - coap_mutex_init(&m_persist_add); -#endif /* COAP_CONSTRAINED_STACK */ - #if COAP_THREAD_SAFE coap_lock_init(); #endif /* COAP_THREAD_SAFE */ @@ -4476,14 +4424,6 @@ coap_cleanup(void) { #endif /* WITH_LWIP */ coap_dtls_shutdown(); -#if COAP_CONSTRAINED_STACK - coap_mutex_destroy(&m_show_pdu); - coap_mutex_destroy(&m_log_impl); - coap_mutex_destroy(&m_dtls_recv); - coap_mutex_destroy(&m_read_session); - coap_mutex_destroy(&m_read_endpoint); - coap_mutex_destroy(&m_persist_add); -#endif /* COAP_CONSTRAINED_STACK */ coap_debug_reset(); } diff --git a/src/coap_subscribe.c b/src/coap_subscribe.c index b4e6e2d004..2f73d26d31 100644 --- a/src/coap_subscribe.c +++ b/src/coap_subscribe.c @@ -77,7 +77,7 @@ coap_persist_observe_add_lkd(coap_context_t *context, size_t data_len; coap_pdu_t *pdu = NULL; #if COAP_CONSTRAINED_STACK - /* e_packet protected by mutex m_persist_add */ + /* e_packet can be protected by global_lock if needed */ static coap_packet_t e_packet; #else /* ! COAP_CONSTRAINED_STACK */ coap_packet_t e_packet; @@ -110,10 +110,6 @@ coap_persist_observe_add_lkd(coap_context_t *context, if (!ep) return NULL; -#if COAP_CONSTRAINED_STACK - coap_mutex_lock(&m_persist_add); -#endif /* COAP_CONSTRAINED_STACK */ - /* Build up packet */ memcpy(&packet->addr_info, s_addr_info, sizeof(packet->addr_info)); packet->ifindex = 0; @@ -301,17 +297,11 @@ coap_persist_observe_add_lkd(coap_context_t *context, (void)oscore_info; #endif /* ! COAP_OSCORE_SUPPORT */ coap_delete_pdu(pdu); -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_persist_add); -#endif /* COAP_CONSTRAINED_STACK */ return s; malformed: coap_log_warn("coap_persist_observe_add: discard malformed PDU\n"); fail: -#if COAP_CONSTRAINED_STACK - coap_mutex_unlock(&m_persist_add); -#endif /* COAP_CONSTRAINED_STACK */ coap_delete_string(uri_path); coap_delete_pdu(pdu); return NULL;