diff --git a/examples/cord_ep/Makefile.ci b/examples/cord_ep/Makefile.ci index 580ff81b4c..e1a8fa544a 100644 --- a/examples/cord_ep/Makefile.ci +++ b/examples/cord_ep/Makefile.ci @@ -27,6 +27,8 @@ BOARD_INSUFFICIENT_MEMORY := \ nucleo-l031k6 \ nucleo-l053r8 \ samd10-xmini \ + saml10-xpro \ + saml11-xpro \ slstk3400a \ stk3200 \ stm32f030f4-demo \ diff --git a/sys/include/net/gnrc/netreg.h b/sys/include/net/gnrc/netreg.h index 321c0a7ef4..a733428928 100644 --- a/sys/include/net/gnrc/netreg.h +++ b/sys/include/net/gnrc/netreg.h @@ -212,6 +212,44 @@ typedef struct gnrc_netreg_entry { } target; /**< Target for the registry entry */ } gnrc_netreg_entry_t; +/** + * @brief The global locking of netregs + * + * A shared lock must be held across calls to @ref gnrc_netreg_lookup, and + * references obtained through that may only be used for the duration for which + * the lock is held. + * + * The shared lock is counting (i.e. multiple sections of code can acquire it + * concurrently), and may be held across blocking operations, as long as they + * don't involve the exclusive lock (e.g. by blocking on a socket creation). + * It's generally good practice to release them as soon as possible. + * + * There is an exclusive counterpart to the lock, which is + * internal to netreg (and used through functions such as @ref + * gnrc_netreg_register and @ref gnrc_netreg_unregister). The current + * implementation priorizes shared locks. This means that shared locks are + * generally acquired fast (they only block if an exclusive operation has + * already started), but constant access through shared locks might starve + * registration and deregistration. + * + * @{ + */ + +/** @brief Acquire a shared lock on the GNRC netreg + * + * This needs to be held around all calls to @ref gnrc_netreg_lookup, and for + * as long as any of its results are used. After that, call @ref + * gnrc_netreg_release_shared. + */ +void gnrc_netreg_acquire_shared(void); +/** @brief Release a shared lock on the GNRC netreg + * + * @pre @ref gnrc_netreg_acquire_shared was called + */ +void gnrc_netreg_release_shared(void); + +/** @} */ + /** * @brief Initializes module. */ @@ -326,6 +364,10 @@ void gnrc_netreg_unregister(gnrc_nettype_t type, gnrc_netreg_entry_t *entry); * @brief Searches for entries with given parameters in the registry and * returns the first found. * + * @pre The caller must hold the lock of @ref gnrc_netreg_acquire_shared from + * before calling this function, and must stop using any obtained pointers + * before releasing the lock through @ref gnrc_netreg_release_shared. + * * @param[in] type Type of the protocol. * @param[in] demux_ctx The demultiplexing context for the registered thread. * See gnrc_netreg_entry_t::demux_ctx. @@ -345,6 +387,13 @@ gnrc_netreg_entry_t *gnrc_netreg_lookup(gnrc_nettype_t type, uint32_t demux_ctx) * * @return Number of entries with the same gnrc_netreg_entry_t::type and * gnrc_netreg_entry_t::demux_ctx as the given parameters. + * + * Note that this returns a snapshot value, which may change at any time after + * that call. This is fine for most applications, as they just shortcut a code + * path if the number is zero. Callers that need that number to stay constant + * can acquire a shared lock through @ref gnrc_netreg_acquire_shared, and rely + * on the number staying constant until that lock is released through @ref + * gnrc_netreg_release_shared. */ int gnrc_netreg_num(gnrc_nettype_t type, uint32_t demux_ctx); @@ -353,6 +402,11 @@ int gnrc_netreg_num(gnrc_nettype_t type, uint32_t demux_ctx); * gnrc_netreg_entry_t::type and gnrc_netreg_entry_t::demux_ctx as the * given entry. * + * The requirement on holding the global lock through @ref + * gnrc_netreg_acquire_shared from @ref gnrc_netreg_lookup extends to any + * results of this function: It may only be released when none of the pointers + * are used any more. + * * @param[in] entry A registry entry retrieved by gnrc_netreg_lookup() or * gnrc_netreg_getnext(). Must not be NULL. * diff --git a/sys/net/gnrc/netapi/gnrc_netapi.c b/sys/net/gnrc/netapi/gnrc_netapi.c index 9659168602..266d0839f3 100644 --- a/sys/net/gnrc/netapi/gnrc_netapi.c +++ b/sys/net/gnrc/netapi/gnrc_netapi.c @@ -85,6 +85,8 @@ static inline int _snd_rcv_mbox(mbox_t *mbox, uint16_t type, gnrc_pktsnip_t *pkt int gnrc_netapi_dispatch(gnrc_nettype_t type, uint32_t demux_ctx, uint16_t cmd, gnrc_pktsnip_t *pkt) { + gnrc_netreg_acquire_shared(); + int numof = gnrc_netreg_num(type, demux_ctx); if (numof != 0) { @@ -134,5 +136,7 @@ int gnrc_netapi_dispatch(gnrc_nettype_t type, uint32_t demux_ctx, } } + gnrc_netreg_release_shared(); + return numof; } diff --git a/sys/net/gnrc/netreg/gnrc_netreg.c b/sys/net/gnrc/netreg/gnrc_netreg.c index de1485834b..556b6bc49a 100644 --- a/sys/net/gnrc/netreg/gnrc_netreg.c +++ b/sys/net/gnrc/netreg/gnrc_netreg.c @@ -14,6 +14,7 @@ #include #include +#include #include "assert.h" #include "log.h" @@ -33,12 +34,101 @@ /* The registry as lookup table by gnrc_nettype_t */ static gnrc_netreg_entry_t *netreg[GNRC_NETTYPE_NUMOF]; +/** Held while accessing _lock_counter, and also while the exclusive lock is held */ +static mutex_t _lock_for_counter = MUTEX_INIT; +/** Number of shared locks on netreg. Saturating arithmetic is used; if this + * reaches UINT_MAX, the lock will never be freed again. This is likely + * accurate, given that it will only happen when the lock was leaked, so it + * can't be freed any more anyway. + * + * This can be accessed only when _lock_for_counter is held; an alternative + * implementation with atomics would likely be possible, but for the case of "I + * can't do that right now" it would still need a mutex. But that's + * optimization that could be done without changing the public API. + * */ +static unsigned int _lock_counter = 0; +/** Held while the netreg lists are being read. This is what the exclusive + * users block on if they can't grab the exclusive lock right away. + * + * This is largely used as a boolean flag (is locked / is unlocked) that is + * modified while the _lock_for_counter is held, and will not block because it + * is synchronized to _lock_counter being 0. It is only ever accessed outside + * _lock_for_counter when waiting for the counter to reach 0, and even then is + * released immediately to avoid deadlocks. (Instead, the exclusive acquisition + * tries to acquire the _lock_for_counter, and if that fails it queues up again + * after the reader that just snatched it). + * */ +static mutex_t _lock_wait_exclusive = MUTEX_INIT; + void gnrc_netreg_init(void) { /* set all pointers in registry to NULL */ memset(netreg, 0, GNRC_NETTYPE_NUMOF * sizeof(gnrc_netreg_entry_t *)); } +void gnrc_netreg_acquire_shared(void) { + mutex_lock(&_lock_for_counter); + if (_lock_counter == 0) { + /* At most, this blocks for the very short time until + * _gnrc_netreg_acquire_exclusive returns it immediately */ + mutex_lock(&_lock_wait_exclusive); + } + if (_lock_counter != UINT_MAX) { + _lock_counter += 1; + } + mutex_unlock(&_lock_for_counter); +} + +void gnrc_netreg_release_shared(void) { + mutex_lock(&_lock_for_counter); + + assert(_lock_counter != 0); /* Release without acquire */ + + if (_lock_counter != UINT_MAX) { + _lock_counter -= 1; + } + if (_lock_counter == 0) { + mutex_unlock(&_lock_wait_exclusive); + } + mutex_unlock(&_lock_for_counter); +} + +/** Assert that there is a shared lock on gnrc_netreg -- this should help weed + * out callers to @ref gnrc_netreg_lookup that don't properly lock. */ +static void _gnrc_netreg_assert_shared(void) { +#if DEVELHELP + /* Even if we just peek: It's not an atomic, so it needs synchronization */ + mutex_lock(&_lock_for_counter); + assert(_lock_counter != 0); + mutex_unlock(&_lock_for_counter); +#endif +} + +static void _gnrc_netreg_acquire_exclusive(void) { + while (true) { + mutex_lock(&_lock_for_counter); + if (_lock_counter == 0) { + /* At most, this blocks for the very short time until + * another caller of _gnrc_netreg_acquire_exclusive returns it + * immediately */ + mutex_lock(&_lock_wait_exclusive); + /* Leaving both locked */ + return; + } + mutex_unlock(&_lock_for_counter); + + mutex_lock(&_lock_wait_exclusive); + /* ... but maybe someone just started grabbing _lock_for_counter, so we + * give them a chance to finish rather than deadlocking them */ + mutex_unlock(&_lock_wait_exclusive); + } +} + +static void _gnrc_netreg_release_exclusive(void) { + mutex_unlock(&_lock_wait_exclusive); + mutex_unlock(&_lock_for_counter); +} + int gnrc_netreg_register(gnrc_nettype_t type, gnrc_netreg_entry_t *entry) { #if DEVELHELP @@ -61,7 +151,9 @@ int gnrc_netreg_register(gnrc_nettype_t type, gnrc_netreg_entry_t *entry) return -EINVAL; } + _gnrc_netreg_acquire_exclusive(); LL_PREPEND(netreg[type], entry); + _gnrc_netreg_release_exclusive(); return 0; } @@ -72,7 +164,13 @@ void gnrc_netreg_unregister(gnrc_nettype_t type, gnrc_netreg_entry_t *entry) return; } + _gnrc_netreg_acquire_exclusive(); LL_DELETE(netreg[type], entry); + /* We can release now already: No new references to this entry can be made + * any more, and the caller is only allowed to reuse the entry and the mbox + * target referenced by it after *this* function returned, not when the + * lock becomes available again. */ + _gnrc_netreg_release_exclusive(); #if defined(MODULE_GNRC_NETAPI_MBOX) /* drain packets still in the mbox */ @@ -101,6 +199,8 @@ static gnrc_netreg_entry_t *_netreg_lookup(gnrc_netreg_entry_t *from, gnrc_nettype_t type, uint32_t demux_ctx) { + _gnrc_netreg_assert_shared(); + gnrc_netreg_entry_t *res = NULL; if (from || !_INVALID_TYPE(type)) { @@ -121,9 +221,14 @@ int gnrc_netreg_num(gnrc_nettype_t type, uint32_t demux_ctx) int num = 0; gnrc_netreg_entry_t *entry = NULL; + gnrc_netreg_acquire_shared(); + while((entry = _netreg_lookup(entry, type, demux_ctx)) != NULL) { num++; } + + gnrc_netreg_release_shared(); + return num; } diff --git a/tests/gcoap_fileserver/Makefile.ci b/tests/gcoap_fileserver/Makefile.ci index a2a48fffd2..95ec690c99 100644 --- a/tests/gcoap_fileserver/Makefile.ci +++ b/tests/gcoap_fileserver/Makefile.ci @@ -8,6 +8,7 @@ BOARD_INSUFFICIENT_MEMORY := \ atmega328p \ atmega328p-xplained-mini \ atxmega-a1-xplained \ + atxmega-a1u-xpro \ atxmega-a3bu-xplained \ blackpill \ bluepill \ diff --git a/tests/unittests/tests-netreg/tests-netreg.c b/tests/unittests/tests-netreg/tests-netreg.c index b56a68861f..04b6cf27f6 100644 --- a/tests/unittests/tests-netreg/tests-netreg.c +++ b/tests/unittests/tests-netreg/tests-netreg.c @@ -38,23 +38,34 @@ static void test_netreg_register__inval_numof(void) static void test_netreg_register__success(void) { + gnrc_netreg_acquire_shared(); gnrc_netreg_entry_t *res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16); TEST_ASSERT_NULL(res); + gnrc_netreg_release_shared(); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + gnrc_netreg_acquire_shared(); TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16))); TEST_ASSERT_EQUAL_INT(TEST_UINT16, res->demux_ctx); TEST_ASSERT_EQUAL_INT(TEST_UINT8, res->target.pid); TEST_ASSERT_NULL((gnrc_netreg_getnext(res))); + gnrc_netreg_release_shared(); } void test_netreg_unregister__success(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_NOT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)); + gnrc_netreg_release_shared(); + gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[0]); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_unregister__success2(void) @@ -64,11 +75,18 @@ void test_netreg_unregister__success2(void) TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[1])); gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[0]); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16))); TEST_ASSERT_EQUAL_INT(TEST_UINT16, res->demux_ctx); TEST_ASSERT_EQUAL_INT(TEST_UINT8 + 1, res->target.pid); + gnrc_netreg_release_shared(); + gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[1]); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_unregister__success3(void) @@ -78,56 +96,82 @@ void test_netreg_unregister__success3(void) TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[1])); gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[1]); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16))); TEST_ASSERT_EQUAL_INT(TEST_UINT16, res->demux_ctx); TEST_ASSERT_EQUAL_INT(TEST_UINT8, res->target.pid); + gnrc_netreg_release_shared(); + gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[0]); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_lookup__wrong_type_undef(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + gnrc_netreg_acquire_shared(); TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_UNDEF, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_lookup__wrong_type_numof(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + gnrc_netreg_acquire_shared(); TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_NUMOF, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_num__empty(void) { + gnrc_netreg_acquire_shared(); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16)); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16 + 1)); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_TEST, GNRC_NETREG_DEMUX_CTX_ALL)); + gnrc_netreg_release_shared(); } void test_netreg_num__wrong_type_undef(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + gnrc_netreg_acquire_shared(); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_UNDEF, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_num__wrong_type_numof(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + gnrc_netreg_acquire_shared(); TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_NUMOF, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_num__2_entries(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_EQUAL_INT(1, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16)); + gnrc_netreg_release_shared(); + TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[1])); + + gnrc_netreg_acquire_shared(); TEST_ASSERT_EQUAL_INT(2, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16)); + gnrc_netreg_release_shared(); } void test_netreg_getnext__NULL(void) { TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0])); + gnrc_netreg_acquire_shared(); TEST_ASSERT_NULL(gnrc_netreg_getnext(NULL)); + gnrc_netreg_release_shared(); } void test_netreg_getnext__2_entries(void) @@ -135,8 +179,10 @@ void test_netreg_getnext__2_entries(void) gnrc_netreg_entry_t *res = NULL; test_netreg_num__2_entries(); + gnrc_netreg_acquire_shared(); TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16))); TEST_ASSERT_NOT_NULL(gnrc_netreg_getnext(res)); + gnrc_netreg_release_shared(); } Test *tests_netreg_tests(void)