Skip to content

Commit

Permalink
multithread: Re-arrange lock checks for locked callbacks
Browse files Browse the repository at this point in the history 
Should fix CIDs
1583518-1583525
1583527-1583549
1583551-1583553
1583555-1583562
1583565-1583584
1583586-1583589
  • Loading branch information
@mrdeep1
mrdeep1 committed Feb 21, 2024
1 parent 7660069 commit a733325
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 64 deletions.
73 changes: 30 additions & 43 deletions include/coap3/coap_mutex_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,6 @@ extern coap_mutex_t m_persist_add;
#if COAP_THREAD_SAFE
# if COAP_THREAD_RECURSIVE_CHECK

typedef void (*coap_free_func_t)(void *stucture);

/*
* Locking, with deadlock detection
*/
Expand All @@ -228,32 +226,19 @@ void coap_lock_unlock_func(coap_lock_t *lock, const char *file, int line);
int coap_lock_lock_func(coap_lock_t *lock, const char *file, int line);

#define coap_lock_lock(s,failed) do { \
assert(s); \
if (!coap_lock_lock_func(&(s)->lock, __FILE__, __LINE__)) { \
failed; \
} \
} while (0)

#define coap_lock_unlock(s) do { \
coap_lock_unlock_func(&(s)->lock, __FILE__, __LINE__); \
} while (0)

#define coap_lock_init(s) do { \
memset(&((s)->lock), 0, sizeof((s)->lock)); \
coap_mutex_init(&(s)->lock.mutex); \
} while (0)

#define coap_lock_being_freed(s,failed) do { \
coap_lock_lock(s,failed); \
(s)->lock.being_freed = 1; \
(s)->lock.freeing_pid = coap_thread_pid; \
coap_lock_unlock(s); \
} while (0)

#define coap_lock_check_locked(s) do { \
assert ((s)->lock.being_freed ? coap_thread_pid == (s)->lock.freeing_pid: coap_thread_pid == (s)->lock.pid); \
assert(s); \
coap_lock_unlock_func(&(s)->lock, __FILE__, __LINE__); \
} while (0)

#define coap_lock_callback(s,func) do { \
coap_lock_check_locked(s); \
(s)->lock.in_callback++; \
(s)->lock.callback_file = __FILE__; \
(s)->lock.callback_line = __LINE__; \
Expand All @@ -262,30 +247,23 @@ int coap_lock_lock_func(coap_lock_t *lock, const char *file, int line);
} while (0)

#define coap_lock_callback_ret(r,s,func) do { \
coap_lock_check_locked(s); \
(s)->lock.in_callback++; \
(s)->lock.callback_file = __FILE__; \
(s)->lock.callback_line = __LINE__; \
r = func; \
(s)->lock.in_callback--; \
} while (0)

#define coap_lock_invert(s,func,f) do { \
if (!(s)->lock.being_freed) { \
coap_lock_unlock(s); \
func; \
coap_lock_lock(s,f); \
} else { \
func; \
} \
} while (0)

# else /* ! COAP_THREAD_RECURSIVE_CHECK */

/*
* Locking, but no deadlock detection
*/
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;
Expand All @@ -295,39 +273,52 @@ void coap_lock_unlock_func(coap_lock_t *lock);
int coap_lock_lock_func(coap_lock_t *lock);

#define coap_lock_lock(s,failed) do { \
assert(s); \
if (!coap_lock_lock_func(&(s)->lock)) { \
failed; \
} \
} while (0)

#define coap_lock_unlock(s) do { \
assert(s); \
coap_lock_unlock_func(&(s)->lock); \
} while (0)

#define coap_lock_callback(s,func) do { \
coap_lock_check_locked(s); \
(s)->lock.in_callback++; \
func; \
(s)->lock.in_callback--; \
} while (0)

#define coap_lock_callback_ret(r,s,func) do { \
coap_lock_check_locked(s); \
(s)->lock.in_callback++; \
r = func; \
(s)->lock.in_callback--; \
} while (0)

# endif /* ! COAP_THREAD_RECURSIVE_CHECK */

