libthr shared locks

[Eric van Gyzen]

On 12/23/2015 11:25, Konstantin Belousov wrote:
> The implementation in the patch
> https://www.kib.kiev.ua/kib/pshared/pshared.1.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.

* thr_mutex.c

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

    m->m_qe.tqe_prev =
        TAILQ_NEXT(last_priv, m_qe)->
        m_qe.tqe_prev;                         

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
the list.

* thr_mutex.c line 451

__pthread_mutex_trylock() calls __thr_pshared_offpage() twice [for
pshared mutexes].  You could eliminate one call by moving
mutex_trylock_common() inline.

* thr_pshared.c line 165

res = NULL seems unnecessary.

* thr_pshared.c

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)
        return (EFAULT);

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.

* thr_rwlock.c

rwlock_init() assumes that __thr_pshared_offpage() does not fail.