From patchwork Thu Aug 28 17:58:27 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Samuel Thibault X-Patchwork-Id: 2575 Received: (qmail 15462 invoked by alias); 28 Aug 2014 17:58:36 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 15447 invoked by uid 89); 28 Aug 2014 17:58:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SUBJ_OBFU_PUNCT_FEW, SUBJ_OBFU_PUNCT_MANY autolearn=no version=3.3.2 X-HELO: toccata.ens-lyon.org Date: Thu, 28 Aug 2014 19:58:27 +0200 From: Samuel Thibault To: libc-alpha@sourceware.org, Andi Kleen Cc: mjw@redhat.com Subject: [PATCH,nptl] Fix lock elision of pthread_mutex_trylock vs unlock Message-ID: <20140828175827.GC3011@type.youpi.perso.aquilenet.fr> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21+34 (58baf7c9f32f) (2010-12-30) Hello, On hardware with RTM, the following crashes: #include 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 , 0xff, 0xff, 0xff, 0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3, 0x0 }, __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 + . */ + +#include +#include + +#include 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 + . */ + +#include +#include "force-elision.h" + +#include "nptl/pthread_mutex_unlock.c"