This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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"
Attachment:
pgpXLd6SZ49qx.pgp
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |