Date: Wed, 21 Dec 2011 13:44:51 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-threads@freebsd.org Subject: Re: [patch] Fix unchecked atomic_cmpset when unlocking mutex Message-ID: <201112211344.51792.jhb@freebsd.org> In-Reply-To: <20111220222617.GC75886@stack.nl> References: <20111220222617.GC75886@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112211344.51792.jhb>