libthr shared locks
Eric van Gyzen
vangyzen at FreeBSD.org
Fri Feb 12 23:24:44 UTC 2016
On 12/23/2015 11:25, Konstantin Belousov wrote:
> The implementation in the patch
> gives shared mutexes, condvars, rwlocks and barriers.
I reviewed everything except kern_umtx.c, which I plan to review on
Monday. Here are my comments so far.
Thank you for converting some macros to functions. I find functions
much cleaner and easier to read and debug.
* thr_mutex.c line 116
The parentheses around (m) can be removed now.
* thr_mutex.c lines 331-333
This seems to read the m_qe.tqe_prev field from a shared mutex. Is that
safe? It seems like a race. The following would seem more direct,
avoiding the shared mutex:
m->m_qe.tqe_prev = &TAILQ_NEXT(last_prev, m_qe);
* thr_mutex.c line 354
*(q)->tqh_last = last_priv;
This seems to modify the tqe_next field in a shared mutex. Is that
safe? Furthermore, that mutex was/is the last on the list, but we seem
to set its tqe_next pointer to an earlier element, creating a cycle in
* thr_mutex.c line 451
__pthread_mutex_trylock() calls __thr_pshared_offpage() twice [for
pshared mutexes]. You could eliminate one call by moving
* thr_pshared.c line 165
res = NULL seems unnecessary.
In __thr_pshared_offpage(), can pshared_lookup() fail in the !doalloc
case? pshared_hash seems to be an authority, not just an optimization.
I ask so that I can understand the code and more effectively review it.
In particular, if pshared_lookup() fails and UMTX_SHM_LOOKUP succeeds,
is it possible to get multiple mappings for the same shm object?
* thr_barrier.c line 110
if (bar == NULL)
POSIX does not mention EFAULT. Should we return ENOMEM, or can we
"extend" the standard? (Ditto for all other _init functions.)
* thr_cond.c line 106
You could use cattr instead of the ?: expression.
rwlock_init() assumes that __thr_pshared_offpage() does not fail.
More information about the freebsd-threads