This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PATCH,nptl] Fix lock elision of pthread_mutex_trylock vs unlock
- From: Samuel Thibault <samuel dot thibault at ens-lyon dot org>
- To: libc-alpha at sourceware dot org, Andi Kleen <ak at linux dot intel dot com>
- Cc: mjw at redhat dot com
- Date: Thu, 28 Aug 2014 19:58:27 +0200
- Subject: [PATCH,nptl] Fix lock elision of pthread_mutex_trylock vs unlock
- Authentication-results: sourceware.org; auth=none
Hello,
On hardware with RTM, the following crashes:
#include <pthread.h>
pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
int main(int argc, char *argv[]) {
pthread_mutex_trylock(&m);
pthread_mutex_unlock(&m);
if (pthread_mutex_destroy(&m))
*(int*)0 = 0;
return 0;
}
The state of the mutex is indeed:
(gdb) p/x m
$1 = {__data = {__lock = 0x0, __count = 0x0, __owner = 0x0, __nusers = 0xffffffff, __kind = 0x0,
__spins = 0x0, __elision = 0x3, __list = {__prev = 0x0, __next = 0x0}}, __size = {0x0 <repeats 12 times>,
0xff, 0xff, 0xff, 0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3, 0x0 <repeats 17 times>}, __align = 0x0}
What happens is that pthread_mutex_trylock does elision (DO_ELISION),
but pthread_mutex_unlock is not aware that trylock did, and doesn't do
elision, and thus erroneously does __nusers-- and pthread_mutex_destroy
returns EBUSY.
pthread_mutex_trylock.c mentions early in the file that we "don't force
elision in trylock, because this can lead to inconsistent lock state if
the lock was actually busy.". I don't know the details, but if trylock
should really not do elision, then it shouldn't do it :) The following
patch does this, and it indeed fixes the issue.
--- a/nptl/pthread_mutex_trylock.c
+++ b/nptl/pthread_mutex_trylock.c
@@ -77,9 +77,6 @@ __pthread_mutex_trylock (mutex)
return 0;
case PTHREAD_MUTEX_TIMED_NP:
- if (DO_ELISION (mutex))
- goto elision;
- /*FALL THROUGH*/
case PTHREAD_MUTEX_ADAPTIVE_NP:
case PTHREAD_MUTEX_ERRORCHECK_NP:
if (lll_trylock (mutex->__data.__lock) != 0)
But perhaps this case is actually safe (I haven't investigated the
details) and thus it's the unlock part which needs fixed, as the
following patch does:
Andy?
Samuel
diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 95ae933..299877b 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -27,6 +27,10 @@
#define lll_unlock_elision(a,b) ({ lll_unlock (a,b); 0; })
#endif
+#ifndef DO_ELISION
+#define DO_ELISION(m) 0
+#endif
+
static int
internal_function
__pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
@@ -44,7 +48,7 @@ __pthread_mutex_unlock_usercnt (mutex, decr)
return __pthread_mutex_unlock_full (mutex, decr);
if (__builtin_expect (type, PTHREAD_MUTEX_TIMED_NP)
- == PTHREAD_MUTEX_TIMED_NP)
+ == PTHREAD_MUTEX_TIMED_NP && !DO_ELISION(mutex))
{
/* Always reset the owner field. */
normal:
@@ -60,7 +64,9 @@ __pthread_mutex_unlock_usercnt (mutex, decr)
return 0;
}
- else if (__glibc_likely (type == PTHREAD_MUTEX_TIMED_ELISION_NP))
+ else if (__glibc_likely (type == PTHREAD_MUTEX_TIMED_ELISION_NP) ||
+ (__glibc_likely (type == PTHREAD_MUTEX_TIMED_NP) &&
+ DO_ELISION(mutex)))
{
/* Don't reset the owner/users fields for elision. */
return lll_unlock_elision (mutex->__data.__lock,
diff --git a/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c b/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c
new file mode 100644
index 0000000..048dd5d
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c
@@ -0,0 +1,22 @@
+/* Elided version of pthread_mutex_trylock.
+ Copyright (C) 2014 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <elision-conf.h>
+#include <force-elision.h>
+
+#include <nptl/pthread_mutex_unlock.c>
diff --git a/sysdeps/unix/sysv/linux/x86/pthread_mutex_unlock.c b/sysdeps/unix/sysv/linux/x86/pthread_mutex_unlock.c
new file mode 100644
index 0000000..80b594c
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/pthread_mutex_unlock.c
@@ -0,0 +1,22 @@
+/* Elided version of pthread_mutex_trylock.
+ Copyright (C) 2011-2014 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <elision-conf.h>
+#include "force-elision.h"
+
+#include "nptl/pthread_mutex_unlock.c"