From patchwork Wed Apr 29 16:14:53 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 6487 Received: (qmail 79200 invoked by alias); 29 Apr 2015 16:15:05 -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 79160 invoked by uid 89); 29 Apr 2015 16:15:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Subject: Re: [PATCH] Fix lost wake-up when pthread_rwlock_timedrwlock times out. From: Torvald Riegel To: Joseph Myers Cc: GLIBC Devel In-Reply-To: References: <1429715209.17814.38.camel@triegel.csb> Date: Wed, 29 Apr 2015 18:14:53 +0200 Message-ID: <1430324093.4450.96.camel@triegel.csb> Mime-Version: 1.0 On Fri, 2015-04-24 at 17:53 +0000, Joseph Myers wrote: > On Wed, 22 Apr 2015, Torvald Riegel wrote: > > > If we set up a rwlock to prefer writers (and disallow recursive rdlock > > acquisitions), then readers will block for writers that are blocked to > > acquire the lock (otherwise, readers could constantly enter and exit, > > and the writer would never get the lock). However, the existing > > implementation did not wake such readers when the writer timed out. > > This patch adds the missing wake-up. > > There's no similar case for writers being blocked on readers. > > > > Tested on x86_64-linux. OK? > > > > 2015-04-22 Torvald Riegel > > > > * nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock): Add > > missing wake-up of readers. > > * nptl/tst-rwlock15.c: New file. > > * nptl/Makefile (tests): Add new test. > > If this was a bug that was user-visible in a release, there should be a > bug filed in Bugzilla for it and appropriate [BZ #N] used. > Thanks for the reminder, here's an updated version. I also added a small performance optimization. Tested on x86_64-linux. OK? 2015-04-28 Torvald Riegel [BZ #18324] * nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock): Add missing wake-up of readers. * nptl/tst-rwlock15.c: New file. * nptl/Makefile (tests): Add new test. commit 8ad628330ff2f712824eae4f903a5315e2ae60d3 Author: Torvald Riegel Date: Tue Apr 21 20:34:21 2015 +0200 Fix lost wake-up when pthread_rwlock_timedrwlock times out. diff --git a/nptl/Makefile b/nptl/Makefile index d784c8d..7e1897c 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -224,6 +224,7 @@ tests = tst-typesizes \ tst-rwlock1 tst-rwlock2 tst-rwlock2a tst-rwlock3 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-once1 tst-once2 tst-once3 tst-once4 \ 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_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c index 416f6e2..958d6db 100644 --- a/nptl/pthread_rwlock_timedwrlock.c +++ b/nptl/pthread_rwlock_timedwrlock.c @@ -32,6 +32,7 @@ pthread_rwlock_timedwrlock (rwlock, abstime) const struct timespec *abstime; { int result = 0; + int wake_readers = 0; /* Make sure we are alone. */ lll_lock (rwlock->__data.__lock, rwlock->__data.__shared); @@ -136,6 +137,17 @@ pthread_rwlock_timedwrlock (rwlock, abstime) if (err == -ETIMEDOUT) { result = ETIMEDOUT; + /* If we prefer writers, it can have happened that readers blocked + for us to acquire the lock first. If we have timed out, we need + to wake such readers if there are any, and if there is no writer + currently (otherwise, the writer will take care of wake-up). */ + if (!PTHREAD_RWLOCK_PREFER_READER_P (rwlock) + && (rwlock->__data.__nr_readers_queued > 0) + && (rwlock->__data.__writer == 0)) + { + ++rwlock->__data.__readers_wakeup; + wake_readers = 1; + } break; } } @@ -143,5 +155,10 @@ pthread_rwlock_timedwrlock (rwlock, abstime) /* We are done, free the lock. */ lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared); + /* Might be required after timeouts. */ + if (wake_readers) + lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX, + rwlock->__data.__shared); + return result; } diff --git a/nptl/tst-rwlock15.c b/nptl/tst-rwlock15.c new file mode 100644 index 0000000..b292701 --- /dev/null +++ b/nptl/tst-rwlock15.c @@ -0,0 +1,116 @@ +/* Copyright (C) 2015 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 + . */ + +/* This tests that a writer that is preferred -- but times out due to a + reader being present -- does not miss to wake other readers blocked on the + writer's pending lock acquisition. */ + +#include +#include +#include +#include +#include + +/* The bug existed in the code that strictly prefers writers over readers. */ +static pthread_rwlock_t r = PTHREAD_RWLOCK_WRITER_NONRECURSIVE_INITIALIZER_NP; + +static void * +writer (void *arg) +{ + struct timespec ts; + if (clock_gettime (CLOCK_REALTIME, &ts) != 0) + { + puts ("clock_gettime failed"); + exit (EXIT_FAILURE); + } + ts.tv_sec += 1; + int e = pthread_rwlock_timedwrlock (&r, &ts); + if (e != ETIMEDOUT) + { + puts ("timedwrlock did not time out"); + exit (EXIT_FAILURE); + } + return NULL; +} + +static void * +reader (void *arg) +{ + /* This isn't a reliable way to get the interleaving we need (because a + failed trylock doesn't synchronize with the writer, and because we could + try to lock after the writer has already timed out). However, both will + just lead to false positives. */ + int e; + while ((e = pthread_rwlock_tryrdlock (&r)) != EBUSY) + { + if (e != 0) + exit (EXIT_FAILURE); + pthread_rwlock_unlock (&r); + } + e = pthread_rwlock_rdlock (&r); + if (e != 0) + { + puts ("reader rdlock failed"); + exit (EXIT_FAILURE); + } + pthread_rwlock_unlock (&r); + return NULL; +} + + +static int +do_test (void) +{ + /* Grab a rdlock, then create a writer and a reader, and wait until they + finished. */ + + if (pthread_rwlock_rdlock (&r) != 0) + { + puts ("initial rdlock failed"); + return 1; + } + + pthread_t thw; + if (pthread_create (&thw, NULL, writer, NULL) != 0) + { + puts ("create failed"); + return 1; + } + pthread_t thr; + if (pthread_create (&thr, NULL, reader, NULL) != 0) + { + puts ("create failed"); + return 1; + } + + if (pthread_join (thw, NULL) != 0) + { + puts ("writer join failed"); + return 1; + } + if (pthread_join (thr, NULL) != 0) + { + puts ("reader join failed"); + return 1; + } + + return 0; +} + + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"