Fix missing wake-ups in pthread_rwlock_rdlock.
Commit Message
This adds wake-ups that would be missing if assuming that for a
non-writer-preferring rwlock, if one thread has acquired a rdlock and
does not release it, another thread will eventually acquire a rdlock too
despite concurrent write lock acquisition attempts. BZ 14958 is about
supporting this assumption.
I've commented on the BZ why I think that strictly speaking, this isn't
a valid test case, but nonetheless worth supporting:
https://sourceware.org/bugzilla/show_bug.cgi?id=14958#c7
I have no intention to work on supporting the requirements about Thread
Execution Scheduling and priorities for pthread_rwlock_rdlock (see BZ
13701); we claim to support it, so with this patch, we can claim that we
assume assume the priorities to all be equal (in which case readers are
to be preferred). See
http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html
Tested on x86_64-linux. (Applies on top of the other rwlock fix I sent.)
OK?
2015-04-28 Torvald Riegel <triegel@redhat.com>
[BZ #14958]
* nptl/pthread_rwlock_rdlock.c (__pthread_rwlock_rdlock): Add missing
wake-up.
(__pthread_rwlock_rdlock_slow): Likewise.
* nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
Likewise.
* nptl/pthread_rwlock_tryrdlock.c (__pthread_rwlock_tryrdlock):
Likewise.
* nptl/pthread_rwlock_unlock.c (__pthread_rwlock_unlock): Add comments.
* nptl/tst-rwlock16.c: New file.
* nptl/Makefile (tests): Add new test.
Comments
Ping.
On Wed, 2015-04-29 at 18:25 +0200, Torvald Riegel wrote:
> This adds wake-ups that would be missing if assuming that for a
> non-writer-preferring rwlock, if one thread has acquired a rdlock and
> does not release it, another thread will eventually acquire a rdlock too
> despite concurrent write lock acquisition attempts. BZ 14958 is about
> supporting this assumption.
>
> I've commented on the BZ why I think that strictly speaking, this isn't
> a valid test case, but nonetheless worth supporting:
> https://sourceware.org/bugzilla/show_bug.cgi?id=14958#c7
>
> I have no intention to work on supporting the requirements about Thread
> Execution Scheduling and priorities for pthread_rwlock_rdlock (see BZ
> 13701); we claim to support it, so with this patch, we can claim that we
> assume assume the priorities to all be equal (in which case readers are
> to be preferred). See
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html
>
> Tested on x86_64-linux. (Applies on top of the other rwlock fix I sent.)
> OK?
>
> 2015-04-28 Torvald Riegel <triegel@redhat.com>
>
> [BZ #14958]
> * nptl/pthread_rwlock_rdlock.c (__pthread_rwlock_rdlock): Add missing
> wake-up.
> (__pthread_rwlock_rdlock_slow): Likewise.
> * nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
> Likewise.
> * nptl/pthread_rwlock_tryrdlock.c (__pthread_rwlock_tryrdlock):
> Likewise.
> * nptl/pthread_rwlock_unlock.c (__pthread_rwlock_unlock): Add comments.
> * nptl/tst-rwlock16.c: New file.
> * nptl/Makefile (tests): Add new test.
I intend to commit this end of this week or some time next week unless I
hear objections or promises of a future review.
On Wed, 2015-05-13 at 11:10 +0200, Torvald Riegel wrote:
> Ping.
>
> On Wed, 2015-04-29 at 18:25 +0200, Torvald Riegel wrote:
> > This adds wake-ups that would be missing if assuming that for a
> > non-writer-preferring rwlock, if one thread has acquired a rdlock and
> > does not release it, another thread will eventually acquire a rdlock too
> > despite concurrent write lock acquisition attempts. BZ 14958 is about
> > supporting this assumption.
> >
> > I've commented on the BZ why I think that strictly speaking, this isn't
> > a valid test case, but nonetheless worth supporting:
> > https://sourceware.org/bugzilla/show_bug.cgi?id=14958#c7
> >
> > I have no intention to work on supporting the requirements about Thread
> > Execution Scheduling and priorities for pthread_rwlock_rdlock (see BZ
> > 13701); we claim to support it, so with this patch, we can claim that we
> > assume assume the priorities to all be equal (in which case readers are
> > to be preferred). See
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html
> >
> > Tested on x86_64-linux. (Applies on top of the other rwlock fix I sent.)
> > OK?
> >
> > 2015-04-28 Torvald Riegel <triegel@redhat.com>
> >
> > [BZ #14958]
> > * nptl/pthread_rwlock_rdlock.c (__pthread_rwlock_rdlock): Add missing
> > wake-up.
> > (__pthread_rwlock_rdlock_slow): Likewise.
> > * nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
> > Likewise.
> > * nptl/pthread_rwlock_tryrdlock.c (__pthread_rwlock_tryrdlock):
> > Likewise.
> > * nptl/pthread_rwlock_unlock.c (__pthread_rwlock_unlock): Add comments.
> > * nptl/tst-rwlock16.c: New file.
> > * nptl/Makefile (tests): Add new test.
>
>
>
Looks good to me barring a couple of minor nits I've noted below that
you can fix up before committing.
Siddhesh
On Wed, Apr 29, 2015 at 06:25:26PM +0200, Torvald Riegel wrote:
> commit ba34831e62b6041da4f49a2816ea5cbde270332c
> Author: Torvald Riegel <triegel@redhat.com>
> Date: Tue Apr 28 23:24:36 2015 +0200
>
> Fix missing wake-ups in pthread_rwlock_rdlock.
>
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 7e1897c..422dfb8 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -224,7 +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-rwlock15 tst-rwlock16 \
> 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_rdlock.c b/nptl/pthread_rwlock_rdlock.c
> index 0edca65..563e841 100644
> --- a/nptl/pthread_rwlock_rdlock.c
> +++ b/nptl/pthread_rwlock_rdlock.c
> @@ -30,6 +30,7 @@ static int __attribute__((noinline))
> __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
> {
> int result = 0;
> + int wake = 0;
Make this bool.
>
> /* Lock is taken in caller. */
>
> @@ -81,7 +82,17 @@ __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
> result = EAGAIN;
> }
> else
> - LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> + {
> + LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> + /* See pthread_rwlock_rdlock. */
> + if (rwlock->__data.__nr_readers == 1
> + && rwlock->__data.__nr_readers_queued > 0
> + && rwlock->__data.__nr_writers_queued > 0)
> + {
> + ++rwlock->__data.__readers_wakeup;
> + wake = 1;
> + }
> + }
>
> break;
> }
> @@ -90,6 +101,10 @@ __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
> /* We are done, free the lock. */
> lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>
> + if (wake)
> + lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> + rwlock->__data.__shared);
> +
> return result;
> }
>
> @@ -100,6 +115,7 @@ int
> __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock)
> {
> int result = 0;
> + int wake = 0;
>
Likewise.
> LIBC_PROBE (rdlock_entry, 1, rwlock);
>
> @@ -126,11 +142,30 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock)
> result = EAGAIN;
> }
> else
> - LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> + {
> + LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> + /* If we are the first reader, and there are blocked readers and
> + writers (which we don't prefer, see above), then it can be the
> + case that we stole the lock from a writer that was already woken
> + to acquire it. That means that we need to take over the writer's
> + responsibility to wake all readers (see pthread_rwlock_unlock).
> + Thus, wake all readers in this case. */
> + if (rwlock->__data.__nr_readers == 1
> + && rwlock->__data.__nr_readers_queued > 0
> + && rwlock->__data.__nr_writers_queued > 0)
> + {
> + ++rwlock->__data.__readers_wakeup;
> + wake = 1;
> + }
> + }
>
> /* We are done, free the lock. */
> lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>
> + if (wake)
> + lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> + rwlock->__data.__shared);
> +
> return result;
> }
>
> diff --git a/nptl/pthread_rwlock_timedrdlock.c b/nptl/pthread_rwlock_timedrdlock.c
> index 0212b49..f984103 100644
> --- a/nptl/pthread_rwlock_timedrdlock.c
> +++ b/nptl/pthread_rwlock_timedrdlock.c
> @@ -32,6 +32,7 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
> const struct timespec *abstime;
> {
> int result = 0;
> + int wake = 0;
>
Likewise.
> /* Make sure we are alone. */
> lll_lock(rwlock->__data.__lock, rwlock->__data.__shared);
> @@ -53,6 +54,17 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
> --rwlock->__data.__nr_readers;
> result = EAGAIN;
> }
> + else
> + {
> + /* See pthread_rwlock_rdlock. */
> + if (rwlock->__data.__nr_readers == 1
> + && rwlock->__data.__nr_readers_queued > 0
> + && rwlock->__data.__nr_writers_queued > 0)
> + {
> + ++rwlock->__data.__readers_wakeup;
> + wake = 1;
> + }
> + }
>
> break;
> }
> @@ -153,5 +165,9 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
> /* We are done, free the lock. */
> lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>
> + if (wake)
> + lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> + rwlock->__data.__shared);
> +
> return result;
> }
> diff --git a/nptl/pthread_rwlock_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c
> index 958d6db..2f71ea2 100644
> --- a/nptl/pthread_rwlock_timedwrlock.c
> +++ b/nptl/pthread_rwlock_timedwrlock.c
> @@ -140,8 +140,16 @@ pthread_rwlock_timedwrlock (rwlock, abstime)
> /* 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)
> + currently (otherwise, the writer will take care of wake-up).
> + Likewise, even if we prefer readers, we can be responsible for
> + wake-up (see pthread_rwlock_unlock) if no reader or writer has
> + acquired the lock. We have timed out and thus not consumed a
> + futex wake-up; therefore, if there is no other blocked writer
> + that would consume the wake-up and thus take over responsibility,
> + we need to wake blocked readers. */
> + if ((!PTHREAD_RWLOCK_PREFER_READER_P (rwlock)
> + || ((rwlock->__data.__nr_readers == 0)
> + && (rwlock->__data.__nr_writers_queued == 0)))
Indentation of the conditional is incorrect.
> && (rwlock->__data.__nr_readers_queued > 0)
> && (rwlock->__data.__writer == 0))
> {
> diff --git a/nptl/pthread_rwlock_tryrdlock.c b/nptl/pthread_rwlock_tryrdlock.c
> index d51d9aa..1127b35 100644
> --- a/nptl/pthread_rwlock_tryrdlock.c
> +++ b/nptl/pthread_rwlock_tryrdlock.c
> @@ -26,6 +26,7 @@ int
> __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
> {
> int result = EBUSY;
> + int wake = 0;
Make this bool.
>
> if (ELIDE_TRYLOCK (rwlock->__data.__rwelision,
> rwlock->__data.__lock == 0
> @@ -45,11 +46,25 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
> result = EAGAIN;
> }
> else
> - result = 0;
> + {
> + result = 0;
> + /* See pthread_rwlock_rdlock. */
> + if (rwlock->__data.__nr_readers == 1
> + && rwlock->__data.__nr_readers_queued > 0
> + && rwlock->__data.__nr_writers_queued > 0)
> + {
> + ++rwlock->__data.__readers_wakeup;
> + wake = 1;
> + }
> + }
> }
>
> lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>
> + if (wake)
> + lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> + rwlock->__data.__shared);
> +
> return result;
> }
> strong_alias (__pthread_rwlock_tryrdlock, pthread_rwlock_tryrdlock)
> diff --git a/nptl/pthread_rwlock_unlock.c b/nptl/pthread_rwlock_unlock.c
> index b8336f6..d2ad4b0 100644
> --- a/nptl/pthread_rwlock_unlock.c
> +++ b/nptl/pthread_rwlock_unlock.c
> @@ -40,8 +40,13 @@ __pthread_rwlock_unlock (pthread_rwlock_t *rwlock)
> rwlock->__data.__writer = 0;
> else
> --rwlock->__data.__nr_readers;
> + /* If there are still readers present, we do not yet need to wake writers
> + nor are responsible to wake any readers. */
> if (rwlock->__data.__nr_readers == 0)
> {
> + /* Note that if there is a blocked writer, we effectively make it
> + responsible for waking any readers because we don't wake readers in
> + this case. */
> if (rwlock->__data.__nr_writers_queued)
> {
> ++rwlock->__data.__writer_wakeup;
> diff --git a/nptl/tst-rwlock16.c b/nptl/tst-rwlock16.c
> new file mode 100644
> index 0000000..8e661db
> --- /dev/null
> +++ b/nptl/tst-rwlock16.c
> @@ -0,0 +1,183 @@
> +/* 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 with a reader-preferring rwlock, all readers are woken if
> + one reader "steals" lock ownership from a blocked writer. */
> +
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <semaphore.h>
> +#include <unistd.h>
> +
> +/* If we strictly prefer writers over readers, a program must not expect
> + that, in the presence of concurrent writers, one reader will also acquire
> + the lock when another reader has already done so. Thus, use the
> + default rwlock type that does not strictly prefer writers. */
> +static pthread_rwlock_t r = PTHREAD_RWLOCK_INITIALIZER;
> +
> +static pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t cv = PTHREAD_COND_INITIALIZER;
> +
> +/* Avoid using glibc-internal atomic operations. */
> +static sem_t stop;
> +static int consumer_stop = 0;
> +
> +static void *
> +writer (void *arg)
> +{
> + int s;
> +
> + do
> + {
> + if (pthread_rwlock_wrlock (&r) != 0)
> + {
> + puts ("wrlock failed");
> + exit (EXIT_FAILURE);
> + }
> + if (pthread_rwlock_unlock (&r) != 0)
> + {
> + puts ("unlock failed");
> + exit (EXIT_FAILURE);
> + }
> + sem_getvalue (&stop, &s);
> + }
> + while (s == 0);
> + return NULL;
> +}
> +
> +static void *
> +reader_producer (void *arg)
> +{
> + int s;
> +
> + do
> + {
> + if (pthread_rwlock_rdlock (&r) != 0)
> + {
> + puts ("rdlock reader failed");
> + exit (EXIT_FAILURE);
> + }
> +
> + sem_getvalue (&stop, &s);
> +
> + pthread_mutex_lock (&m);
> + if (s != 0)
> + consumer_stop = 1;
> + pthread_cond_signal (&cv);
> + pthread_mutex_unlock (&m);
> +
> + if (pthread_rwlock_unlock (&r) != 0)
> + {
> + puts ("unlock reader failed");
> + exit (EXIT_FAILURE);
> + }
> + }
> + while (s == 0);
> + puts ("producer finished");
> + return NULL;
> +}
> +
> +static void *
> +reader_consumer (void *arg)
> +{
> + int s;
> +
> + do
> + {
> + if (pthread_rwlock_rdlock (&r) != 0)
> + {
> + puts ("rdlock reader failed");
> + exit (EXIT_FAILURE);
> + }
> +
> + pthread_mutex_lock (&m);
> + s = consumer_stop;
> + if (s == 0)
> + pthread_cond_wait (&cv, &m);
> + pthread_mutex_unlock (&m);
> +
> + if (pthread_rwlock_unlock (&r) != 0)
> + {
> + puts ("unlock reader failed");
> + exit (EXIT_FAILURE);
> + }
> + }
> + while (s == 0);
> + puts ("consumer finished");
> + return NULL;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> + pthread_t w1, w2, rp, rc;
> +
> + if (pthread_create (&w1, NULL, writer, NULL) != 0)
> + {
> + puts ("create failed");
> + return 1;
> + }
> + if (pthread_create (&w2, NULL, writer, NULL) != 0)
> + {
> + puts ("create failed");
> + return 1;
> + }
> + if (pthread_create (&rc, NULL, reader_consumer, NULL) != 0)
> + {
> + puts ("create failed");
> + return 1;
> + }
> + if (pthread_create (&rp, NULL, reader_producer, NULL) != 0)
> + {
> + puts ("create failed");
> + return 1;
> + }
> +
> + sleep (2);
> + sem_post (&stop);
> +
> + if (pthread_join (w1, NULL) != 0)
> + {
> + puts ("w1 join failed");
> + return 1;
> + }
> + if (pthread_join (w2, NULL) != 0)
> + {
> + puts ("w2 join failed");
> + return 1;
> + }
> + if (pthread_join (rp, NULL) != 0)
> + {
> + puts ("reader_producer join failed");
> + return 1;
> + }
> + if (pthread_join (rc, NULL) != 0)
> + {
> + puts ("reader_consumer join failed");
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +
> +#define TIMEOUT 3
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
commit ba34831e62b6041da4f49a2816ea5cbde270332c
Author: Torvald Riegel <triegel@redhat.com>
Date: Tue Apr 28 23:24:36 2015 +0200
Fix missing wake-ups in pthread_rwlock_rdlock.
@@ -224,7 +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-rwlock15 tst-rwlock16 \
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 \
@@ -30,6 +30,7 @@ static int __attribute__((noinline))
__pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
{
int result = 0;
+ int wake = 0;
/* Lock is taken in caller. */
@@ -81,7 +82,17 @@ __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
result = EAGAIN;
}
else
- LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
+ {
+ LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
+ /* See pthread_rwlock_rdlock. */
+ if (rwlock->__data.__nr_readers == 1
+ && rwlock->__data.__nr_readers_queued > 0
+ && rwlock->__data.__nr_writers_queued > 0)
+ {
+ ++rwlock->__data.__readers_wakeup;
+ wake = 1;
+ }
+ }
break;
}
@@ -90,6 +101,10 @@ __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
/* We are done, free the lock. */
lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
+ if (wake)
+ lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
+ rwlock->__data.__shared);
+
return result;
}
@@ -100,6 +115,7 @@ int
__pthread_rwlock_rdlock (pthread_rwlock_t *rwlock)
{
int result = 0;
+ int wake = 0;
LIBC_PROBE (rdlock_entry, 1, rwlock);
@@ -126,11 +142,30 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock)
result = EAGAIN;
}
else
- LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
+ {
+ LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
+ /* If we are the first reader, and there are blocked readers and
+ writers (which we don't prefer, see above), then it can be the
+ case that we stole the lock from a writer that was already woken
+ to acquire it. That means that we need to take over the writer's
+ responsibility to wake all readers (see pthread_rwlock_unlock).
+ Thus, wake all readers in this case. */
+ if (rwlock->__data.__nr_readers == 1
+ && rwlock->__data.__nr_readers_queued > 0
+ && rwlock->__data.__nr_writers_queued > 0)
+ {
+ ++rwlock->__data.__readers_wakeup;
+ wake = 1;
+ }
+ }
/* We are done, free the lock. */
lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
+ if (wake)
+ lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
+ rwlock->__data.__shared);
+
return result;
}
@@ -32,6 +32,7 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
const struct timespec *abstime;
{
int result = 0;
+ int wake = 0;
/* Make sure we are alone. */
lll_lock(rwlock->__data.__lock, rwlock->__data.__shared);
@@ -53,6 +54,17 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
--rwlock->__data.__nr_readers;
result = EAGAIN;
}
+ else
+ {
+ /* See pthread_rwlock_rdlock. */
+ if (rwlock->__data.__nr_readers == 1
+ && rwlock->__data.__nr_readers_queued > 0
+ && rwlock->__data.__nr_writers_queued > 0)
+ {
+ ++rwlock->__data.__readers_wakeup;
+ wake = 1;
+ }
+ }
break;
}
@@ -153,5 +165,9 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
/* We are done, free the lock. */
lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
+ if (wake)
+ lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
+ rwlock->__data.__shared);
+
return result;
}
@@ -140,8 +140,16 @@ pthread_rwlock_timedwrlock (rwlock, abstime)
/* 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)
+ currently (otherwise, the writer will take care of wake-up).
+ Likewise, even if we prefer readers, we can be responsible for
+ wake-up (see pthread_rwlock_unlock) if no reader or writer has
+ acquired the lock. We have timed out and thus not consumed a
+ futex wake-up; therefore, if there is no other blocked writer
+ that would consume the wake-up and thus take over responsibility,
+ we need to wake blocked readers. */
+ if ((!PTHREAD_RWLOCK_PREFER_READER_P (rwlock)
+ || ((rwlock->__data.__nr_readers == 0)
+ && (rwlock->__data.__nr_writers_queued == 0)))
&& (rwlock->__data.__nr_readers_queued > 0)
&& (rwlock->__data.__writer == 0))
{
@@ -26,6 +26,7 @@ int
__pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
{
int result = EBUSY;
+ int wake = 0;
if (ELIDE_TRYLOCK (rwlock->__data.__rwelision,
rwlock->__data.__lock == 0
@@ -45,11 +46,25 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
result = EAGAIN;
}
else
- result = 0;
+ {
+ result = 0;
+ /* See pthread_rwlock_rdlock. */
+ if (rwlock->__data.__nr_readers == 1
+ && rwlock->__data.__nr_readers_queued > 0
+ && rwlock->__data.__nr_writers_queued > 0)
+ {
+ ++rwlock->__data.__readers_wakeup;
+ wake = 1;
+ }
+ }
}
lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
+ if (wake)
+ lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
+ rwlock->__data.__shared);
+
return result;
}
strong_alias (__pthread_rwlock_tryrdlock, pthread_rwlock_tryrdlock)
@@ -40,8 +40,13 @@ __pthread_rwlock_unlock (pthread_rwlock_t *rwlock)
rwlock->__data.__writer = 0;
else
--rwlock->__data.__nr_readers;
+ /* If there are still readers present, we do not yet need to wake writers
+ nor are responsible to wake any readers. */
if (rwlock->__data.__nr_readers == 0)
{
+ /* Note that if there is a blocked writer, we effectively make it
+ responsible for waking any readers because we don't wake readers in
+ this case. */
if (rwlock->__data.__nr_writers_queued)
{
++rwlock->__data.__writer_wakeup;
new file mode 100644
@@ -0,0 +1,183 @@
+/* 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 with a reader-preferring rwlock, all readers are woken if
+ one reader "steals" lock ownership from a blocked writer. */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <semaphore.h>
+#include <unistd.h>
+
+/* If we strictly prefer writers over readers, a program must not expect
+ that, in the presence of concurrent writers, one reader will also acquire
+ the lock when another reader has already done so. Thus, use the
+ default rwlock type that does not strictly prefer writers. */
+static pthread_rwlock_t r = PTHREAD_RWLOCK_INITIALIZER;
+
+static pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t cv = PTHREAD_COND_INITIALIZER;
+
+/* Avoid using glibc-internal atomic operations. */
+static sem_t stop;
+static int consumer_stop = 0;
+
+static void *
+writer (void *arg)
+{
+ int s;
+
+ do
+ {
+ if (pthread_rwlock_wrlock (&r) != 0)
+ {
+ puts ("wrlock failed");
+ exit (EXIT_FAILURE);
+ }
+ if (pthread_rwlock_unlock (&r) != 0)
+ {
+ puts ("unlock failed");
+ exit (EXIT_FAILURE);
+ }
+ sem_getvalue (&stop, &s);
+ }
+ while (s == 0);
+ return NULL;
+}
+
+static void *
+reader_producer (void *arg)
+{
+ int s;
+
+ do
+ {
+ if (pthread_rwlock_rdlock (&r) != 0)
+ {
+ puts ("rdlock reader failed");
+ exit (EXIT_FAILURE);
+ }
+
+ sem_getvalue (&stop, &s);
+
+ pthread_mutex_lock (&m);
+ if (s != 0)
+ consumer_stop = 1;
+ pthread_cond_signal (&cv);
+ pthread_mutex_unlock (&m);
+
+ if (pthread_rwlock_unlock (&r) != 0)
+ {
+ puts ("unlock reader failed");
+ exit (EXIT_FAILURE);
+ }
+ }
+ while (s == 0);
+ puts ("producer finished");
+ return NULL;
+}
+
+static void *
+reader_consumer (void *arg)
+{
+ int s;
+
+ do
+ {
+ if (pthread_rwlock_rdlock (&r) != 0)
+ {
+ puts ("rdlock reader failed");
+ exit (EXIT_FAILURE);
+ }
+
+ pthread_mutex_lock (&m);
+ s = consumer_stop;
+ if (s == 0)
+ pthread_cond_wait (&cv, &m);
+ pthread_mutex_unlock (&m);
+
+ if (pthread_rwlock_unlock (&r) != 0)
+ {
+ puts ("unlock reader failed");
+ exit (EXIT_FAILURE);
+ }
+ }
+ while (s == 0);
+ puts ("consumer finished");
+ return NULL;
+}
+
+
+static int
+do_test (void)
+{
+ pthread_t w1, w2, rp, rc;
+
+ if (pthread_create (&w1, NULL, writer, NULL) != 0)
+ {
+ puts ("create failed");
+ return 1;
+ }
+ if (pthread_create (&w2, NULL, writer, NULL) != 0)
+ {
+ puts ("create failed");
+ return 1;
+ }
+ if (pthread_create (&rc, NULL, reader_consumer, NULL) != 0)
+ {
+ puts ("create failed");
+ return 1;
+ }
+ if (pthread_create (&rp, NULL, reader_producer, NULL) != 0)
+ {
+ puts ("create failed");
+ return 1;
+ }
+
+ sleep (2);
+ sem_post (&stop);
+
+ if (pthread_join (w1, NULL) != 0)
+ {
+ puts ("w1 join failed");
+ return 1;
+ }
+ if (pthread_join (w2, NULL) != 0)
+ {
+ puts ("w2 join failed");
+ return 1;
+ }
+ if (pthread_join (rp, NULL) != 0)
+ {
+ puts ("reader_producer join failed");
+ return 1;
+ }
+ if (pthread_join (rc, NULL) != 0)
+ {
+ puts ("reader_consumer join failed");
+ return 1;
+ }
+
+ return 0;
+}
+
+
+#define TIMEOUT 3
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"