From 084a3fb5d5c2280115793d192fe96944b85979c9 Mon Sep 17 00:00:00 2001 From: Jon Shallow Date: Thu, 11 Jul 2024 08:56:18 +0100 Subject: [PATCH] Locking: Use single global lock instead of context lock When context is de-allocated, other threads can still access the context lock held within context. Indicating being freed when coap_free_context() was called worked in most cases, but not all. --- include/coap3/coap_net_internal.h | 6 - include/coap3/coap_threadsafe_internal.h | 379 +++++++++++------------ man/coap_locking.txt.in | 72 ++--- src/coap_net.c | 16 +- src/coap_session.c | 1 + src/coap_threadsafe.c | 98 +++--- 6 files changed, 265 insertions(+), 307 deletions(-) diff --git a/include/coap3/coap_net_internal.h b/include/coap3/coap_net_internal.h index 2a4f270f92..c840b1628e 100644 --- a/include/coap3/coap_net_internal.h +++ b/include/coap3/coap_net_internal.h @@ -195,12 +195,6 @@ struct coap_context_t { basis */ #endif /* COAP_SERVER_SUPPORT */ uint32_t block_mode; /**< Zero or more COAP_BLOCK_ or'd options */ -#if COAP_THREAD_SAFE - /** - * Context lock for multi-thread support - */ - coap_lock_t lock; -#endif /* COAP_THREAD_SAFE */ }; /** diff --git a/include/coap3/coap_threadsafe_internal.h b/include/coap3/coap_threadsafe_internal.h index 1de9c3e03a..211afefb56 100644 --- a/include/coap3/coap_threadsafe_internal.h +++ b/include/coap3/coap_threadsafe_internal.h @@ -14,6 +14,13 @@ * @brief CoAP mapping of locking functions */ +/** + * @ingroup internal_api + * @defgroup locking_internal Multi-thread Support + * Internal API for Multi-thread Locking Support + * @{ + */ + #ifndef COAP_THREADSAFE_INTERNAL_H_ #define COAP_THREADSAFE_INTERNAL_H_ @@ -31,45 +38,59 @@ * it has to be locked, but a retransmission of a PDU by coap_process_io() * has the context already locked. * - * So the initial support for thread safe is done at the context level. + * However, when the context is going away (coap_free_context()), other + * threads may still be access the lock in what is now freed memory. + * A solution (by flagging being freed), worked, but still with a timing + * window wen the context was finally de-allocated. Coverity Scan did + * not like the solution. * - * Any public API call needs to potentially lock context, as there may be - * multiple contexts. If a public API needs thread safe protection, the - * coap_X() function locks the context lock, calls the coap_X_lkd() function - * that does all the work and on return unlocks the context before returning - * to the caller of coap_X(). + * So the initial support for thread safe is done at global lock level + * using global_lock. However, context is provided as a parameter should + * context level locking be used. + * + * Any public API call needs to potentially lock global_lock. + * + * If a public API needs thread safe protection, the coap_X() function + * locks the global_lock lock, calls the coap_X_lkd() function + * that does all the work and on return unlocks the global_lock before + * returning * to the caller of coap_X(). These coap_X() functions + * need COAP_API in their definitions. * * Any internal libcoap calls that are to the public API coap_X() must call * coap_X_lkd() if the calling code is already locked. + * [The compiler will throw out a deprecation warning against any internal + * libcoap call to a COAP_API labelled function] * * Any call-back into app space must be done by using the coap_lock_callback() - * (or coap_lock_callback_ret()) wrapper where the context remains locked. + * (or coap_lock_callback_ret()) wrapper where the global_lock remains locked. * * Note: * libcoap may call a handler, which may in turn call into libcoap, which may - * then call a handler. context will remain locked thoughout this process + * then call a handler. global_lock will remain locked thoughout this process * by the same thread. * * Alternatively, coap_lock_callback_release() (or - * coap_lock_callback_ret_release()), is used where the context is unlocked + * coap_lock_callback_ret_release()), is used where the global_lock is unlocked * for the duration of the call-back. Used for things like a request * handler which could be busy for some time. * * Note: On return from the call-back, the code has to be careful not to - * use memory locations that make have been updated in the call-back by + * use memory locations that may have been updated in the call-back by * calling a Public API. * * Any wait on select() or equivalent when a thread is waiting on an event - * must be preceded by unlock context, and then context re-locked after + * must be preceded by unlock global_lock, and then global_lock re-locked after * return; * * To check for recursive deadlock coding errors, COAP_THREAD_RECURSIVE_CHECK * needs to be defined. * - * If thread safe is not enabled, then locking of the context does not take + * If thread safe is not enabled, then locking of the global_lock does not take * place. */ + #if COAP_THREAD_SAFE + # if COAP_THREAD_RECURSIVE_CHECK /* @@ -78,176 +99,158 @@ typedef struct coap_lock_t { coap_mutex_t mutex; coap_thread_pid_t pid; - coap_thread_pid_t freeing_pid; const char *lock_file; unsigned int lock_line; unsigned int unlock_line; const char *unlock_file; const char *callback_file; unsigned int callback_line; - unsigned int being_freed; unsigned int in_callback; unsigned int lock_count; } coap_lock_t; /** - * Unlock the (context) lock. + * Unlock the global_lock lock. * If this is a nested lock (Public API - libcoap - app call-back - Public API), - * then the lock remains locked, but lock->in_callback is decremented. + * then the lock remains locked, but global_lock.in_callback is decremented. + * + * Note: Invoked by wrapper macro, not used directly. * - * @param lock The lock to unlock. * @param file The file from which coap_lock_unlock_func() is getting called. * @param line The line no from which coap_lock_unlock_func() is getting called. */ -void coap_lock_unlock_func(coap_lock_t *lock, const char *file, int line); +void coap_lock_unlock_func(const char *file, int line); /** - * Lock the (context) lock. + * Lock the global_lock lock. * If this is a nested lock (Public API - libcoap - app call-back - Public API), - * then increment the lock->in_callback. - * If lock->being_freed is set and @p force is not set, then lock ends up unlocked. + * then increment the global_lock.in_callback. * - * @param lock The lock to unlock. - * @param force If set, then lock even if lock->being_freed is set. - * @param file The file from which coap_lock_unlock_func() is getting called. - * @param line The line no from which coap_lock_unlock_func() is getting called. + * Note: Invoked by wrapper macro, not used directly. + * + * @param file The file from which coap_lock_lock_func() is getting called. + * @param line The line no from which coap_lock_lock_func() is getting called. * - * @return @c 0 if lock->being_freed is set (and @p force is not set), else @c 1. + * @return @c 0 if libcoap has not started (coap_startup() not called), else @c 1. */ -int coap_lock_lock_func(coap_lock_t *lock, int force, const char *file, int line); +int coap_lock_lock_func(const char *file, int line); /** + * libcoap library code. Lock The global_lock. + * * Invoked when * Not locked at all - * Not locked, context being freed * Locked, app call-back, call from app call-back * Locked, app call-back, call from app call-back, app call-back, call from app call-back * Result - * context locked - * context not locked if context being freed and @p failed is executed. @p failed must + * global_lock locked. + * global_lock not locked if libcoap not started and @p failed is executed. @p failed must * be code that skips doing the lock protected code. * - * @param c Context to lock. - * @param failed Code to execute on lock failure + * @param c Context. + * @param failed Code to execute on lock failure. * */ #define coap_lock_lock(c,failed) do { \ - assert(c); \ - if (!coap_lock_lock_func(&(c)->lock, 0, __FILE__, __LINE__)) { \ + if (!coap_lock_lock_func(__FILE__, __LINE__)) { \ failed; \ } \ } while (0) /** + * libcoap library code. Unlock The global_lock. + * * Unlocked when * Same thread locked context * Not when called from app call-back * - * @param c Context to unlock. + * @param c Context. */ #define coap_lock_unlock(c) do { \ - assert(c); \ - coap_lock_unlock_func(&(c)->lock, __FILE__, __LINE__); \ + coap_lock_unlock_func(__FILE__, __LINE__); \ } while (0) /** + * libcoap library code. Invoke an app callback, leaving global_lock locked. + * * Called when * Locked - * Unlocked by thread free'ing off context (need to lock over app call-back) * - * @param c Context to lock if not locked - * @param func app call-back function to invoke + * @param c Context. + * @param func app call-back function to invoke. * */ #define coap_lock_callback(c,func) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (being_freed) { \ - coap_lock_lock_func(&(c)->lock, 1, __FILE__, __LINE__); \ - } else { \ - coap_lock_check_locked(c); \ - } \ - (c)->lock.in_callback++; \ - (c)->lock.callback_file = __FILE__; \ - (c)->lock.callback_line = __LINE__; \ + coap_lock_check_locked(c); \ + global_lock.in_callback++; \ + global_lock.callback_file = __FILE__; \ + global_lock.callback_line = __LINE__; \ func; \ - (c)->lock.in_callback--; \ - if (being_freed) { \ - coap_lock_unlock_func(&(c)->lock, __FILE__, __LINE__); \ - } \ + global_lock.in_callback--; \ } while (0) /** + * libcoap library code. Invoke an app callback that has a return value, + * leaving global_lock locked. + * * Called when * Locked - * Unlocked by thread free'ing off context (need to lock over app call-back) * * @param r Return value from @func. - * @param c Context to lock. - * @param func app call-back function to invoke + * @param c Context. + * @param func app call-back function to invoke. * */ #define coap_lock_callback_ret(r,c,func) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (being_freed) { \ - coap_lock_lock_func(&(c)->lock, 1, __FILE__, __LINE__); \ - } else { \ - coap_lock_check_locked(c); \ - } \ - (c)->lock.in_callback++; \ - (c)->lock.callback_file = __FILE__; \ - (c)->lock.callback_line = __LINE__; \ + coap_lock_check_locked(c); \ + global_lock.in_callback++; \ + global_lock.callback_file = __FILE__; \ + global_lock.callback_line = __LINE__; \ (r) = func; \ - (c)->lock.in_callback--; \ - if (being_freed) { \ - coap_lock_unlock_func(&(c)->lock, __FILE__, __LINE__); \ - } \ + global_lock.in_callback--; \ } while (0) /** + * libcoap library code. Invoke an app callback, unlocking global_lock first. + * * Called when - * Locked (need to unlock over app call-back) - * Unlocked by thread free'ing off context + * Locked * - * @param c Context to unlock. - * @param func app call-back function to invoke - * @param failed Code to execute on lock failure + * @param c Context. + * @param func app call-back function to invoke. + * @param failed Code to execute on (re-)lock failure. * */ #define coap_lock_callback_release(c,func,failed) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (!being_freed) { \ - coap_lock_check_locked(c); \ - coap_lock_unlock(c); \ - func; \ - coap_lock_lock(c,failed); \ - } else { \ - func; \ - } \ + coap_lock_check_locked(c); \ + coap_lock_unlock(c); \ + func; \ + coap_lock_lock(c,failed); \ } while (0) /** + * libcoap library code. Invoke an app callback that has a return value, + * unlocking global_lock first. + * * Called when * Locked (need to unlock over app call-back) * Unlocked by thread free'ing off context * * @param r Return value from @func. * @param c Context to unlock. - * @param func app call-back function to invoke + * @param func app call-back function to invoke. * @param failed Code to execute on lock failure * */ #define coap_lock_callback_ret_release(r,c,func,failed) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (!being_freed) { \ - coap_lock_check_locked(c); \ - coap_lock_unlock(c); \ - (r) = func; \ - coap_lock_lock(c,failed); \ - } else { \ - (r) = func; \ - } \ + coap_lock_check_locked(c); \ + coap_lock_unlock(c); \ + (r) = func; \ + coap_lock_lock(c,failed); \ } while (0) +extern coap_lock_t global_lock; + # else /* ! COAP_THREAD_RECURSIVE_CHECK */ /* @@ -256,195 +259,182 @@ int coap_lock_lock_func(coap_lock_t *lock, int force, const char *file, int line typedef struct coap_lock_t { coap_mutex_t mutex; coap_thread_pid_t pid; - coap_thread_pid_t freeing_pid; - uint32_t being_freed; uint32_t in_callback; volatile uint32_t lock_count; } coap_lock_t; /** - * Unlock the (context) lock. + * Unlock the global_lock lock. * If this is a nested lock (Public API - libcoap - app call-back - Public API), - * then the lock remains locked, but lock->in_callback is decremented. + * then the lock remains locked, but global_lock.in_callback is decremented. + * + * Note: Invoked by wrapper macro, not used directly. * - * @param lock The lock to unlock. */ -void coap_lock_unlock_func(coap_lock_t *lock); +void coap_lock_unlock_func(void); /** - * Lock the (context) lock. + * Lock the global_lock lock. * If this is a nested lock (Public API - libcoap - app call-back - Public API), - * then increment the lock->in_callback. - * If lock->being_freed is set and @p force is not set, then lock ends up unlocked. + * then increment the global_lock.in_callback. * - * @param lock The lock to unlock. - * @param force If set, then lock even if lock->being_freed is set. + * Note: Invoked by wrapper macro, not used directly. * - * @return @c 0 if lock->being_freed is set (and @p force is not set), else @c 1. + * @return @c 0 if libcoap has not started (coap_startup() not called), else @c 1. */ -int coap_lock_lock_func(coap_lock_t *lock, int force); +int coap_lock_lock_func(void); /** + * libcoap library code. Lock The global_lock. + * * Invoked when * Not locked at all - * Not locked, context being freed * Locked, app call-back, call from app call-back * Locked, app call-back, call from app call-back, app call-back, call from app call-back * Result - * context locked - * context not locked if context being freed and @p failed is executed. @p failed must + * global_lock locked. + * global not locked if libcoap not started and @p failed is executed. @p failed must * be code that skips doing the lock protected code. * - * @param c Context to lock. + * @param c Contex. * @param failed Code to execute on lock failure * */ #define coap_lock_lock(c,failed) do { \ assert(c); \ - if (!coap_lock_lock_func(&(c)->lock, 0)) { \ + if (!coap_lock_lock_func()) { \ failed; \ } \ } while (0) /** + * libcoap library code. Unlock The global_lock. + * * Unlocked when - * Same thread locked context - * Not when called from app call-back + * Same thread locked context. + * Not when called from app call-back. * - * @param c Context to unlock. + * @param c Context. */ #define coap_lock_unlock(c) do { \ assert(c); \ - coap_lock_unlock_func(&(c)->lock); \ + coap_lock_unlock_func(); \ } while (0) /** + * libcoap library code. Invoke an app callback, leaving global_lock locked. + * * Called when * Locked - * Unlocked by thread free'ing off context (need to lock over app call-back) * - * @param c Context to lock. - * @param func app call-back function to invoke + * @param c Context. + * @param func app call-back function to invoke. * */ #define coap_lock_callback(c,func) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (being_freed) { \ - coap_lock_lock_func(&(c)->lock, 1); \ - } else { \ - coap_lock_check_locked(c); \ - } \ - (c)->lock.in_callback++; \ + coap_lock_check_locked(c); \ + global_lock.in_callback++; \ func; \ - (c)->lock.in_callback--; \ - if (being_freed) { \ - coap_lock_unlock_func(&(c)->lock); \ - } \ + global_lock.in_callback--; \ } while (0) /** + * libcoap library code. Invoke an app callback that has a return value, + * leaving global_lock locked. + * * Called when * Locked - * Unlocked by thread free'ing off context (need to lock over app call-back) * * @param r Return value from @func. - * @param c Context to lock. - * @param func app call-back function to invoke + * @param c Context. + * @param func app call-back function to invoke. * */ #define coap_lock_callback_ret(r,c,func) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (being_freed) { \ - coap_lock_lock_func(&(c)->lock, 1); \ - } else { \ - coap_lock_check_locked(c); \ - } \ - (c)->lock.in_callback++; \ - (c)->lock.in_callback++; \ + coap_lock_check_locked(c); \ + global_lock.in_callback++; \ + global_lock.in_callback++; \ (r) = func; \ - (c)->lock.in_callback--; \ - if (being_freed) { \ - coap_lock_unlock_func(&(c)->lock); \ - } \ + global_lock.in_callback--; \ } while (0) /** + * libcoap library code. Invoke an app callback, unlocking global_lock first. + * * Called when * Locked (need to unlock over app call-back) - * Unlocked by thread free'ing off context * - * @param c Context to unlock. - * @param func app call-back function to invoke - * @param failed Code to execute on lock failure + * @param c Context. + * @param func app call-back function to invoke. + * @param failed Code to execute on (re-)lock failure. * */ #define coap_lock_callback_release(c,func,failed) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (!being_freed) { \ - coap_lock_check_locked(c); \ - coap_lock_unlock(c); \ - func; \ - coap_lock_lock(c,failed); \ - } else { \ - func; \ - } \ + coap_lock_check_locked(c); \ + coap_lock_unlock(c); \ + func; \ + coap_lock_lock(c,failed); \ } while (0) /** + * libcoap library code. Invoke an app callback that has a return value, + * unlocking global_lock first. + * * Called when * Locked (need to unlock over app call-back) - * Unlocked by thread free'ing off context * * @param r Return value from @func. - * @param c Context to unlock. - * @param func app call-back function to invoke - * @param failed Code to execute on lock failure + * @param c Context. + * @param func app call-back function to invoke. + * @param failed Code to execute on lock failure. * */ #define coap_lock_callback_ret_release(r,c,func,failed) do { \ - int being_freed = (c)->lock.being_freed && coap_thread_pid == (c)->lock.freeing_pid; \ - if (!being_freed) { \ - coap_lock_check_locked(c); \ - coap_lock_unlock(c); \ - (r) = func; \ - coap_lock_lock(c,failed); \ - } else { \ - (r) = func; \ - } \ + coap_lock_check_locked(c); \ + coap_lock_unlock(c); \ + (r) = func; \ + coap_lock_lock(c,failed); \ } while (0) # endif /* ! COAP_THREAD_RECURSIVE_CHECK */ -#define coap_lock_init(c) do { \ - assert(c); \ - memset(&((c)->lock), 0, sizeof((c)->lock)); \ - coap_mutex_init(&(c)->lock.mutex); \ - } while (0) - -#define coap_lock_being_freed(c,failed) do { \ - coap_lock_lock(c,failed); \ - (c)->lock.being_freed = 1; \ - (c)->lock.freeing_pid = coap_thread_pid; \ - coap_lock_unlock(c); \ +/** + * libcoap library code. Initialize the global_lock. + */ +#define coap_lock_init() do { \ + memset(&global_lock.mutex, 0, sizeof(global_lock.mutex)); \ + coap_mutex_init(&global_lock.mutex); \ } while (0) +/** + * libcoap library code. Check that global_lock is locked. + */ #define coap_lock_check_locked(c) do { \ - assert((c) && \ - coap_thread_pid == ((c)->lock.being_freed ? (c)->lock.freeing_pid : \ - (c)->lock.pid)); \ + assert(coap_thread_pid == global_lock.pid); \ } while (0) -#define coap_lock_invert(c,func,f) do { \ +/** + * libcoap library code. Lock an alternative lock. To prevent + * locking order issues, global_lock is unlocked, the alternative + * lock is locked and then global_lock is re-locked. + * + * Called when + * Locked (need to unlock over locking of alternative lock) + * + * @param c Context. + * @param alt_lock Alternative lock locking code. + * @param failed Code to execute on lock failure. + * + */ +#define coap_lock_invert(c,alt_lock,failed) do { \ coap_lock_check_locked(c); \ - if (!(c)->lock.being_freed) { \ - coap_lock_unlock(c); \ - func; \ - coap_lock_lock(c,f); \ - } else { \ - func; \ - } \ + coap_lock_unlock(c); \ + alt_lock; \ + coap_lock_lock(c,failed); \ } while (0) +extern coap_lock_t global_lock; + #else /* ! COAP_THREAD_SAFE */ /* @@ -454,8 +444,7 @@ typedef coap_mutex_t coap_lock_t; #define coap_lock_lock(c,failed) #define coap_lock_unlock(c) -#define coap_lock_init(c) -#define coap_lock_being_freed(c,failed) +#define coap_lock_init() #define coap_lock_check_locked(c) {} #define coap_lock_callback(c,func) func #define coap_lock_callback_ret(r,c,func) (r) = func @@ -465,4 +454,6 @@ typedef coap_mutex_t coap_lock_t; #endif /* ! COAP_THREAD_SAFE */ +/** @} */ + #endif /* COAP_THREADSAFE_INTERNAL_H_ */ diff --git a/man/coap_locking.txt.in b/man/coap_locking.txt.in index d77867fcec..7dbe24917d 100644 --- a/man/coap_locking.txt.in +++ b/man/coap_locking.txt.in @@ -14,7 +14,6 @@ coap_locking, coap_lock_init, coap_lock_lock, coap_lock_unlock, -coap_lock_being_freed, coap_lock_check_locked, coap_lock_callback, coap_lock_callback_release, @@ -27,15 +26,12 @@ SYNOPSIS -------- *#include * -*void coap_lock_init(coap_context_t *_context_);* +*void coap_lock_init(void);* *void coap_lock_lock(coap_context_t *_context_, coap_code_t _failed_statement_);* *void coap_lock_unlock(coap_context_t *_context_);* -*void coap_lock_being_freed(coap_context_t *_context_, -coap_code_t _failed_statement_);* - *void coap_lock_check_locked(coap_context_t *_context_);* *void coap_lock_callback(coap_context_t *_context_, @@ -63,15 +59,15 @@ or *-lcoap-@LIBCOAP_API_VERSION@-tinydtls*. Otherwise, link with DESCRIPTION ----------- This man page focuses on the locking support provided for making libcoap -thread safe. Usage is internal to libcoap. +thread safe. Usage is internal to libcoap library. The functions are actually macros which create different code depending on what levels of locking has been configured. Locking uses *coap_mutex_*() functions. So, _failed_statement_ is the C code to execute if the -locking fails for any reason. This should only happen when an another -thread has called *coap_free_context*(3). This code should prevent +locking fails for any reason. This should only happen when *coap_cleanup*(3) +has been called, or *coap_startup*(3) has not been called. This code should prevent execution of the following code that would have been under the lock protection and certainly not cause the corresponding *coap_lock_unlock*() function to be called. @@ -88,28 +84,26 @@ not multi-thread access safe. COAP_THREAD_RECURSIVE_CHECK If set, and COAP_THREAD_SAFE is set, checks that if a lock is locked, it reports that the same lock is being (re-)locked. -Currently, locking is only done at the _context_ level for the Public API -functions where appropriate. Per _session_ was also considered, but things became -complicated with one thread locking _context_ / _session_ and another thread -trying to lock _session_ / _context_ in a different order. +Currently, locking is only done at the *global_lock* level for the Public API +functions where appropriate. -In principal, libcoap code internally should only unlock _context_ when waiting +In principal, libcoap code internally should only unlock *global_lock* when waiting on a *select*() or equivalent, or when calling a request handler, and then lock up again on function return. Any other unlock - app call-back - lock needs to be carefully analyzed as to any potential issues being created by the app call-back if it calls any Public API, updating any data that is relied on after lock takes place. -*coap_lock_callback*() (or *coap_lock_callback_ret*()) wrapper leaves the _context_ -locked when calling app call-back, but allows the app call-back to call a -Public API when in the locked state. +*coap_lock_callback*() (or *coap_lock_callback_ret*()) wrapper leaves the +*global_lock* locked when calling app call-back, but allows the app call-back to +call a Public API when in the locked state. *coap_lock_callback_release*() (or *coap_lock_callback_ret_release*()) unlocks -_context_ when calling app call-back. The allows the app call-back to go off +*global_lock* when calling app call-back. The allows the app call-back to go off and do other slow/blocking activity. Any calls to a Public API then locks up -_context_ before preceding. +*global_lock* before preceding. -Any libcoap code that runs with _context_ locked should not call a Public API, +Any libcoap code that runs with *global_lock* locked should not call a Public API, but call the _lkd equivalent (if available). FUNCTIONS @@ -117,41 +111,33 @@ FUNCTIONS *Function: coap_lock_init()* -The *coap_lock_init*() function is used to initialize the lock structure -in the _context_ structure. +The *coap_lock_init*() function is used to initialize the *global_lock* lock +structure. *Function: coap_lock_lock()* -The *coap_lock_lock*() function is used to lock _context_ from multiple thread +The *coap_lock_lock*() function is used to lock *global_lock* from multiple thread access. If the locking fails for any reason, then _failed_statement_ will get executed. *Function: coap_lock_unlock()* -The *coap_lock_unlock*() function is used to unlock _context_ so that another -thread can access _context_ and the underlying structures. - -*Function: coap_lock_being_freed()* - -The *coap_lock_being_freed*() function is used to lock _context_ when _context_ -and all the underlying structures are going to be released (called from -*coap_free_context*(3)). Any subsequent call to *coap_lock_lock*() by another -thread will fail. If this locking fails for any reason, then _failed_statement_ -will get executed. +The *coap_lock_unlock*() function is used to unlock *global_lock* so that another +thread can access libcoap and the underlying structures. *Function: coap_lock_check_lock()* The *coap_lock_check_lock*() function is used to check the internal version -(potentially has __locked_ appended in the name) of a public AP is getting called -with _context_ locked. +(potentially has __lkd_ appended in the name) of a public AP is getting called +with *global_lock* locked. *Function: coap_lock_callback()* The *coap_lock_callback*() function is used whenever a callback handler is getting called, instead of calling the function directly. The lock information -in _context_ is updated so that if a public API is called from within the handler, +in *global_lock* is updated so that if a public API is called from within the handler, recursive locking is enabled for that particular thread. On return from the -callback, the lock in _context_ is suitably restored. _callback_function_ is the +callback, the lock in *global_lock* is suitably restored. _callback_function_ is the callback handler to be called, along with all of the appropriate parameters. *Function: coap_lock_callback_ret()* @@ -164,9 +150,9 @@ in _return_value_. The *coap_lock_callback_release*() function is used whenever a callback handler is getting called, instead of calling the function directly. The lock information -in _context_ is released so that if a public API is called from within the handler, +in *global_lock* is released so that if a public API is called from within the handler, it can do its own lock. The intent here is to reduce lock contention. On return -from the callback, the lock in _context_ is re-locked, but if there is a failure in +from the callback, the lock in *global_lock* is re-locked, but if there is a failure in re-locking, _failed_statement_ is executed. _callback_function_ is the callback handler to be called, along with all of the appropriate parameters. @@ -180,16 +166,12 @@ callback handler function in _return_value_. The *coap_lock_invert*() function is used where there are other locking mechanisms external to libcoap and the locking order needs to be external lock, -then libcoap code locked. _context_ already needs to be locked before calling -*coap_lock_invert*(). If *coap_lock_invert*() is called, then _context_ will +then libcoap code locked. *global_lock* already needs to be locked before calling +*coap_lock_invert*(). If *coap_lock_invert*() is called, then *global_lock* will get unlocked, _locking_function_ with all of its parameters called, and then -_context_ re-locked. If for any reason locking fails, then _failed_statement_ +*global_lock* re-locked. If for any reason locking fails, then _failed_statement_ will get executed. -SEE ALSO --------- -*coap_context*(3) - FURTHER INFORMATION ------------------- See diff --git a/src/coap_net.c b/src/coap_net.c index 91f62e78b5..787e3e12a2 100644 --- a/src/coap_net.c +++ b/src/coap_net.c @@ -205,6 +205,7 @@ coap_delete_node(coap_queue_t *node) { int ret; #if COAP_THREAD_SAFE coap_context_t *context; + (void)context; #endif /* COAP_THREAD_SAFE */ if (!node) @@ -584,7 +585,7 @@ coap_new_context(const coap_address_t *listen_addr) { } memset(c, 0, sizeof(coap_context_t)); - coap_lock_init(c); + coap_lock_init(); coap_lock_lock(c, coap_free_type(COAP_CONTEXT, c); return NULL); #ifdef COAP_EPOLL_SUPPORT c->epfd = epoll_create1(0); @@ -676,9 +677,8 @@ coap_free_context(coap_context_t *context) { * Note there is an immediate unlock to release any other 'context' waiters * So that their coap_lock_lock() will fail as 'context' is realy no more. */ - coap_lock_being_freed(context, return); + coap_lock_lock(context, return); coap_free_context_lkd(context); - /* No need to unlock as context is no longer there */ } void @@ -4404,6 +4404,12 @@ 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 + */ +coap_lock_t global_lock; +#endif /* COAP_THREAD_SAFE */ void coap_startup(void) { @@ -4425,6 +4431,10 @@ coap_startup(void) { coap_mutex_init(&m_persist_add); #endif /* COAP_CONSTRAINED_STACK */ +#if COAP_THREAD_SAFE + coap_lock_init(); +#endif /* COAP_THREAD_SAFE */ + #if defined(HAVE_WINSOCK2_H) WORD wVersionRequested = MAKEWORD(2, 2); WSADATA wsaData; diff --git a/src/coap_session.c b/src/coap_session.c index 80313114cc..33b7ab2b01 100644 --- a/src/coap_session.c +++ b/src/coap_session.c @@ -363,6 +363,7 @@ coap_session_release(coap_session_t *session) { if (session) { #if COAP_THREAD_SAFE coap_context_t *context = session->context; + (void)context; #endif /* COAP_THREAD_SAFE */ coap_lock_lock(context, return); diff --git a/src/coap_threadsafe.c b/src/coap_threadsafe.c index 0a723d8cad..dc9dcf3bbe 100644 --- a/src/coap_threadsafe.c +++ b/src/coap_threadsafe.c @@ -18,103 +18,83 @@ #if COAP_THREAD_SAFE #if COAP_THREAD_RECURSIVE_CHECK void -coap_lock_unlock_func(coap_lock_t *lock, const char *file, int line) { - assert(coap_thread_pid == lock->pid); - if (lock->in_callback) { - assert(lock->lock_count > 0); - lock->lock_count--; +coap_lock_unlock_func(const char *file, int line) { + assert(coap_thread_pid == global_lock.pid); + if (global_lock.in_callback) { + assert(global_lock.lock_count > 0); + global_lock.lock_count--; } else { - lock->pid = 0; - lock->unlock_file = file; - lock->unlock_line = line; - coap_mutex_unlock(&lock->mutex); + global_lock.pid = 0; + global_lock.unlock_file = file; + global_lock.unlock_line = line; + coap_mutex_unlock(&global_lock.mutex); } } int -coap_lock_lock_func(coap_lock_t *lock, int force, const char *file, int line) { - if ((!force && lock->being_freed) || !coap_started) { - /* context is going away or libcoap not initialized with coap_startup() */ +coap_lock_lock_func(const char *file, int line) { + if (!coap_started) { + /* libcoap not initialized with coap_startup() */ return 0; } - if (coap_mutex_trylock(&lock->mutex)) { - if (coap_thread_pid == lock->pid) { + if (coap_mutex_trylock(&global_lock.mutex)) { + if (coap_thread_pid == global_lock.pid) { /* This thread locked the mutex */ - if (lock->in_callback) { + if (global_lock.in_callback) { /* This is called from within an app callback */ - lock->lock_count++; - assert(lock->in_callback == lock->lock_count); + global_lock.lock_count++; + assert(global_lock.in_callback == global_lock.lock_count); return 1; } else { coap_log_alert("Thread Deadlock: Last %s: %u, this %s: %u\n", - lock->lock_file, lock->lock_line, file, line); + global_lock.lock_file, global_lock.lock_line, file, line); assert(0); } } /* Wait for the other thread to unlock */ - coap_mutex_lock(&lock->mutex); + coap_mutex_lock(&global_lock.mutex); } /* Just got the lock, so should not be in a locked callback */ - assert(!lock->in_callback); - lock->pid = coap_thread_pid; - lock->lock_file = file; - lock->lock_line = line; -#ifndef __COVERITY__ - if (!force && lock->being_freed) { - /* - * lock->being_freed set when previous thread had lock locked. - * Have to unlock to let the next context locking user clean up. - */ - coap_lock_unlock_func(lock, file, line); - return 0; - } -#endif /* __COVERITY__ */ + assert(!global_lock.in_callback); + global_lock.pid = coap_thread_pid; + global_lock.lock_file = file; + global_lock.lock_line = line; return 1; } #else /* ! COAP_THREAD_RECURSIVE_CHECK */ void -coap_lock_unlock_func(coap_lock_t *lock) { - assert(coap_thread_pid == lock->pid); - if (lock->in_callback) { - assert(lock->lock_count > 0); - lock->lock_count--; +coap_lock_unlock_func(void) { + assert(coap_thread_pid == global_lock.pid); + if (global_lock.in_callback) { + assert(global_lock.lock_count > 0); + global_lock.lock_count--; } else { - lock->pid = 0; - coap_mutex_unlock(&lock->mutex); + global_lock.pid = 0; + coap_mutex_unlock(&global_lock.mutex); } } int -coap_lock_lock_func(coap_lock_t *lock, int force) { - if ((!force && lock->being_freed) || !coap_started) { - /* context is going away or libcoap not initialized with coap_startup() */ +coap_lock_lock_func(void) { + if (!coap_started) { + /* libcoap not initialized with coap_startup() */ return 0; } /* * Some OS do not have support for coap_mutex_trylock() so * cannot use that here and have to rely on lock-pid being stable */ - if (lock->in_callback && coap_thread_pid == lock->pid) { - lock->lock_count++; - assert(lock->in_callback == lock->lock_count); + if (global_lock.in_callback && coap_thread_pid == global_lock.pid) { + global_lock.lock_count++; + assert(global_lock.in_callback == global_lock.lock_count); return 1; } - coap_mutex_lock(&lock->mutex); + coap_mutex_lock(&global_lock.mutex); /* Just got the lock, so should not be in a locked callback */ - assert(!lock->in_callback); - lock->pid = coap_thread_pid; -#ifndef __COVERITY__ - if (!force && lock->being_freed) { - /* - * lock->being_freed set when previous thread had lock locked. - * Have to unlock to let the next context locking user clean up. - */ - coap_lock_unlock_func(lock); - return 0; - } -#endif /* __COVERITY__ */ + assert(!global_lock.in_callback); + global_lock.pid = coap_thread_pid; return 1; } #endif /* ! COAP_THREAD_RECURSIVE_CHECK */