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: 383999 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 5F9921400E9 for ; Fri, 29 Aug 2014 03:58:43 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; q=dns; s=default; b=sN+20aTb81sKMksk nCLdAJ1Bu7/AoXcZ9XEcEEqZahBwwmlmiTeobjQYqNXOElKXUWzzaO6p+YQSVYhc 7075dZ5We0TwVeXjLCaCEoOmCSyBSXwTmlLAAC3pKqBd6y9G87CAeA88k50WfwH8 OOhzr9DqO+rPeavsVCroDUf12Hk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; s=default; bh=EXsbMLCgg4BCIxd+1XHtHk WEAHE=; b=oO40hvPzlKgx9xSFMqetIhmYggxEgMu117g0GDIhn7HKsbsb0hDiCS ZDfm0Z3TrTQvQkcssKXqiHvffmTLbCJ8hMWcHqSl5f0+Plcpv10hhTZIjAiy7R3Q mHNk1urVY7mAfBOfOe4tr1tLxlSMsUhhsPgnGoj5No0KIIv1K38ew= 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"