[PATCH] Fix missing wake-ups in pthread_rwlock_rdlock.
Siddhesh Poyarekar
siddhesh@redhat.com
Thu Jun 4 10:28:00 GMT 2015
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"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150604/e583823e/attachment.sig>
More information about the Libc-alpha
mailing list