#define coap_lock_init(s) do { \
assert(s); \
memset(&((s)->lock), 0, sizeof((s)->lock)); \
coap_mutex_init(&(s)->lock.mutex); \
} while (0)

#define coap_lock_being_freed(s,failed) do { \
coap_lock_lock(s,failed); \
(s)->lock.being_freed = 1; \
(s)->lock.freeing_pid = coap_thread_pid; \
coap_lock_unlock(s); \
} while (0)

#define coap_lock_callback(s,func) do { \
(s)->lock.in_callback++; \
func; \
(s)->lock.in_callback--; \
} while (0)

#define coap_lock_callback_ret(r,s,func) do { \
(s)->lock.in_callback++; \
r = func; \
(s)->lock.in_callback--; \
#define coap_lock_check_locked(s) do { \
assert ((s) && (s)->lock.being_freed ? coap_thread_pid == (s)->lock.freeing_pid: coap_thread_pid == (s)->lock.pid); \
} while (0)

#define coap_lock_invert(s,func,f) do { \
coap_lock_check_locked(s); \
if (!(s)->lock.being_freed) { \
coap_lock_unlock(s); \
func; \
Expand All @@ -337,10 +328,6 @@ int coap_lock_lock_func(coap_lock_t *lock);
} \
} while (0)

#define coap_lock_check_locked(s) {}

# endif /* ! COAP_THREAD_RECURSIVE_CHECK */

#else /* ! COAP_THREAD_SAFE */

/*
Expand Down
43 changes: 22 additions & 21 deletions src/coap_threadsafe.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -760,11 +760,11 @@ coap_session_send_ping(coap_session_t *session) {
#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--;
} else {
assert(lock->pid != 0);
lock->pid = 0;
lock->unlock_file = file;
lock->unlock_line = line;
Expand All @@ -774,62 +774,63 @@ coap_lock_unlock_func(coap_lock_t *lock, const char *file, int line) {

int
coap_lock_lock_func(coap_lock_t *lock, const char *file, int line) {
if (coap_mutex_trylock(&lock->mutex)) {
if (lock->in_callback) {
if (coap_thread_pid != lock->pid) {
coap_mutex_lock(&lock->mutex);
} else {
lock->lock_count++;
assert(lock->in_callback == lock->lock_count);
}
} else {
if (lock->in_callback && coap_thread_pid == lock->pid) {
lock->lock_count++;
assert(lock->in_callback == lock->lock_count);
} else {
if (coap_mutex_trylock(&lock->mutex)) {
if (coap_thread_pid == lock->pid) {
coap_log_alert("Thread Deadlock: Last %s: %u, this %s: %u\n",
lock->lock_file, lock->lock_line, file, line);
assert(0);
}
coap_mutex_lock(&lock->mutex);
lock->pid = coap_thread_pid;
lock->lock_file = file;
lock->lock_line = line;
}
/* 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;
if (lock->being_freed) {
coap_lock_unlock_func(lock, file, line);
return 0;
}
} else {
lock->pid = coap_thread_pid;
lock->lock_file = file;
lock->lock_line = line;
}
return 1;
}
#else /* COAP_THREAD_RECURSIVE_CHECK */

#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--;
} else {
lock->pid = 0;
coap_mutex_unlock(&lock->mutex);
}
}

int
coap_lock_lock_func(coap_lock_t *lock) {
if (lock->in_callback) {
if (lock->in_callback && coap_thread_pid == lock->pid) {
lock->lock_count++;
assert(lock->in_callback == lock->lock_count);
} else {
coap_mutex_lock(&lock->mutex);
/* Just got the lock, so should not be in a locked callback */
assert(!lock->in_callback);
lock->pid = coap_thread_pid;
if (lock->being_freed) {
coap_lock_unlock_func(lock);
return 0;
}
}
return 1;
}
#endif /* COAP_THREAD_RECURSIVE_CHECK */

#endif /* ! COAP_THREAD_RECURSIVE_CHECK */

#else /* ! COAP_THREAD_SAFE */

Expand Down

0 comments on commit a733325

Please sign in to comment.