From patchwork Sat Mar 25 19:49:01 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 743504 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 3vr9rm5Sh7z9s73 for ; Sun, 26 Mar 2017 06:51:04 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="NQG/4tvQ"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:subject:from:to:cc:content-type :date:mime-version; q=dns; s=default; b=sA+fViy4cXiwDThDIhz/tYYV 10DNfuRvxT5h57pYVQygHqQoH5Q20zS247VTT/enLjibmhFEUYSXzt7Hb9/3sVN5 Q6boJLg9yODCmVdtHEXJYibaI0QRtaVKyydCroO6Lj8UQOZ+PpuakVDNfF4m3qWd Q3Uv+UbTkApK97A86ZY= 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:message-id:subject:from:to:cc:content-type :date:mime-version; s=default; bh=oKoGhR9BYpH+6iV+lHM47yOsHCs=; b= NQG/4tvQEBvTeXCggMqSyHfm6GJG+UCy84sgonw5dUkA323mK0jUbDcyTETh1CPf VWzaoQbRIIsf/MDZOMLzD3A5HdfVt4g3X8DMAc425tW5mQ6BW8rdbLZj+JWvjHqC TyMyw5oTEyIe7LlBSZK6b/TJL7nKk8dncxAUHpZON0k= Received: (qmail 36818 invoked by alias); 25 Mar 2017 19:50:57 -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 36805 invoked by uid 89); 25 Mar 2017 19:50:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=acquired, handing, up-to-date, uptodate X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3250FC04B316 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=triegel@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 3250FC04B316 Message-ID: <1490471341.26906.366.camel@redhat.com> Subject: [PATCH] rwlock: Fix explicit hand-over. From: Torvald Riegel To: GLIBC Devel Cc: Waiman Long , Carlos O'Donell Date: Sat, 25 Mar 2017 20:49:01 +0100 Mime-Version: 1.0 This patch fixes a bug in the new rwlock's detection of when explicit hand-over has to happen (see the description of the rwlock for what this is). Without this patch, the rwlock can fail to execute the explicit hand-over in certain cases (e.g., empty critical sections that switch quickly between read and write phases). This can then lead to errors in how __wrphase_futex is accessed, which in turn can lead to deadlocks. This bug was reported by Waiman Long. (The testcase in the patch is a reduced form of your benchmark. It would be nice if you could test this too with your benchmark/machine). Tested on x86_64. The new test triggers the bug with the unfixed rwlock, but not always. We could make the duration of the test longer to increase the likelihood of the test triggering the bug, but this would increase the testsuite runtime, so I'm not sure whether this is worth it. [BZ #21298] * nptl/pthread_rwlock_common.c (__pthread_rwlock_rdlock_full): Fix explicit hand-over. (__pthread_rwlock_wrlock_full): Likewise. * nptl/tst-rwlock20.c: New file. * nptl/Makefile: Add new test. commit da1a4b2807e489f199aa966050fe0b7a7a82a856 Author: Torvald Riegel Date: Sat Mar 25 00:36:46 2017 +0100 rwlock: Fix explicit hand-over. Without this patch, the rwlock can fail to execute the explicit hand-over in certain cases (e.g., empty critical sections that switch quickly between read and write phases). This can then lead to errors in how __wrphase_futex is accessed, which in turn can lead to deadlocks. [BZ #21298] * nptl/pthread_rwlock_common.c (__pthread_rwlock_rdlock_full): Fix explicit hand-over. (__pthread_rwlock_wrlock_full): Likewise. * nptl/tst-rwlock20.c: New file. * nptl/Makefile: Add new test. diff --git a/nptl/Makefile b/nptl/Makefile index 6d48c0c..0cc2873 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -241,7 +241,7 @@ tests = tst-typesizes \ tst-rwlock4 tst-rwlock5 tst-rwlock6 tst-rwlock7 tst-rwlock8 \ tst-rwlock9 tst-rwlock10 tst-rwlock11 tst-rwlock12 tst-rwlock13 \ tst-rwlock14 tst-rwlock15 tst-rwlock16 tst-rwlock17 tst-rwlock18 \ - tst-rwlock19 \ + tst-rwlock19 tst-rwlock20 \ tst-once1 tst-once2 tst-once3 tst-once4 tst-once5 \ tst-key1 tst-key2 tst-key3 tst-key4 \ tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \ diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c index 256508c..86516e5 100644 --- a/nptl/pthread_rwlock_common.c +++ b/nptl/pthread_rwlock_common.c @@ -372,9 +372,17 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, cannot have acquired the lock previously as a reader (which could result in deadlock if we would wait for the primary writer to run). However, this seems to be a corner case and handling it specially not be worth the - complexity. */ + complexity. + Otherwise, if we were in a write phase, we save this fact so that it is + not lost when we overwrite r when trying to install a read phase (see + below; those attempts could observe a read phase that has been started + by a writer in the meantime, which in turn could result in incorrectly + skipping explicit hand-over). */ + bool registered_while_in_write_phase = false; if (__glibc_likely ((r & PTHREAD_RWLOCK_WRPHASE) == 0)) return 0; + else + registered_while_in_write_phase = true; /* If there is no primary writer but we are in a write phase, we can try to install a read phase ourself. */ @@ -390,15 +398,17 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, { /* We started the read phase, so we are also responsible for updating the write-phase futex. Relaxed MO is sufficient. - Note that there can be no other reader that we have to wake - because all other readers will see the read phase started by us - (or they will try to start it themselves); if a writer started - the read phase, we cannot have started it. Furthermore, we - cannot discard a PTHREAD_RWLOCK_FUTEX_USED flag because we will - overwrite the value set by the most recent writer (or the readers - before it in case of explicit hand-over) and we know that there - are no waiting readers. */ - atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0); + We have to do the same steps as a writer would when handing + over the read phase to us because other readers cannot + distinguish between us and the writer. Therefore, we must + take the same steps as a writer would, which includes + potentially having to wake other readers. */ + if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0) + & PTHREAD_RWLOCK_FUTEX_USED) != 0) + { + int private = __pthread_rwlock_get_private (rwlock); + futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private); + } return 0; } else @@ -407,11 +417,11 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock, } } - if ((r & PTHREAD_RWLOCK_WRPHASE) != 0) + if (registered_while_in_write_phase) { - /* We are in a write phase, and there must be a primary writer because - of the previous loop. Block until the primary writer gives up the - write phase. This case requires explicit hand-over using + /* We are or were in a write phase, and there must be a primary writer + because of the previous loop. Block until the primary writer gives + up the write phase. This case requires explicit hand-over using __wrphase_futex. However, __wrphase_futex might not have been set to 1 yet (either because explicit hand-over to the writer is still ongoing, or because @@ -741,10 +751,17 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, r = atomic_load_relaxed (&rwlock->__data.__readers); } /* Our snapshot of __readers is up-to-date at this point because we - either set WRLOCKED using a CAS or were handed over WRLOCKED from + either set WRLOCKED using a CAS (and update r accordingly below, + which was used as expected value for the CAS) or got WRLOCKED from another writer whose snapshot of __readers we inherit. */ + r |= PTHREAD_RWLOCK_WRLOCKED; } + /* Save whether we got wrlocked while being in a read phase so that this + fact isn't lost when we reuse r when trying to install a write phase + below. Also see __pthread_rwlock_rdlock_full. */ + bool got_wrlocked_while_in_read_phase = (r & PTHREAD_RWLOCK_WRPHASE) == 0; + /* If we are in a read phase and there are no readers, try to start a write phase. */ while (((r & PTHREAD_RWLOCK_WRPHASE) == 0) @@ -758,10 +775,13 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, &r, r | PTHREAD_RWLOCK_WRPHASE)) { /* We have started a write phase, so need to enable readers to wait. - See the similar case in__pthread_rwlock_rdlock_full. */ + See the similar case in __pthread_rwlock_rdlock_full. Unlike in + that similar case, we are the (only) primary writer and so do + not need to wake another writer. */ atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1); + /* Make sure we fall through to the end of the function. */ - r |= PTHREAD_RWLOCK_WRPHASE; + got_wrlocked_while_in_read_phase = false; break; } /* TODO Back-off. */ @@ -774,7 +794,7 @@ __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock, atomic_store_relaxed (&rwlock->__data.__writers_futex, 1 | (may_share_futex_used_flag ? PTHREAD_RWLOCK_FUTEX_USED : 0)); - if (__glibc_unlikely ((r & PTHREAD_RWLOCK_WRPHASE) == 0)) + if (__glibc_unlikely (got_wrlocked_while_in_read_phase)) { /* We are not in a read phase and there are readers (because of the previous loop). Thus, we have to wait for explicit hand-over from diff --git a/nptl/tst-rwlock20.c b/nptl/tst-rwlock20.c new file mode 100644 index 0000000..fc636e3 --- /dev/null +++ b/nptl/tst-rwlock20.c @@ -0,0 +1,128 @@ +/* Test program for a read-phase / write-phase explicit hand-over. + Copyright (C) 2000-2017 Free Software Foundation, Inc. + + 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; see the file COPYING.LIB. If + not, see . */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define THREADS 2 + +#define KIND PTHREAD_RWLOCK_PREFER_READER_NP + +static pthread_rwlock_t lock; +static int done = 0; + +static void* +tf (void* arg) +{ + while (atomic_load_relaxed (&done) == 0) + { + int rcnt = 0; + int wcnt = 100; + if ((uintptr_t) arg == 0) + { + rcnt = 1; + wcnt = 1; + } + + do + { + if (wcnt) + { + pthread_rwlock_wrlock (&lock); + pthread_rwlock_unlock (&lock); + wcnt--; + } + if (rcnt) + { + pthread_rwlock_rdlock (&lock); + pthread_rwlock_unlock (&lock); + rcnt--; + } + } + while ((atomic_load_relaxed (&done) == 0) && (rcnt + wcnt > 0)); + + } + return NULL; +} + + + +static int +do_test (void) +{ + pthread_t thr[THREADS]; + int n; + void *res; + pthread_rwlockattr_t a; + + if (pthread_rwlockattr_init (&a) != 0) + { + puts ("rwlockattr_t failed"); + exit (1); + } + + if (pthread_rwlockattr_setkind_np (&a, KIND) != 0) + { + puts ("rwlockattr_setkind failed"); + exit (1); + } + + if (pthread_rwlock_init (&lock, &a) != 0) + { + puts ("rwlock_init failed"); + exit (1); + } + + /* Make standard error the same as standard output. */ + dup2 (1, 2); + + /* Make sure we see all message, even those on stdout. */ + setvbuf (stdout, NULL, _IONBF, 0); + + for (n = 0; n < THREADS; ++n) + if (pthread_create (&thr[n], NULL, tf, (void *) (uintptr_t) n) != 0) + { + puts ("thread create failed"); + exit (1); + } + struct timespec delay; + delay.tv_sec = 10; + delay.tv_nsec = 0; + nanosleep (&delay, NULL); + atomic_store_relaxed (&done, 1); + + /* Wait for all the threads. */ + for (n = 0; n < THREADS; ++n) + if (pthread_join (thr[n], &res) != 0) + { + puts ("writer join failed"); + exit (1); + } + + return 0; +} + +#define TIMEOUT 15 +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"