This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix lost wake-up when pthread_rwlock_timedrwlock times out.
- From: Torvald Riegel <triegel at redhat dot com>
- To: Joseph Myers <joseph at codesourcery dot com>
- Cc: GLIBC Devel <libc-alpha at sourceware dot org>
- Date: Wed, 29 Apr 2015 18:14:53 +0200
- Subject: Re: [PATCH] Fix lost wake-up when pthread_rwlock_timedrwlock times out.
- Authentication-results: sourceware.org; auth=none
- References: <1429715209 dot 17814 dot 38 dot camel at triegel dot csb> <alpine dot DEB dot 2 dot 10 dot 1504241752410 dot 30193 at digraph dot polyomino dot org dot uk>
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 <triegel@redhat.com>
> >
> > * 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 <triegel@redhat.com>
[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 <triegel@redhat.com>
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
+ <http://www.gnu.org/licenses/>. */
+
+/* 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 <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+
+/* 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"