core: recursive mutex implementation #5731
Conversation
|
Why not just have a function |
|
Alternatively make it an optional module in |
miri64
added
Area: core
|
I added it to core because of the discussion in #5656, but if sys is preferable, i can move it as well. |
|
|
|
Arghs, sorry >.<! Should have examined the code more carefully. Might be okay to have this in core after all. |
|
(I leave this decision to the assigned reviewers though). |
miri64
added
the
CI: ready for build
core/rmutex.c
Outdated
| kernel_pid_t owner; | ||
| /* try to lock the mutex */ | ||
| DEBUG("rmutex %" PRIi16" : trylock\n", thread_getpid()); | ||
| switch (mutex_trylock(&rmutex->mutex)) { |
There was a problem hiding this comment.
Please use if (… == 0) { instead of switch with a fall-through.
|
IMO the implementation is small and useful enough to go into mutex.c directly. Most is not all platforms link with |
Kijewski
removed
the
CI: ready for build
Well it still uses another type. |
|
It seems like some comments by @miri64 on this PR that I received via email disappeared? Anyway, regarding atomic accesses, please use atomic_int, alternatively, with #5688 applied, use |
|
@gebart you mean the one above: #5731 (comment) |
|
@miri64 yes, I guess it was just temporary blindness then |
|
I posted an update using I think |
|
Did you try _Atomic(kernel_pid_t)? |
|
_Atomic(kernel_pid_t) works on some boards |
core/rmutex.c
Outdated
| rmutex->refcount--; | ||
|
|
||
| /* check if we still hold the mutex */ | ||
| if (rmutex->refcount==0) { |
There was a problem hiding this comment.
spaces before and after ==
|
I guess To fix your error when adding CFLAGS you could try -std=gnu11 instead, I think that will fix the missing typedefs. |
|
Ok with gnu11 it works on the native board but not at stmf3discovery board. |
|
I think we agreed to use an atomic_XXX_t for owner. But I am still not sure which one is best in this case. I tend to |
core/rmutex.c
Outdated
| /* mutex is already held | ||
| * | ||
| * Case 1: the current thread holds the mutex | ||
| * Invariant 1: holds |
There was a problem hiding this comment.
Maybe I'm missing something, but what invariant are you referring to here?
There was a problem hiding this comment.
Invariant is an incorrect word i think.
the point is that the outcome of the condition
if ( owner != thread_getpid() )
well never change even if rmutex->owner is changed by an other thread (holder of the mutex)
i will update the comment occrdingly
|
I think |
|
(Not very elegant, agreed...) |
|
insn't there something like |
|
Have you tried |
|
You could also check for the compiler version, I guess. |
|
OFF Topic: I think i just found an issue in ATOMIC_VALUE on avr8 copiles to you see two Problem case: Suppose a higher priority thread interrupts just after the first |
|
to avoid doing work twice i tell you what i will do:
|
Sounds like a good plan to me. I can test on MSP430. |
|
Regarding the AVR problem: could you open an issue, please? |
|
@melshuber I will move the the milestone label now as we will 'feature freeze' in about the next minutes. That must not mean that your PR can't be merged to master once approved and built correctly. |
|
Seems reasonable. |
|
Now that #5688 has been merged i think most of the problems However before i rebase i'd like to address the other two issues left in #5731 (comment). Bullet 1) static-tests - I have still no idea how to fix it? |
The error pops up when at the start of the build, the "needs squashing" label is set. It isn't anymore.
You mean removing the board with little memory? that's perfect! |
In contrast to normal mutexes, reeentrant mutexes allow to be relocked multiple times from the same thread.
* fixed BOARD_INSUFFICIENT_MEMORY for rmutex
melshuber
force-pushed
the
master
branch
2 times, most recently
from
e083cf6 to
b9bb22b
Compare
|
Sorry, i accidently pushed some garbage, it should be already fixed - hower you might want to retrigger jenkins |
core/rmutex.c
Outdated
| * Condition 2: holds | ||
| * rmutex->owner == thread_getpid() | ||
| * | ||
| * Note for Case 1: |
core/rmutex.c
Outdated
| kernel_pid_t owner; | ||
|
|
||
| /* try to lock the mutex */ | ||
| if (mutex_trylock(&rmutex->mutex) == 0) { |
There was a problem hiding this comment.
Some code deduplication possible here, you could make an internal function like
int _rmutex_lock(rmutex_t *rmutex, bool try)
{
kernel_pid_t owner;
if (mutex_trylock(&rmutex->mutex) == 0) {
owner = atomic_load_explicit( &rmutex->owner, memory_order_relaxed);
if ( owner != thread_getpid() ) {
if (try) {
return 0;
}
mutex_lock(&rmutex->mutex);
}
/* and so on */
}
void rmutex_lock(rmutex_t *rmutex)
{
_rmutex_lock(rmutex, false);
}
int rmutex_trylock(rmutex_t *rmutex)
{
return _rmutex_lock(rmutex, true);
}
There was a problem hiding this comment.
Good points, I aggree.
Do you prefer a the update squashed or as separate commit
There was a problem hiding this comment.
please make the update into a separate commit for reviewing first, but it should be squashed before merge.
|
looks good now, tests works: ACK. Still, I'd rather leave the final merge to someone more familiar with RIOT core ... |
| void rmutex_unlock(rmutex_t *rmutex) | ||
| { | ||
| assert(atomic_load_explicit(&rmutex->owner,memory_order_relaxed) == thread_getpid()); | ||
| assert(rmutex->refcount > 0); |
There was a problem hiding this comment.
Sorry for the late commenting, but I just stumbled over this:
This means calling rmutex_unlock() from as non-owner or with refcount==0 is undefined? Wouldn't it be safer to handle those cases somehow sane?
E.g., after calling rmutex_unlock(), the mutex is guarantueed to be free. That would mean, if non-owner calls, it needs to lock first. If the owner calls but refcount==0, the function just returns.
What do you think?
There was a problem hiding this comment.
I just checked, C11 rmutexes have "The behavior is undefined if the mutex is not locked by the calling thread.", so I guess we can go with that. So let's postpone this discussion.
There was a problem hiding this comment.
i think calling unlock on without owning the lock is an error in the application, iff doing so the behaviour can be left undefined. We could argue to add a comment in the api description
|
&go. |
This recursive Mute implementation is derived from the non-recursive variant.
The implementation is basically a port of
https://github.com/RIOT-OS/RIOT/pull/4529/files#diff-8f48e1b9ed7a0a48d0c686a87cc5084eR35.