Date: Fri, 12 Feb 2016 17:24:40 -0600 From: Eric van Gyzen <vangyzen@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com>, threads@freebsd.org, arch@freebsd.org Subject: Re: libthr shared locks Message-ID: <56BE69B8.9020808@FreeBSD.org> In-Reply-To: <20151223172528.GT3625@kib.kiev.ua> References: <20151223172528.GT3625@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56BE69B8.9020808>