Repository navigation
net/unicoap: get rid of state locking - #23079
mguetschow wants to merge 5 commits into
Conversation
all functions manipulating state are now executed on a single thread, data races can therefore not occur anymore see RIOT-OS#22266 (comment)
github-actions
Bot
added
Area: network
| } | ||
|
|
||
| /** @brief Gets client memo array from @p state object */ | ||
| static inline unicoap_client_memo_t *unicoap_state_get_client_memos(unicoap_state_t* state) { |
There was a problem hiding this comment.
Since we only access those in state.c, they could be moved there. Just mirrored how unicoap_get_listeners were defined.
There was a problem hiding this comment.
If we move them there, we could get rid of the explicit _state() call at the call sites.
There was a problem hiding this comment.
Btw unicoap_get_listeners is currently also only called from within state.c. Do we foresee that to change in the future?
There was a problem hiding this comment.
Would anything speak against moving them to state.c for now?
mguetschow
added
the
CI: ready for build
| int unicoap_listener_deregister(unicoap_listener_t* listener) | ||
| { | ||
| assert(listener); | ||
|
|
||
| _listener_args_t args = { | ||
| .job = UNICOAP_JOB(_listener_deregister), | ||
| .listener = listener, | ||
| .roadblock = MUTEX_INIT_LOCKED, | ||
| }; | ||
| if (thread_getpid() == _unicoap_pid || _unicoap_pid == KERNEL_PID_UNDEF) { | ||
| _listener_deregister(&args.job); | ||
| } | ||
| else { | ||
| unicoap_loop_enqueue(&args.job); | ||
| /* Block until callback calls unlock. */ | ||
| mutex_lock(&args.roadblock); | ||
| } | ||
| return args.res; | ||
| } |
There was a problem hiding this comment.
Hm, I'm questioning if this overhead is really warranted... But going back to locks means basically locking for the duration of every queued event, so we'd block the calling thread anyway.
There was a problem hiding this comment.
One more thing: Either we modify unicoap_loop_enqueue or at every call site (including the client) we check if we're already on the queue/thread. because now I won't be able to call this function from within unicoap anymore.
There was a problem hiding this comment.
if this overhead is really warranted... But going back to locks
it's the same we do in the client functions. As you say, at runtime it is the same, but you need to make sure that you use locks everywhere consistently. There is some slight overhead on the stack of the calling thread, but that is just a few bytes for _listener_args_t.
at every call site (including the client) we check if we're already on the queue/thread
We already do that (see the if-clause above). We could only move that into unicoap_loop_enqueue if unicoap_job_t would always contain a mutex_t which is an overhead we don't want to pay? maybe adding a second unicoap_loop_enqueue_threadsafe could get an extended job that has such a mutex, to abstract away the mutex / thread checking logic?
There was a problem hiding this comment.
can't we just use locking around the listeners then?
There was a problem hiding this comment.
and can't we just check if we're already on the queue/thread by comparing the thread with our stored PID and then not do the enque stuff but instead directly call the job function? or even better, can you unlock a lock before locking it, like a semaphore? then we'd just need to adjust the enqueue function to check the PID and if true execute the job function directly.
|
Now with all these state getters, perhaps we could, paying the cost of a function call, move some functions into client.c and server.c? |
Which ones would you be thinking of? |
carl-tud
changed the title
| * @brief Size of @ref unicoap_messaging_state_udp_dtls_t.carbon_copy | ||
| */ | ||
| size_t pdu_size; | ||
| size_t carbon_copy_size; |
There was a problem hiding this comment.
| size_t carbon_copy_size; | |
| size_t carbon_copy_length; |
nit
There was a problem hiding this comment.
This is my fault, ignoring my own capacity/count/length rules
The client memo stuff into client/state.c, listener and CoRE link stuff into server/state.c and server/core.c |
|
Other than the size/length nit and listener situation, good to go. would like to rebase #23084 onto this. |
carl-tud
changed the title
Contribution description
All functions manipulating state are now (asserted to be) executed on a single thread, data races can therefore not occur anymore
Testing procedure
Play around with your favorite unicoap examples. Compile-time regressions should be spotted by Murdock.
Issues/PRs references
as discussed in #22266 (comment)
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: