dlt_user: Rework on dlt global mutex locks#780
Conversation
|
@lti9hc kindly review |
There was a problem hiding this comment.
Pull Request Overview
This pull request reworks the DLT global mutex locking mechanism to provide safe reentrant behavior and eliminate lock-order inversions. The main changes include implementing per-thread reentrancy counters using pthread keys, guarding cleanup handlers to prevent mutex underflow, and removing the problematic rwlock->mutex lock-order inversion through a compute-then-publish pattern.
Key Changes
- Replace
DLT_SEM_LOCK/DLT_SEM_FREEmacros with reentrantdlt_mutex_lock()/dlt_mutex_free()functions that use per-thread counters - Add guarded cleanup handlers that check reentry state before unlocking to prevent mutex underflow
- Eliminate lock-order inversion by computing trace-load settings under rwlock, then publishing pointers under DLT mutex separately
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/shared/dlt_common.c | Update comment to reference new mutex function name |
| src/lib/dlt_user.c | Replace all DLT_SEM_* macros with new reentrant mutex functions and implement per-thread reentry tracking |
| include/dlt/dlt_user.h.in | Remove old DLT_SEM_LOCK/DLT_SEM_FREE macro definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
minminlittleshrimp
force-pushed
the
dlt_devel_fix_global_mutex_locks
branch
3 times, most recently
from
26f52c9 to
1938c38
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
minminlittleshrimp
changed the title
minminlittleshrimp
force-pushed
the
dlt_devel_fix_global_mutex_locks
branch
from
1938c38 to
40e36e7
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
minminlittleshrimp
changed the title
- Harden mutex handling, make global mutex and cleanup more robust - Fix lock-ordering, prevent lock-order inversion between components Signed-off-by: LUU QUANG MINH <Minh.LuuQuang@vn.bosch.com>
minminlittleshrimp
force-pushed
the
dlt_devel_fix_global_mutex_locks
branch
from
40e36e7 to
067afa7
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ctx_entry->contextID); | ||
| pthread_rwlock_unlock(&trace_load_rw_lock); | ||
|
|
||
| dlt_mutex_lock(); |
There was a problem hiding this comment.
why do you want to use mutex lock here?
There was a problem hiding this comment.
trace_load_settings is a defined pointer included inside dlt_ll_ts_type, it is part of Dlt_user, at line 5034, an assignment of the pointer dlt_ll_ts from the struct to ctx_entry is performed, hence as the comment says, it should be protected with global mutex
| } | ||
|
|
||
| #ifdef DLT_TRACE_LOAD_CTRL_ENABLE | ||
| pthread_rwlock_wrlock(&trace_load_rw_lock); |
There was a problem hiding this comment.
It should not remove trace load lock
I suggest that using lock ordering
ex:
mutex_lock
trace load lock
trace load unlock
mutex unlock
There was a problem hiding this comment.
The removal of trace load rwlock here is correct since the inside code block only protecting Dlt_user struct, no related trace load setting struct
There was a problem hiding this comment.
rwlock shall be refactored after this merge, due to the fact that rwlock is abused (less read more write) and thus atomic shall be considered. The scope of this patch focus mainly on fixing bug of double lock, double free and mutex inversion ABBA from dlt_mutex and rwlock.
permitted in pid=46659
[17569.258894]~DLT~46659~ERROR ~DLT sem free error: 1 - Operation not permitted in pid=46659
Send 99999 Hello
[17569.258896]~DLT~46659~ERROR ~DLT sem lock error: 35 - Resource deadlock avoided pid=46659
[17569.258898]~DLT~46659~ERROR ~DLT sem lock error: 35 - Resource deadlock avoided pid=46659
[17569.258902]~DLT~46659~ERROR ~DLT sem free error: 1 - Operation not permitted in pid=46659
[17569.258906]~DLT~46659~ERROR ~DLT sem free error: 1 - Operation not permitted in pid=46659
[17570.259482]~DLT~46659~ERROR ~DLT sem free error: 1 - Operation not permitted in pid=46659
[17570.259500]~DLT~46659~ERROR ~DLT sem free error: 1 - Operation not permitted in pid=46659
Uh oh!
There was an error while loading. Please reload this page.