[patch] Fix unchecked atomic_cmpset when unlocking mutex
John Baldwin
jhb at freebsd.org
Wed Dec 21 18:46:28 UTC 2011
On Tuesday, December 20, 2011 5:26:17 pm Jilles Tjoelker wrote:
> When unlocking a contested mutex that is not PI or PP, the mutex is made
> available for new lockers faster by unlocking it in userland and only
> calling the kernel to wake sleeping threads. This seems to make sense.
>
> Although the atomic_cmpset here should never fail because this thread
> owns the mutex and other threads may only set UMUTEX_CONTESTED which is
> already set, it seems prudent anyway to check for this.
>
> If the atomic_cmpset fails, use the slow path that unlocks in the
> kernel.
>
> Index: lib/libthr/thread/thr_umtx.c
> ===================================================================
> --- lib/libthr/thread/thr_umtx.c (revision 228504)
> +++ lib/libthr/thread/thr_umtx.c (working copy)
> @@ -154,10 +154,9 @@
> {
> #ifndef __ia64__
> /* XXX this logic has a race-condition on ia64. */
> - if ((mtx->m_flags & (UMUTEX_PRIO_PROTECT | UMUTEX_PRIO_INHERIT)) == 0) {
> - atomic_cmpset_rel_32(&mtx->m_owner, id | UMUTEX_CONTESTED, UMUTEX_CONTESTED);
> + if ((mtx->m_flags & (UMUTEX_PRIO_PROTECT | UMUTEX_PRIO_INHERIT)) == 0 &&
> + atomic_cmpset_rel_32(&mtx->m_owner, id | UMUTEX_CONTESTED, UMUTEX_CONTESTED))
> return _umtx_op_err(mtx, UMTX_OP_MUTEX_WAKE, 0, 0, 0);
> - }
> #endif /* __ia64__ */
> return _umtx_op_err(mtx, UMTX_OP_MUTEX_UNLOCK, 0, 0, 0);
> }
Not checking for a failing atomic_cmpset seems inherently broken to me. When
I first saw this, I wondered if fixing this would fix the "race" on ia64.
--
John Baldwin
More information about the freebsd-threads
mailing list