|
|
d8307d |
nptl: Fix pthread_rwlock_try*lock stalls (Bug 23844)
|
|
|
d8307d |
|
|
|
d8307d |
For a full analysis of both the pthread_rwlock_tryrdlock() stall
|
|
|
d8307d |
and the pthread_rwlock_trywrlock() stall see:
|
|
|
d8307d |
https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c14
|
|
|
d8307d |
|
|
|
d8307d |
In the pthread_rwlock_trydlock() function we fail to inspect for
|
|
|
d8307d |
PTHREAD_RWLOCK_FUTEX_USED in __wrphase_futex and wake the waiting
|
|
|
d8307d |
readers.
|
|
|
d8307d |
|
|
|
d8307d |
In the pthread_rwlock_trywrlock() function we write 1 to
|
|
|
d8307d |
__wrphase_futex and loose the setting of the PTHREAD_RWLOCK_FUTEX_USED
|
|
|
d8307d |
bit, again failing to wake waiting readers during unlock.
|
|
|
d8307d |
|
|
|
d8307d |
The fix in the case of pthread_rwlock_trydlock() is to check for
|
|
|
d8307d |
PTHREAD_RWLOCK_FUTEX_USED and wake the readers.
|
|
|
d8307d |
|
|
|
d8307d |
The fix in the case of pthread_rwlock_trywrlock() is to only write
|
|
|
d8307d |
1 to __wrphase_futex if we installed the write phase, since all other
|
|
|
d8307d |
readers would be spinning waiting for this step.
|
|
|
d8307d |
|
|
|
d8307d |
We add two new tests, one exercises the stall for
|
|
|
d8307d |
pthread_rwlock_trywrlock() which is easy to exercise, and one exercises
|
|
|
d8307d |
the stall for pthread_rwlock_trydlock() which is harder to exercise.
|
|
|
d8307d |
|
|
|
d8307d |
The pthread_rwlock_trywrlock() test fails consistently without the fix,
|
|
|
d8307d |
and passes after. The pthread_rwlock_tryrdlock() test fails roughly
|
|
|
d8307d |
5-10% of the time without the fix, and passes all the time after.
|
|
|
d8307d |
|
|
|
d8307d |
Signed-off-by: Carlos O'Donell <carlos@redhat.com>
|
|
|
d8307d |
Signed-off-by: Torvald Riegel <triegel@redhat.com>
|
|
|
d8307d |
Signed-off-by: Rik Prohaska <prohaska7@gmail.com>
|
|
|
d8307d |
Co-authored-by: Torvald Riegel <triegel@redhat.com>
|
|
|
d8307d |
Co-authored-by: Rik Prohaska <prohaska7@gmail.com>
|
|
|
d8307d |
(cherry picked from commit 5fc9ed4c4058bfbdf51ad6e7aac7d209b580e8c4)
|
|
|
d8307d |
|
|
|
d8307d |
diff --git a/ChangeLog b/ChangeLog
|
|
|
d8307d |
index 08b42bd2f56471e3..ed1a2ffe8356fd96 100644
|
|
|
d8307d |
--- a/ChangeLog
|
|
|
d8307d |
+++ b/ChangeLog
|
|
|
d8307d |
@@ -1,3 +1,20 @@
|
|
|
d8307d |
+2019-01-31 Carlos O'Donell <carlos@redhat.com>
|
|
|
d8307d |
+ Torvald Riegel <triegel@redhat.com>
|
|
|
d8307d |
+ Rik Prohaska <prohaska7@gmail.com>
|
|
|
d8307d |
+
|
|
|
d8307d |
+ [BZ# 23844]
|
|
|
d8307d |
+ * nptl/Makefile (tests): Add tst-rwlock-tryrdlock-stall, and
|
|
|
d8307d |
+ tst-rwlock-trywrlock-stall.
|
|
|
d8307d |
+ * nptl/pthread_rwlock_tryrdlock.c (__pthread_rwlock_tryrdlock):
|
|
|
d8307d |
+ Wake waiters if PTHREAD_RWLOCK_FUTEX_USED is set.
|
|
|
d8307d |
+ * nptl/pthread_rwlock_trywrlock.c (__pthread_rwlock_trywrlock):
|
|
|
d8307d |
+ Set __wrphase_fute to 1 only if we started the write phase.
|
|
|
d8307d |
+ * nptl/tst-rwlock-tryrdlock-stall.c: New file.
|
|
|
d8307d |
+ * nptl/tst-rwlock-trywrlock-stall.c: New file.
|
|
|
d8307d |
+ * support/Makefile (libsupport-routines): Add xpthread_rwlock_destroy.
|
|
|
d8307d |
+ * support/xpthread_rwlock_destroy.c: New file.
|
|
|
d8307d |
+ * support/xthread.h: Declare xpthread_rwlock_destroy.
|
|
|
d8307d |
+
|
|
|
d8307d |
2018-08-01 Carlos O'Donel <carlos@redhat.com>
|
|
|
d8307d |
|
|
|
d8307d |
* version.h (RELEASE): Set to "stable".
|
|
|
d8307d |
diff --git a/nptl/Makefile b/nptl/Makefile
|
|
|
d8307d |
index 2d2db648f730db61..b1003cf56b31ddfa 100644
|
|
|
d8307d |
--- a/nptl/Makefile
|
|
|
d8307d |
+++ b/nptl/Makefile
|
|
|
d8307d |
@@ -319,7 +319,8 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
|
|
|
d8307d |
tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
|
|
|
d8307d |
tst-cnd-timedwait tst-thrd-detach tst-mtx-basic tst-thrd-sleep \
|
|
|
d8307d |
tst-mtx-recursive tst-tss-basic tst-call-once tst-mtx-timedlock \
|
|
|
d8307d |
- tst-rwlock-pwn
|
|
|
d8307d |
+ tst-rwlock-pwn \
|
|
|
d8307d |
+ tst-rwlock-tryrdlock-stall tst-rwlock-trywrlock-stall
|
|
|
d8307d |
|
|
|
d8307d |
tests-internal := tst-rwlock19 tst-rwlock20 \
|
|
|
d8307d |
tst-sem11 tst-sem12 tst-sem13 \
|
|
|
d8307d |
diff --git a/nptl/pthread_rwlock_tryrdlock.c b/nptl/pthread_rwlock_tryrdlock.c
|
|
|
d8307d |
index 4aec1fc15acb2448..31a88d33a6e8f256 100644
|
|
|
d8307d |
--- a/nptl/pthread_rwlock_tryrdlock.c
|
|
|
d8307d |
+++ b/nptl/pthread_rwlock_tryrdlock.c
|
|
|
d8307d |
@@ -94,15 +94,22 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
|
|
|
d8307d |
/* Same as in __pthread_rwlock_rdlock_full:
|
|
|
d8307d |
We started the read phase, so we are also responsible for
|
|
|
d8307d |
updating the write-phase futex. Relaxed MO is sufficient.
|
|
|
d8307d |
- Note that there can be no other reader that we have to wake
|
|
|
d8307d |
- because all other readers will see the read phase started by us
|
|
|
d8307d |
- (or they will try to start it themselves); if a writer started
|
|
|
d8307d |
- the read phase, we cannot have started it. Furthermore, we
|
|
|
d8307d |
- cannot discard a PTHREAD_RWLOCK_FUTEX_USED flag because we will
|
|
|
d8307d |
- overwrite the value set by the most recent writer (or the readers
|
|
|
d8307d |
- before it in case of explicit hand-over) and we know that there
|
|
|
d8307d |
- are no waiting readers. */
|
|
|
d8307d |
- atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0);
|
|
|
d8307d |
+ We have to do the same steps as a writer would when handing over the
|
|
|
d8307d |
+ read phase to use because other readers cannot distinguish between
|
|
|
d8307d |
+ us and the writer.
|
|
|
d8307d |
+ Note that __pthread_rwlock_tryrdlock callers will not have to be
|
|
|
d8307d |
+ woken up because they will either see the read phase started by us
|
|
|
d8307d |
+ or they will try to start it themselves; however, callers of
|
|
|
d8307d |
+ __pthread_rwlock_rdlock_full just increase the reader count and then
|
|
|
d8307d |
+ check what state the lock is in, so they cannot distinguish between
|
|
|
d8307d |
+ us and a writer that acquired and released the lock in the
|
|
|
d8307d |
+ meantime. */
|
|
|
d8307d |
+ if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0)
|
|
|
d8307d |
+ & PTHREAD_RWLOCK_FUTEX_USED) != 0)
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ int private = __pthread_rwlock_get_private (rwlock);
|
|
|
d8307d |
+ futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private);
|
|
|
d8307d |
+ }
|
|
|
d8307d |
}
|
|
|
d8307d |
|
|
|
d8307d |
return 0;
|
|
|
d8307d |
diff --git a/nptl/pthread_rwlock_trywrlock.c b/nptl/pthread_rwlock_trywrlock.c
|
|
|
d8307d |
index 5a73eba756077297..f2e3443466a2554f 100644
|
|
|
d8307d |
--- a/nptl/pthread_rwlock_trywrlock.c
|
|
|
d8307d |
+++ b/nptl/pthread_rwlock_trywrlock.c
|
|
|
d8307d |
@@ -46,8 +46,15 @@ __pthread_rwlock_trywrlock (pthread_rwlock_t *rwlock)
|
|
|
d8307d |
&rwlock->__data.__readers, &r,
|
|
|
d8307d |
r | PTHREAD_RWLOCK_WRPHASE | PTHREAD_RWLOCK_WRLOCKED))
|
|
|
d8307d |
{
|
|
|
d8307d |
+ /* We have become the primary writer and we cannot have shared
|
|
|
d8307d |
+ the PTHREAD_RWLOCK_FUTEX_USED flag with someone else, so we
|
|
|
d8307d |
+ can simply enable blocking (see full wrlock code). */
|
|
|
d8307d |
atomic_store_relaxed (&rwlock->__data.__writers_futex, 1);
|
|
|
d8307d |
- atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1);
|
|
|
d8307d |
+ /* If we started a write phase, we need to enable readers to
|
|
|
d8307d |
+ wait. If we did not, we must not change it because other threads
|
|
|
d8307d |
+ may have set the PTHREAD_RWLOCK_FUTEX_USED in the meantime. */
|
|
|
d8307d |
+ if ((r & PTHREAD_RWLOCK_WRPHASE) == 0)
|
|
|
d8307d |
+ atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1);
|
|
|
d8307d |
atomic_store_relaxed (&rwlock->__data.__cur_writer,
|
|
|
d8307d |
THREAD_GETMEM (THREAD_SELF, tid));
|
|
|
d8307d |
return 0;
|
|
|
d8307d |
diff --git a/nptl/tst-rwlock-tryrdlock-stall.c b/nptl/tst-rwlock-tryrdlock-stall.c
|
|
|
d8307d |
new file mode 100644
|
|
|
d8307d |
index 0000000000000000..5e476da2b8d00c6a
|
|
|
d8307d |
--- /dev/null
|
|
|
d8307d |
+++ b/nptl/tst-rwlock-tryrdlock-stall.c
|
|
|
d8307d |
@@ -0,0 +1,355 @@
|
|
|
d8307d |
+/* Bug 23844: Test for pthread_rwlock_tryrdlock stalls.
|
|
|
d8307d |
+ Copyright (C) 2019 Free Software Foundation, Inc.
|
|
|
d8307d |
+ This file is part of the GNU C Library.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The GNU C Library is free software; you can redistribute it and/or
|
|
|
d8307d |
+ modify it under the terms of the GNU Lesser General Public
|
|
|
d8307d |
+ License as published by the Free Software Foundation; either
|
|
|
d8307d |
+ version 2.1 of the License, or (at your option) any later version.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The GNU C Library is distributed in the hope that it will be useful,
|
|
|
d8307d |
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
|
|
|
d8307d |
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
|
|
d8307d |
+ Lesser General Public License for more details.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ You should have received a copy of the GNU Lesser General Public
|
|
|
d8307d |
+ License along with the GNU C Library; if not, see
|
|
|
d8307d |
+ <http://www.gnu.org/licenses/>. */
|
|
|
d8307d |
+
|
|
|
d8307d |
+/* For a full analysis see comment:
|
|
|
d8307d |
+ https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c14
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Provided here for reference:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ --- Analysis of pthread_rwlock_tryrdlock() stall ---
|
|
|
d8307d |
+ A read lock begins to execute.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ In __pthread_rwlock_rdlock_full:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We can attempt a read lock, but find that the lock is
|
|
|
d8307d |
+ in a write phase (PTHREAD_RWLOCK_WRPHASE, or WP-bit
|
|
|
d8307d |
+ is set), and the lock is held by a primary writer
|
|
|
d8307d |
+ (PTHREAD_RWLOCK_WRLOCKED is set). In this case we must
|
|
|
d8307d |
+ wait for explicit hand over from the writer to us or
|
|
|
d8307d |
+ one of the other waiters. The read lock threads are
|
|
|
d8307d |
+ about to execute:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 341 r = (atomic_fetch_add_acquire (&rwlock->__data.__readers,
|
|
|
d8307d |
+ 342 (1 << PTHREAD_RWLOCK_READER_SHIFT))
|
|
|
d8307d |
+ 343 + (1 << PTHREAD_RWLOCK_READER_SHIFT));
|
|
|
d8307d |
+
|
|
|
d8307d |
+ An unlock beings to execute.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Then in __pthread_rwlock_wrunlock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 547 unsigned int r = atomic_load_relaxed (&rwlock->__data.__readers);
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 549 while (!atomic_compare_exchange_weak_release
|
|
|
d8307d |
+ 550 (&rwlock->__data.__readers, &r,
|
|
|
d8307d |
+ 551 ((r ^ PTHREAD_RWLOCK_WRLOCKED)
|
|
|
d8307d |
+ 552 ^ ((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0 ? 0
|
|
|
d8307d |
+ 553 : PTHREAD_RWLOCK_WRPHASE))))
|
|
|
d8307d |
+ 554 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 556 }
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We clear PTHREAD_RWLOCK_WRLOCKED, and if there are
|
|
|
d8307d |
+ no readers so we leave the lock in PTHRAD_RWLOCK_WRPHASE.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Back in the read lock.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The read lock adjusts __readres as above.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 383 while ((r & PTHREAD_RWLOCK_WRPHASE) != 0
|
|
|
d8307d |
+ 384 && (r & PTHREAD_RWLOCK_WRLOCKED) == 0)
|
|
|
d8307d |
+ 385 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 390 if (atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers, &r,
|
|
|
d8307d |
+ 391 r ^ PTHREAD_RWLOCK_WRPHASE))
|
|
|
d8307d |
+ 392 {
|
|
|
d8307d |
+
|
|
|
d8307d |
+ And then attemps to start the read phase.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Assume there happens to be a tryrdlock at this point, noting
|
|
|
d8307d |
+ that PTHREAD_RWLOCK_WRLOCKED is clear, and PTHREAD_RWLOCK_WRPHASE
|
|
|
d8307d |
+ is 1. So the try lock attemps to start the read phase.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ In __pthread_rwlock_tryrdlock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 44 if ((r & PTHREAD_RWLOCK_WRPHASE) == 0)
|
|
|
d8307d |
+ 45 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 49 if (((r & PTHREAD_RWLOCK_WRLOCKED) != 0)
|
|
|
d8307d |
+ 50 && (rwlock->__data.__flags
|
|
|
d8307d |
+ 51 == PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP))
|
|
|
d8307d |
+ 52 return EBUSY;
|
|
|
d8307d |
+ 53 rnew = r + (1 << PTHREAD_RWLOCK_READER_SHIFT);
|
|
|
d8307d |
+ 54 }
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 89 while (!atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers,
|
|
|
d8307d |
+ 90 &r, rnew));
|
|
|
d8307d |
+
|
|
|
d8307d |
+ And succeeds.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Back in the write unlock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 557 if ((r >> PTHREAD_RWLOCK_READER_SHIFT) != 0)
|
|
|
d8307d |
+ 558 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 563 if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0)
|
|
|
d8307d |
+ 564 & PTHREAD_RWLOCK_FUTEX_USED) != 0)
|
|
|
d8307d |
+ 565 futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private);
|
|
|
d8307d |
+ 566 }
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We note that PTHREAD_RWLOCK_FUTEX_USED is non-zero
|
|
|
d8307d |
+ and don't wake anyone. This is OK because we handed
|
|
|
d8307d |
+ over to the trylock. It will be the trylock's responsibility
|
|
|
d8307d |
+ to wake any waiters.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Back in the read lock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The read lock fails to install PTHRAD_REWLOCK_WRPHASE as 0 because
|
|
|
d8307d |
+ the __readers value was adjusted by the trylock, and so it falls through
|
|
|
d8307d |
+ to waiting on the lock for explicit handover from either a new writer
|
|
|
d8307d |
+ or a new reader.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 448 int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex,
|
|
|
d8307d |
+ 449 1 | PTHREAD_RWLOCK_FUTEX_USED,
|
|
|
d8307d |
+ 450 abstime, private);
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We use PTHREAD_RWLOCK_FUTEX_USED to indicate the futex
|
|
|
d8307d |
+ is in use.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ At this point we have readers waiting on the read lock
|
|
|
d8307d |
+ to unlock. The wrlock is done. The trylock is finishing
|
|
|
d8307d |
+ the installation of the read phase.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 92 if ((r & PTHREAD_RWLOCK_WRPHASE) != 0)
|
|
|
d8307d |
+ 93 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 105 atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0);
|
|
|
d8307d |
+ 106 }
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The trylock does note that we were the one that
|
|
|
d8307d |
+ installed the read phase, but the comments are not
|
|
|
d8307d |
+ correct, the execution ordering above shows that
|
|
|
d8307d |
+ readers might indeed be waiting, and they are.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The atomic_store_relaxed throws away PTHREAD_RWLOCK_FUTEX_USED,
|
|
|
d8307d |
+ and the waiting reader is never worken becuase as noted
|
|
|
d8307d |
+ above it is conditional on the futex being used.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The solution is for the trylock thread to inspect
|
|
|
d8307d |
+ PTHREAD_RWLOCK_FUTEX_USED and wake the waiting readers.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ --- Analysis of pthread_rwlock_trywrlock() stall ---
|
|
|
d8307d |
+
|
|
|
d8307d |
+ A write lock begins to execute, takes the write lock,
|
|
|
d8307d |
+ and then releases the lock...
|
|
|
d8307d |
+
|
|
|
d8307d |
+ In pthread_rwlock_wrunlock():
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 547 unsigned int r = atomic_load_relaxed (&rwlock->__data.__readers);
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 549 while (!atomic_compare_exchange_weak_release
|
|
|
d8307d |
+ 550 (&rwlock->__data.__readers, &r,
|
|
|
d8307d |
+ 551 ((r ^ PTHREAD_RWLOCK_WRLOCKED)
|
|
|
d8307d |
+ 552 ^ ((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0 ? 0
|
|
|
d8307d |
+ 553 : PTHREAD_RWLOCK_WRPHASE))))
|
|
|
d8307d |
+ 554 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 556 }
|
|
|
d8307d |
+
|
|
|
d8307d |
+ ... leaving it in the write phase with zero readers
|
|
|
d8307d |
+ (the case where we leave the write phase in place
|
|
|
d8307d |
+ during a write unlock).
|
|
|
d8307d |
+
|
|
|
d8307d |
+ A write trylock begins to execute.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ In __pthread_rwlock_trywrlock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 40 while (((r & PTHREAD_RWLOCK_WRLOCKED) == 0)
|
|
|
d8307d |
+ 41 && (((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0)
|
|
|
d8307d |
+ 42 || (prefer_writer && ((r & PTHREAD_RWLOCK_WRPHASE) != 0))))
|
|
|
d8307d |
+ 43 {
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The lock is not locked.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ There are no readers.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 45 if (atomic_compare_exchange_weak_acquire (
|
|
|
d8307d |
+ 46 &rwlock->__data.__readers, &r,
|
|
|
d8307d |
+ 47 r | PTHREAD_RWLOCK_WRPHASE | PTHREAD_RWLOCK_WRLOCKED))
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We atomically install the write phase and we take the
|
|
|
d8307d |
+ exclusive write lock.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 48 {
|
|
|
d8307d |
+ 49 atomic_store_relaxed (&rwlock->__data.__writers_futex, 1);
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We get this far.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ A reader lock begins to execute.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ In pthread_rwlock_rdlock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 437 for (;;)
|
|
|
d8307d |
+ 438 {
|
|
|
d8307d |
+ 439 while (((wpf = atomic_load_relaxed (&rwlock->__data.__wrphase_futex))
|
|
|
d8307d |
+ 440 | PTHREAD_RWLOCK_FUTEX_USED) == (1 | PTHREAD_RWLOCK_FUTEX_USED))
|
|
|
d8307d |
+ 441 {
|
|
|
d8307d |
+ 442 int private = __pthread_rwlock_get_private (rwlock);
|
|
|
d8307d |
+ 443 if (((wpf & PTHREAD_RWLOCK_FUTEX_USED) == 0)
|
|
|
d8307d |
+ 444 && (!atomic_compare_exchange_weak_relaxed
|
|
|
d8307d |
+ 445 (&rwlock->__data.__wrphase_futex,
|
|
|
d8307d |
+ 446 &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED)))
|
|
|
d8307d |
+ 447 continue;
|
|
|
d8307d |
+ 448 int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex,
|
|
|
d8307d |
+ 449 1 | PTHREAD_RWLOCK_FUTEX_USED,
|
|
|
d8307d |
+ 450 abstime, private);
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We are in a write phase, so the while() on line 439 is true.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The value of wpf does not have PTHREAD_RWLOCK_FUTEX_USED set
|
|
|
d8307d |
+ since this is the first reader to lock.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The atomic operation sets wpf with PTHREAD_RELOCK_FUTEX_USED
|
|
|
d8307d |
+ on the expectation that this reader will be woken during
|
|
|
d8307d |
+ the handoff.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Back in pthread_rwlock_trywrlock:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 50 atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1);
|
|
|
d8307d |
+ 51 atomic_store_relaxed (&rwlock->__data.__cur_writer,
|
|
|
d8307d |
+ 52 THREAD_GETMEM (THREAD_SELF, tid));
|
|
|
d8307d |
+ 53 return 0;
|
|
|
d8307d |
+ 54 }
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 57 }
|
|
|
d8307d |
+
|
|
|
d8307d |
+ We write 1 to __wrphase_futex discarding PTHREAD_RWLOCK_FUTEX_USED,
|
|
|
d8307d |
+ and so in the unlock we will not awaken the waiting reader.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The solution to this is to realize that if we did not start the write
|
|
|
d8307d |
+ phase we need not write 1 or any other value to __wrphase_futex.
|
|
|
d8307d |
+ This ensures that any readers (which saw __wrphase_futex != 0) can
|
|
|
d8307d |
+ set PTHREAD_RWLOCK_FUTEX_USED and this can be used at unlock to
|
|
|
d8307d |
+ wake them.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ If we installed the write phase then all other readers are looping
|
|
|
d8307d |
+ here:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ In __pthread_rwlock_rdlock_full:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ 437 for (;;)
|
|
|
d8307d |
+ 438 {
|
|
|
d8307d |
+ 439 while (((wpf = atomic_load_relaxed (&rwlock->__data.__wrphase_futex))
|
|
|
d8307d |
+ 440 | PTHREAD_RWLOCK_FUTEX_USED) == (1 | PTHREAD_RWLOCK_FUTEX_USED))
|
|
|
d8307d |
+ 441 {
|
|
|
d8307d |
+ ...
|
|
|
d8307d |
+ 508 }
|
|
|
d8307d |
+
|
|
|
d8307d |
+ waiting for the write phase to be installed or removed before they
|
|
|
d8307d |
+ can begin waiting on __wrphase_futex (part of the algorithm), or
|
|
|
d8307d |
+ taking a concurrent read lock, and thus we can safely write 1 to
|
|
|
d8307d |
+ __wrphase_futex.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ If we did not install the write phase then the readers may already
|
|
|
d8307d |
+ be waiting on the futex, the original writer wrote 1 to __wrphase_futex
|
|
|
d8307d |
+ as part of starting the write phase, and we cannot also write 1
|
|
|
d8307d |
+ without loosing the PTHREAD_RWLOCK_FUTEX_USED bit.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ ---
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Summary for the pthread_rwlock_tryrdlock() stall:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The stall is caused by pthread_rwlock_tryrdlock failing to check
|
|
|
d8307d |
+ that PTHREAD_RWLOCK_FUTEX_USED is set in the __wrphase_futex futex
|
|
|
d8307d |
+ and then waking the futex.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The fix for bug 23844 ensures that waiters on __wrphase_futex are
|
|
|
d8307d |
+ correctly woken. Before the fix the test stalls as readers can
|
|
|
d8307d |
+ wait forever on __wrphase_futex. */
|
|
|
d8307d |
+
|
|
|
d8307d |
+#include <stdio.h>
|
|
|
d8307d |
+#include <stdlib.h>
|
|
|
d8307d |
+#include <unistd.h>
|
|
|
d8307d |
+#include <pthread.h>
|
|
|
d8307d |
+#include <support/xthread.h>
|
|
|
d8307d |
+#include <errno.h>
|
|
|
d8307d |
+
|
|
|
d8307d |
+/* We need only one lock to reproduce the issue. We will need multiple
|
|
|
d8307d |
+ threads to get the exact case where we have a read, try, and unlock
|
|
|
d8307d |
+ all interleaving to produce the case where the readers are waiting
|
|
|
d8307d |
+ and the try fails to wake them. */
|
|
|
d8307d |
+pthread_rwlock_t onelock;
|
|
|
d8307d |
+
|
|
|
d8307d |
+/* The number of threads is arbitrary but empirically chosen to have
|
|
|
d8307d |
+ enough threads that we see the condition where waiting readers are
|
|
|
d8307d |
+ not woken by a successful tryrdlock. */
|
|
|
d8307d |
+#define NTHREADS 32
|
|
|
d8307d |
+
|
|
|
d8307d |
+_Atomic int do_exit;
|
|
|
d8307d |
+
|
|
|
d8307d |
+void *
|
|
|
d8307d |
+run_loop (void *arg)
|
|
|
d8307d |
+{
|
|
|
d8307d |
+ int i = 0, ret;
|
|
|
d8307d |
+ while (!do_exit)
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ /* Arbitrarily choose if we are the writer or reader. Choose a
|
|
|
d8307d |
+ high enough ratio of readers to writers to make it likely
|
|
|
d8307d |
+ that readers block (and eventually are susceptable to
|
|
|
d8307d |
+ stalling).
|
|
|
d8307d |
+
|
|
|
d8307d |
+ If we are a writer, take the write lock, and then unlock.
|
|
|
d8307d |
+ If we are a reader, try the lock, then lock, then unlock. */
|
|
|
d8307d |
+ if ((i % 8) != 0)
|
|
|
d8307d |
+ xpthread_rwlock_wrlock (&onelock);
|
|
|
d8307d |
+ else
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ if ((ret = pthread_rwlock_tryrdlock (&onelock)) != 0)
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ if (ret == EBUSY)
|
|
|
d8307d |
+ xpthread_rwlock_rdlock (&onelock);
|
|
|
d8307d |
+ else
|
|
|
d8307d |
+ exit (EXIT_FAILURE);
|
|
|
d8307d |
+ }
|
|
|
d8307d |
+ }
|
|
|
d8307d |
+ /* Thread does some work and then unlocks. */
|
|
|
d8307d |
+ xpthread_rwlock_unlock (&onelock);
|
|
|
d8307d |
+ i++;
|
|
|
d8307d |
+ }
|
|
|
d8307d |
+ return NULL;
|
|
|
d8307d |
+}
|
|
|
d8307d |
+
|
|
|
d8307d |
+int
|
|
|
d8307d |
+do_test (void)
|
|
|
d8307d |
+{
|
|
|
d8307d |
+ int i;
|
|
|
d8307d |
+ pthread_t tids[NTHREADS];
|
|
|
d8307d |
+ xpthread_rwlock_init (&onelock, NULL);
|
|
|
d8307d |
+ for (i = 0; i < NTHREADS; i++)
|
|
|
d8307d |
+ tids[i] = xpthread_create (NULL, run_loop, NULL);
|
|
|
d8307d |
+ /* Run for some amount of time. Empirically speaking exercising
|
|
|
d8307d |
+ the stall via pthread_rwlock_tryrdlock is much harder, and on
|
|
|
d8307d |
+ a 3.5GHz 4 core x86_64 VM system it takes somewhere around
|
|
|
d8307d |
+ 20-200s to stall, approaching 100% stall past 200s. We can't
|
|
|
d8307d |
+ wait that long for a regression test so we just test for 20s,
|
|
|
d8307d |
+ and expect the stall to happen with a 5-10% chance (enough for
|
|
|
d8307d |
+ developers to see). */
|
|
|
d8307d |
+ sleep (20);
|
|
|
d8307d |
+ /* Then exit. */
|
|
|
d8307d |
+ printf ("INFO: Exiting...\n");
|
|
|
d8307d |
+ do_exit = 1;
|
|
|
d8307d |
+ /* If any readers stalled then we will timeout waiting for them. */
|
|
|
d8307d |
+ for (i = 0; i < NTHREADS; i++)
|
|
|
d8307d |
+ xpthread_join (tids[i]);
|
|
|
d8307d |
+ printf ("INFO: Done.\n");
|
|
|
d8307d |
+ xpthread_rwlock_destroy (&onelock);
|
|
|
d8307d |
+ printf ("PASS: No pthread_rwlock_tryrdlock stalls detected.\n");
|
|
|
d8307d |
+ return 0;
|
|
|
d8307d |
+}
|
|
|
d8307d |
+
|
|
|
d8307d |
+#define TIMEOUT 30
|
|
|
d8307d |
+#include <support/test-driver.c>
|
|
|
d8307d |
diff --git a/nptl/tst-rwlock-trywrlock-stall.c b/nptl/tst-rwlock-trywrlock-stall.c
|
|
|
d8307d |
new file mode 100644
|
|
|
d8307d |
index 0000000000000000..14d27cbcbc882cb1
|
|
|
d8307d |
--- /dev/null
|
|
|
d8307d |
+++ b/nptl/tst-rwlock-trywrlock-stall.c
|
|
|
d8307d |
@@ -0,0 +1,108 @@
|
|
|
d8307d |
+/* Bug 23844: Test for pthread_rwlock_trywrlock stalls.
|
|
|
d8307d |
+ Copyright (C) 2019 Free Software Foundation, Inc.
|
|
|
d8307d |
+ This file is part of the GNU C Library.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The GNU C Library is free software; you can redistribute it and/or
|
|
|
d8307d |
+ modify it under the terms of the GNU Lesser General Public
|
|
|
d8307d |
+ License as published by the Free Software Foundation; either
|
|
|
d8307d |
+ version 2.1 of the License, or (at your option) any later version.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The GNU C Library is distributed in the hope that it will be useful,
|
|
|
d8307d |
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
|
|
|
d8307d |
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
|
|
d8307d |
+ Lesser General Public License for more details.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ You should have received a copy of the GNU Lesser General Public
|
|
|
d8307d |
+ License along with the GNU C Library; if not, see
|
|
|
d8307d |
+ <http://www.gnu.org/licenses/>. */
|
|
|
d8307d |
+
|
|
|
d8307d |
+/* For a full analysis see comments in tst-rwlock-tryrdlock-stall.c.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ Summary for the pthread_rwlock_trywrlock() stall:
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The stall is caused by pthread_rwlock_trywrlock setting
|
|
|
d8307d |
+ __wrphase_futex futex to 1 and loosing the
|
|
|
d8307d |
+ PTHREAD_RWLOCK_FUTEX_USED bit.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The fix for bug 23844 ensures that waiters on __wrphase_futex are
|
|
|
d8307d |
+ correctly woken. Before the fix the test stalls as readers can
|
|
|
d8307d |
+ wait forever on __wrphase_futex. */
|
|
|
d8307d |
+
|
|
|
d8307d |
+#include <stdio.h>
|
|
|
d8307d |
+#include <stdlib.h>
|
|
|
d8307d |
+#include <unistd.h>
|
|
|
d8307d |
+#include <pthread.h>
|
|
|
d8307d |
+#include <support/xthread.h>
|
|
|
d8307d |
+#include <errno.h>
|
|
|
d8307d |
+
|
|
|
d8307d |
+/* We need only one lock to reproduce the issue. We will need multiple
|
|
|
d8307d |
+ threads to get the exact case where we have a read, try, and unlock
|
|
|
d8307d |
+ all interleaving to produce the case where the readers are waiting
|
|
|
d8307d |
+ and the try clears the PTHREAD_RWLOCK_FUTEX_USED bit and a
|
|
|
d8307d |
+ subsequent unlock fails to wake them. */
|
|
|
d8307d |
+pthread_rwlock_t onelock;
|
|
|
d8307d |
+
|
|
|
d8307d |
+/* The number of threads is arbitrary but empirically chosen to have
|
|
|
d8307d |
+ enough threads that we see the condition where waiting readers are
|
|
|
d8307d |
+ not woken by a successful unlock. */
|
|
|
d8307d |
+#define NTHREADS 32
|
|
|
d8307d |
+
|
|
|
d8307d |
+_Atomic int do_exit;
|
|
|
d8307d |
+
|
|
|
d8307d |
+void *
|
|
|
d8307d |
+run_loop (void *arg)
|
|
|
d8307d |
+{
|
|
|
d8307d |
+ int i = 0, ret;
|
|
|
d8307d |
+ while (!do_exit)
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ /* Arbitrarily choose if we are the writer or reader. Choose a
|
|
|
d8307d |
+ high enough ratio of readers to writers to make it likely
|
|
|
d8307d |
+ that readers block (and eventually are susceptable to
|
|
|
d8307d |
+ stalling).
|
|
|
d8307d |
+
|
|
|
d8307d |
+ If we are a writer, take the write lock, and then unlock.
|
|
|
d8307d |
+ If we are a reader, try the lock, then lock, then unlock. */
|
|
|
d8307d |
+ if ((i % 8) != 0)
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ if ((ret = pthread_rwlock_trywrlock (&onelock)) != 0)
|
|
|
d8307d |
+ {
|
|
|
d8307d |
+ if (ret == EBUSY)
|
|
|
d8307d |
+ xpthread_rwlock_wrlock (&onelock);
|
|
|
d8307d |
+ else
|
|
|
d8307d |
+ exit (EXIT_FAILURE);
|
|
|
d8307d |
+ }
|
|
|
d8307d |
+ }
|
|
|
d8307d |
+ else
|
|
|
d8307d |
+ xpthread_rwlock_rdlock (&onelock);
|
|
|
d8307d |
+ /* Thread does some work and then unlocks. */
|
|
|
d8307d |
+ xpthread_rwlock_unlock (&onelock);
|
|
|
d8307d |
+ i++;
|
|
|
d8307d |
+ }
|
|
|
d8307d |
+ return NULL;
|
|
|
d8307d |
+}
|
|
|
d8307d |
+
|
|
|
d8307d |
+int
|
|
|
d8307d |
+do_test (void)
|
|
|
d8307d |
+{
|
|
|
d8307d |
+ int i;
|
|
|
d8307d |
+ pthread_t tids[NTHREADS];
|
|
|
d8307d |
+ xpthread_rwlock_init (&onelock, NULL);
|
|
|
d8307d |
+ for (i = 0; i < NTHREADS; i++)
|
|
|
d8307d |
+ tids[i] = xpthread_create (NULL, run_loop, NULL);
|
|
|
d8307d |
+ /* Run for some amount of time. The pthread_rwlock_tryrwlock stall
|
|
|
d8307d |
+ is very easy to trigger and happens in seconds under the test
|
|
|
d8307d |
+ conditions. */
|
|
|
d8307d |
+ sleep (10);
|
|
|
d8307d |
+ /* Then exit. */
|
|
|
d8307d |
+ printf ("INFO: Exiting...\n");
|
|
|
d8307d |
+ do_exit = 1;
|
|
|
d8307d |
+ /* If any readers stalled then we will timeout waiting for them. */
|
|
|
d8307d |
+ for (i = 0; i < NTHREADS; i++)
|
|
|
d8307d |
+ xpthread_join (tids[i]);
|
|
|
d8307d |
+ printf ("INFO: Done.\n");
|
|
|
d8307d |
+ xpthread_rwlock_destroy (&onelock);
|
|
|
d8307d |
+ printf ("PASS: No pthread_rwlock_tryrwlock stalls detected.\n");
|
|
|
d8307d |
+ return 0;
|
|
|
d8307d |
+}
|
|
|
d8307d |
+
|
|
|
d8307d |
+#include <support/test-driver.c>
|
|
|
d8307d |
diff --git a/support/Makefile b/support/Makefile
|
|
|
d8307d |
index 93a514301654132e..41da4abaaa5a645a 100644
|
|
|
d8307d |
--- a/support/Makefile
|
|
|
d8307d |
+++ b/support/Makefile
|
|
|
d8307d |
@@ -129,6 +129,7 @@ libsupport-routines = \
|
|
|
d8307d |
xpthread_mutexattr_settype \
|
|
|
d8307d |
xpthread_once \
|
|
|
d8307d |
xpthread_rwlock_init \
|
|
|
d8307d |
+ xpthread_rwlock_destroy \
|
|
|
d8307d |
xpthread_rwlock_rdlock \
|
|
|
d8307d |
xpthread_rwlock_unlock \
|
|
|
d8307d |
xpthread_rwlock_wrlock \
|
|
|
d8307d |
diff --git a/support/xpthread_rwlock_destroy.c b/support/xpthread_rwlock_destroy.c
|
|
|
d8307d |
new file mode 100644
|
|
|
d8307d |
index 0000000000000000..6d6e95356963b47f
|
|
|
d8307d |
--- /dev/null
|
|
|
d8307d |
+++ b/support/xpthread_rwlock_destroy.c
|
|
|
d8307d |
@@ -0,0 +1,26 @@
|
|
|
d8307d |
+/* pthread_rwlock_destroy with error checking.
|
|
|
d8307d |
+ Copyright (C) 2019 Free Software Foundation, Inc.
|
|
|
d8307d |
+ This file is part of the GNU C Library.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The GNU C Library is free software; you can redistribute it and/or
|
|
|
d8307d |
+ modify it under the terms of the GNU Lesser General Public
|
|
|
d8307d |
+ License as published by the Free Software Foundation; either
|
|
|
d8307d |
+ version 2.1 of the License, or (at your option) any later version.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ The GNU C Library is distributed in the hope that it will be useful,
|
|
|
d8307d |
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
|
|
|
d8307d |
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
|
|
d8307d |
+ Lesser General Public License for more details.
|
|
|
d8307d |
+
|
|
|
d8307d |
+ You should have received a copy of the GNU Lesser General Public
|
|
|
d8307d |
+ License along with the GNU C Library; if not, see
|
|
|
d8307d |
+ <http://www.gnu.org/licenses/>. */
|
|
|
d8307d |
+
|
|
|
d8307d |
+#include <support/xthread.h>
|
|
|
d8307d |
+
|
|
|
d8307d |
+void
|
|
|
d8307d |
+xpthread_rwlock_destroy (pthread_rwlock_t *rwlock)
|
|
|
d8307d |
+{
|
|
|
d8307d |
+ xpthread_check_return ("pthread_rwlock_destroy",
|
|
|
d8307d |
+ pthread_rwlock_destroy (rwlock));
|
|
|
d8307d |
+}
|
|
|
d8307d |
diff --git a/support/xthread.h b/support/xthread.h
|
|
|
d8307d |
index 623f5ad0acb895ef..1af77280871464c2 100644
|
|
|
d8307d |
--- a/support/xthread.h
|
|
|
d8307d |
+++ b/support/xthread.h
|
|
|
d8307d |
@@ -84,6 +84,7 @@ void xpthread_rwlockattr_setkind_np (pthread_rwlockattr_t *attr, int pref);
|
|
|
d8307d |
void xpthread_rwlock_wrlock (pthread_rwlock_t *rwlock);
|
|
|
d8307d |
void xpthread_rwlock_rdlock (pthread_rwlock_t *rwlock);
|
|
|
d8307d |
void xpthread_rwlock_unlock (pthread_rwlock_t *rwlock);
|
|
|
d8307d |
+void xpthread_rwlock_destroy (pthread_rwlock_t *rwlock);
|
|
|
d8307d |
|
|
|
d8307d |
__END_DECLS
|
|
|
d8307d |
|