Merge ~ahasenack/ubuntu/+source/glibc:bionic-glibc-thread-lock-23844 into ubuntu/+source/glibc:ubuntu/bionic-devel

Proposed by Dimitri John Ledkov
Status: Merged
Merge reported by: Andreas Hasenack
Merged at revision: af68c90898291fa2ee752c156975bcd388b29532
Proposed branch: ~ahasenack/ubuntu/+source/glibc:bionic-glibc-thread-lock-23844
Merge into: ubuntu/+source/glibc:ubuntu/bionic-devel
Diff against target: 702 lines (+677/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/all/branch-pthread_rwlock_trywrlock-hang-23844.patch (+669/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Dimitri John Ledkov (community) Approve
Canonical Server Core Reviewers Pending
Canonical Server Pending
Adam Conrad Pending
git-ubuntu developers Pending
Review via email: mp+380381@code.launchpad.net

Description of the change

Bionic-only glibc fix. The bug has reports of the following projects being affected:
- bind 9.16.0 (not shipped in Bionic). It's important to note that the upcoming 9.16.1 release of bind is already calling out ubuntu as being the only one left with this bug (see https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/3156/diffs)
- subversions hangs under load in bionic (https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c21)
- openvswitch (https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c23 and https://github.com/openvswitch/ovs-issues/issues/175)

I'm out of my depth here and just did the manual work of grabbing the upstream patch, applying it in a test build, and verifying the test case. I also prepared the SRU paperwork, but it could do with more help in the "regression potential" section. The patch I grabbed also has test cases, but I haven't seen their output in the build logs, so I'm not sure if something else is needed to get them to run, or if they ran and I didn't notice, or if such cherry-picks typically don't even include the upstream test cases.

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Bionic-only glibc fix. The bug has reports of the following projects being affected:
- bind 9.16.0 (not shipped in Bionic). It's important to note that the upcoming 9.16.1 release of bind is already calling out ubuntu as being the only one left with this bug (see https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/3156/diffs)
- subversions hangs under load in bionic (https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c21)
- openvswitch (https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c23 and https://github.com/openvswitch/ovs-issues/issues/175)

I'm out of my depth here and just did the manual work of grabbing the upstream patch, applying it in a test build, and verifying the test case. I also prepared the SRU paperwork, but it could do with more help in the "regression potential" section. The patch I grabbed also has test cases, but I haven't seen their output in the build logs, so I'm not sure if something else is needed to get them to run, or if they ran and I didn't notice, or if such cherry-picks typically don't even include the upstream test cases.

Revision history for this message
Dimitri John Ledkov (xnox) :
review: Approve
Revision history for this message
Nish Aravamudan (nacc) wrote :

FWIW, I think we (DigitalOcean) have seen the OVS lockup in our fleet recently. We don't have a test case to reproduce it (yet), but we are definitely interested in testing the proposed fix.

Revision history for this message
Nish Aravamudan (nacc) wrote :

@andreas: in the interest of moving this along, how would you like to proceed on this bugfix? Also, I think the version should be 2.27-3ubuntu1.1, right?

af68c90... by Andreas Hasenack

changelog: fix sru version

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Ops, you are right. I fixed the version. Do you think you would be able to test the fix from the ppa?

I created a new one, since the fixed version is lower than what I had before:

https://launchpad.net/~ahasenack/+archive/ubuntu/glibc-pthread-lock-1864864

I just uploaded, so it will be a while before it can be fetched.

Revision history for this message
Nish Aravamudan (nacc) wrote :

Yes, I will first see if the deadlock was something we could easily reproduce (iirc, it was symptomatic of *other* problems we were seeing in our network stack). I'll try and get ack to you ASAP.

Revision history for this message
Nish Aravamudan (nacc) wrote :

So it turns out we weren't able to provoke this issue specifically, but think it might have been occurring during some other problems. Given the lack of a testcase, I'm not sure I can deploy it safely on our infrastructure.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Ok, thanks for trying

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

This is in bionic-proposed already.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 83ffe7e..0478efa 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+glibc (2.27-3ubuntu1.1) bionic; urgency=medium
7+
8+ * branch-pthread_rwlock_trywrlock-hang-23844.patch: nptl: Fix
9+ pthread_rwlock_try*lock stalls (Bug 23844) (LP: #1864864)
10+
11+ -- Andreas Hasenack <andreas@canonical.com> Tue, 03 Mar 2020 14:58:55 -0300
12+
13 glibc (2.27-3ubuntu1) bionic; urgency=medium
14
15 * Merge with Debian, with packaging tweaks and master updated to 2018-03-29.
16diff --git a/debian/patches/all/branch-pthread_rwlock_trywrlock-hang-23844.patch b/debian/patches/all/branch-pthread_rwlock_trywrlock-hang-23844.patch
17new file mode 100644
18index 0000000..bcc56f6
19--- /dev/null
20+++ b/debian/patches/all/branch-pthread_rwlock_trywrlock-hang-23844.patch
21@@ -0,0 +1,669 @@
22+From e8c13d5f7a6cd34bc2ac51a0c89fcbbfd2e5c043 Mon Sep 17 00:00:00 2001
23+From: Carlos O'Donell <carlos@redhat.com>
24+Date: Mon, 21 Jan 2019 22:50:12 -0500
25+Subject: [PATCH] nptl: Fix pthread_rwlock_try*lock stalls (Bug 23844)
26+
27+For a full analysis of both the pthread_rwlock_tryrdlock() stall
28+and the pthread_rwlock_trywrlock() stall see:
29+https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c14
30+
31+In the pthread_rwlock_trydlock() function we fail to inspect for
32+PTHREAD_RWLOCK_FUTEX_USED in __wrphase_futex and wake the waiting
33+readers.
34+
35+In the pthread_rwlock_trywrlock() function we write 1 to
36+__wrphase_futex and loose the setting of the PTHREAD_RWLOCK_FUTEX_USED
37+bit, again failing to wake waiting readers during unlock.
38+
39+The fix in the case of pthread_rwlock_trydlock() is to check for
40+PTHREAD_RWLOCK_FUTEX_USED and wake the readers.
41+
42+The fix in the case of pthread_rwlock_trywrlock() is to only write
43+1 to __wrphase_futex if we installed the write phase, since all other
44+readers would be spinning waiting for this step.
45+
46+We add two new tests, one exercises the stall for
47+pthread_rwlock_trywrlock() which is easy to exercise, and one exercises
48+the stall for pthread_rwlock_trydlock() which is harder to exercise.
49+
50+The pthread_rwlock_trywrlock() test fails consistently without the fix,
51+and passes after. The pthread_rwlock_tryrdlock() test fails roughly
52+5-10% of the time without the fix, and passes all the time after.
53+
54+Signed-off-by: Carlos O'Donell <carlos@redhat.com>
55+Signed-off-by: Torvald Riegel <triegel@redhat.com>
56+Signed-off-by: Rik Prohaska <prohaska7@gmail.com>
57+Co-authored-by: Torvald Riegel <triegel@redhat.com>
58+Co-authored-by: Rik Prohaska <prohaska7@gmail.com>
59+(cherry picked from commit 5fc9ed4c4058bfbdf51ad6e7aac7d209b580e8c4)
60+---
61+ ChangeLog | 17 ++
62+ NEWS | 1 +
63+ nptl/Makefile | 3 +-
64+ nptl/pthread_rwlock_tryrdlock.c | 25 ++-
65+ nptl/pthread_rwlock_trywrlock.c | 9 +-
66+ nptl/tst-rwlock-tryrdlock-stall.c | 355 ++++++++++++++++++++++++++++++
67+ nptl/tst-rwlock-trywrlock-stall.c | 108 +++++++++
68+ support/Makefile | 1 +
69+ support/xpthread_rwlock_destroy.c | 26 +++
70+ support/xthread.h | 1 +
71+ 10 files changed, 535 insertions(+), 11 deletions(-)
72+ create mode 100644 nptl/tst-rwlock-tryrdlock-stall.c
73+ create mode 100644 nptl/tst-rwlock-trywrlock-stall.c
74+ create mode 100644 support/xpthread_rwlock_destroy.c
75+
76+ Ubuntu notes:
77+ - removed NEWS and ChangeLog hunks
78+ - fixed conflict in nptl/Makefile
79+
80+Origin: backport, https://sourceware.org/git/?p=glibc.git;a=commit;h=e8c13d5f7a6cd34bc2ac51a0c89fcbbfd2e5c043
81+Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23844
82+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1864864
83+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=952609
84+Last-Update: 2020-03-09
85+
86+diff --git a/nptl/Makefile b/nptl/Makefile
87+index 2d2db648f7..b1003cf56b 100644
88+--- a/nptl/Makefile
89++++ b/nptl/Makefile
90+@@ -309,7 +309,8 @@
91+ tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
92+ tst-robust-fork tst-create-detached tst-memstream \
93+ tst-thread-exit-clobber tst-minstack-cancel tst-minstack-exit \
94+- tst-minstack-throw
95++ tst-minstack-throw \
96++ tst-rwlock-tryrdlock-stall tst-rwlock-trywrlock-stall
97+
98+ tests-internal := tst-rwlock19 tst-rwlock20 \
99+ tst-sem11 tst-sem12 tst-sem13 \
100+diff --git a/nptl/pthread_rwlock_tryrdlock.c b/nptl/pthread_rwlock_tryrdlock.c
101+index 4aec1fc15a..31a88d33a6 100644
102+--- a/nptl/pthread_rwlock_tryrdlock.c
103++++ b/nptl/pthread_rwlock_tryrdlock.c
104+@@ -94,15 +94,22 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
105+ /* Same as in __pthread_rwlock_rdlock_full:
106+ We started the read phase, so we are also responsible for
107+ updating the write-phase futex. Relaxed MO is sufficient.
108+- Note that there can be no other reader that we have to wake
109+- because all other readers will see the read phase started by us
110+- (or they will try to start it themselves); if a writer started
111+- the read phase, we cannot have started it. Furthermore, we
112+- cannot discard a PTHREAD_RWLOCK_FUTEX_USED flag because we will
113+- overwrite the value set by the most recent writer (or the readers
114+- before it in case of explicit hand-over) and we know that there
115+- are no waiting readers. */
116+- atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0);
117++ We have to do the same steps as a writer would when handing over the
118++ read phase to use because other readers cannot distinguish between
119++ us and the writer.
120++ Note that __pthread_rwlock_tryrdlock callers will not have to be
121++ woken up because they will either see the read phase started by us
122++ or they will try to start it themselves; however, callers of
123++ __pthread_rwlock_rdlock_full just increase the reader count and then
124++ check what state the lock is in, so they cannot distinguish between
125++ us and a writer that acquired and released the lock in the
126++ meantime. */
127++ if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0)
128++ & PTHREAD_RWLOCK_FUTEX_USED) != 0)
129++ {
130++ int private = __pthread_rwlock_get_private (rwlock);
131++ futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private);
132++ }
133+ }
134+
135+ return 0;
136+diff --git a/nptl/pthread_rwlock_trywrlock.c b/nptl/pthread_rwlock_trywrlock.c
137+index 5a73eba756..f2e3443466 100644
138+--- a/nptl/pthread_rwlock_trywrlock.c
139++++ b/nptl/pthread_rwlock_trywrlock.c
140+@@ -46,8 +46,15 @@ __pthread_rwlock_trywrlock (pthread_rwlock_t *rwlock)
141+ &rwlock->__data.__readers, &r,
142+ r | PTHREAD_RWLOCK_WRPHASE | PTHREAD_RWLOCK_WRLOCKED))
143+ {
144++ /* We have become the primary writer and we cannot have shared
145++ the PTHREAD_RWLOCK_FUTEX_USED flag with someone else, so we
146++ can simply enable blocking (see full wrlock code). */
147+ atomic_store_relaxed (&rwlock->__data.__writers_futex, 1);
148+- atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1);
149++ /* If we started a write phase, we need to enable readers to
150++ wait. If we did not, we must not change it because other threads
151++ may have set the PTHREAD_RWLOCK_FUTEX_USED in the meantime. */
152++ if ((r & PTHREAD_RWLOCK_WRPHASE) == 0)
153++ atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1);
154+ atomic_store_relaxed (&rwlock->__data.__cur_writer,
155+ THREAD_GETMEM (THREAD_SELF, tid));
156+ return 0;
157+diff --git a/nptl/tst-rwlock-tryrdlock-stall.c b/nptl/tst-rwlock-tryrdlock-stall.c
158+new file mode 100644
159+index 0000000000..5e476da2b8
160+--- /dev/null
161++++ b/nptl/tst-rwlock-tryrdlock-stall.c
162+@@ -0,0 +1,355 @@
163++/* Bug 23844: Test for pthread_rwlock_tryrdlock stalls.
164++ Copyright (C) 2019 Free Software Foundation, Inc.
165++ This file is part of the GNU C Library.
166++
167++ The GNU C Library is free software; you can redistribute it and/or
168++ modify it under the terms of the GNU Lesser General Public
169++ License as published by the Free Software Foundation; either
170++ version 2.1 of the License, or (at your option) any later version.
171++
172++ The GNU C Library is distributed in the hope that it will be useful,
173++ but WITHOUT ANY WARRANTY; without even the implied warranty of
174++ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
175++ Lesser General Public License for more details.
176++
177++ You should have received a copy of the GNU Lesser General Public
178++ License along with the GNU C Library; if not, see
179++ <http://www.gnu.org/licenses/>. */
180++
181++/* For a full analysis see comment:
182++ https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c14
183++
184++ Provided here for reference:
185++
186++ --- Analysis of pthread_rwlock_tryrdlock() stall ---
187++ A read lock begins to execute.
188++
189++ In __pthread_rwlock_rdlock_full:
190++
191++ We can attempt a read lock, but find that the lock is
192++ in a write phase (PTHREAD_RWLOCK_WRPHASE, or WP-bit
193++ is set), and the lock is held by a primary writer
194++ (PTHREAD_RWLOCK_WRLOCKED is set). In this case we must
195++ wait for explicit hand over from the writer to us or
196++ one of the other waiters. The read lock threads are
197++ about to execute:
198++
199++ 341 r = (atomic_fetch_add_acquire (&rwlock->__data.__readers,
200++ 342 (1 << PTHREAD_RWLOCK_READER_SHIFT))
201++ 343 + (1 << PTHREAD_RWLOCK_READER_SHIFT));
202++
203++ An unlock beings to execute.
204++
205++ Then in __pthread_rwlock_wrunlock:
206++
207++ 547 unsigned int r = atomic_load_relaxed (&rwlock->__data.__readers);
208++ ...
209++ 549 while (!atomic_compare_exchange_weak_release
210++ 550 (&rwlock->__data.__readers, &r,
211++ 551 ((r ^ PTHREAD_RWLOCK_WRLOCKED)
212++ 552 ^ ((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0 ? 0
213++ 553 : PTHREAD_RWLOCK_WRPHASE))))
214++ 554 {
215++ ...
216++ 556 }
217++
218++ We clear PTHREAD_RWLOCK_WRLOCKED, and if there are
219++ no readers so we leave the lock in PTHRAD_RWLOCK_WRPHASE.
220++
221++ Back in the read lock.
222++
223++ The read lock adjusts __readres as above.
224++
225++ 383 while ((r & PTHREAD_RWLOCK_WRPHASE) != 0
226++ 384 && (r & PTHREAD_RWLOCK_WRLOCKED) == 0)
227++ 385 {
228++ ...
229++ 390 if (atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers, &r,
230++ 391 r ^ PTHREAD_RWLOCK_WRPHASE))
231++ 392 {
232++
233++ And then attemps to start the read phase.
234++
235++ Assume there happens to be a tryrdlock at this point, noting
236++ that PTHREAD_RWLOCK_WRLOCKED is clear, and PTHREAD_RWLOCK_WRPHASE
237++ is 1. So the try lock attemps to start the read phase.
238++
239++ In __pthread_rwlock_tryrdlock:
240++
241++ 44 if ((r & PTHREAD_RWLOCK_WRPHASE) == 0)
242++ 45 {
243++ ...
244++ 49 if (((r & PTHREAD_RWLOCK_WRLOCKED) != 0)
245++ 50 && (rwlock->__data.__flags
246++ 51 == PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP))
247++ 52 return EBUSY;
248++ 53 rnew = r + (1 << PTHREAD_RWLOCK_READER_SHIFT);
249++ 54 }
250++ ...
251++ 89 while (!atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers,
252++ 90 &r, rnew));
253++
254++ And succeeds.
255++
256++ Back in the write unlock:
257++
258++ 557 if ((r >> PTHREAD_RWLOCK_READER_SHIFT) != 0)
259++ 558 {
260++ ...
261++ 563 if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0)
262++ 564 & PTHREAD_RWLOCK_FUTEX_USED) != 0)
263++ 565 futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private);
264++ 566 }
265++
266++ We note that PTHREAD_RWLOCK_FUTEX_USED is non-zero
267++ and don't wake anyone. This is OK because we handed
268++ over to the trylock. It will be the trylock's responsibility
269++ to wake any waiters.
270++
271++ Back in the read lock:
272++
273++ The read lock fails to install PTHRAD_REWLOCK_WRPHASE as 0 because
274++ the __readers value was adjusted by the trylock, and so it falls through
275++ to waiting on the lock for explicit handover from either a new writer
276++ or a new reader.
277++
278++ 448 int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex,
279++ 449 1 | PTHREAD_RWLOCK_FUTEX_USED,
280++ 450 abstime, private);
281++
282++ We use PTHREAD_RWLOCK_FUTEX_USED to indicate the futex
283++ is in use.
284++
285++ At this point we have readers waiting on the read lock
286++ to unlock. The wrlock is done. The trylock is finishing
287++ the installation of the read phase.
288++
289++ 92 if ((r & PTHREAD_RWLOCK_WRPHASE) != 0)
290++ 93 {
291++ ...
292++ 105 atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0);
293++ 106 }
294++
295++ The trylock does note that we were the one that
296++ installed the read phase, but the comments are not
297++ correct, the execution ordering above shows that
298++ readers might indeed be waiting, and they are.
299++
300++ The atomic_store_relaxed throws away PTHREAD_RWLOCK_FUTEX_USED,
301++ and the waiting reader is never worken becuase as noted
302++ above it is conditional on the futex being used.
303++
304++ The solution is for the trylock thread to inspect
305++ PTHREAD_RWLOCK_FUTEX_USED and wake the waiting readers.
306++
307++ --- Analysis of pthread_rwlock_trywrlock() stall ---
308++
309++ A write lock begins to execute, takes the write lock,
310++ and then releases the lock...
311++
312++ In pthread_rwlock_wrunlock():
313++
314++ 547 unsigned int r = atomic_load_relaxed (&rwlock->__data.__readers);
315++ ...
316++ 549 while (!atomic_compare_exchange_weak_release
317++ 550 (&rwlock->__data.__readers, &r,
318++ 551 ((r ^ PTHREAD_RWLOCK_WRLOCKED)
319++ 552 ^ ((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0 ? 0
320++ 553 : PTHREAD_RWLOCK_WRPHASE))))
321++ 554 {
322++ ...
323++ 556 }
324++
325++ ... leaving it in the write phase with zero readers
326++ (the case where we leave the write phase in place
327++ during a write unlock).
328++
329++ A write trylock begins to execute.
330++
331++ In __pthread_rwlock_trywrlock:
332++
333++ 40 while (((r & PTHREAD_RWLOCK_WRLOCKED) == 0)
334++ 41 && (((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0)
335++ 42 || (prefer_writer && ((r & PTHREAD_RWLOCK_WRPHASE) != 0))))
336++ 43 {
337++
338++ The lock is not locked.
339++
340++ There are no readers.
341++
342++ 45 if (atomic_compare_exchange_weak_acquire (
343++ 46 &rwlock->__data.__readers, &r,
344++ 47 r | PTHREAD_RWLOCK_WRPHASE | PTHREAD_RWLOCK_WRLOCKED))
345++
346++ We atomically install the write phase and we take the
347++ exclusive write lock.
348++
349++ 48 {
350++ 49 atomic_store_relaxed (&rwlock->__data.__writers_futex, 1);
351++
352++ We get this far.
353++
354++ A reader lock begins to execute.
355++
356++ In pthread_rwlock_rdlock:
357++
358++ 437 for (;;)
359++ 438 {
360++ 439 while (((wpf = atomic_load_relaxed (&rwlock->__data.__wrphase_futex))
361++ 440 | PTHREAD_RWLOCK_FUTEX_USED) == (1 | PTHREAD_RWLOCK_FUTEX_USED))
362++ 441 {
363++ 442 int private = __pthread_rwlock_get_private (rwlock);
364++ 443 if (((wpf & PTHREAD_RWLOCK_FUTEX_USED) == 0)
365++ 444 && (!atomic_compare_exchange_weak_relaxed
366++ 445 (&rwlock->__data.__wrphase_futex,
367++ 446 &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED)))
368++ 447 continue;
369++ 448 int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex,
370++ 449 1 | PTHREAD_RWLOCK_FUTEX_USED,
371++ 450 abstime, private);
372++
373++ We are in a write phase, so the while() on line 439 is true.
374++
375++ The value of wpf does not have PTHREAD_RWLOCK_FUTEX_USED set
376++ since this is the first reader to lock.
377++
378++ The atomic operation sets wpf with PTHREAD_RELOCK_FUTEX_USED
379++ on the expectation that this reader will be woken during
380++ the handoff.
381++
382++ Back in pthread_rwlock_trywrlock:
383++
384++ 50 atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1);
385++ 51 atomic_store_relaxed (&rwlock->__data.__cur_writer,
386++ 52 THREAD_GETMEM (THREAD_SELF, tid));
387++ 53 return 0;
388++ 54 }
389++ ...
390++ 57 }
391++
392++ We write 1 to __wrphase_futex discarding PTHREAD_RWLOCK_FUTEX_USED,
393++ and so in the unlock we will not awaken the waiting reader.
394++
395++ The solution to this is to realize that if we did not start the write
396++ phase we need not write 1 or any other value to __wrphase_futex.
397++ This ensures that any readers (which saw __wrphase_futex != 0) can
398++ set PTHREAD_RWLOCK_FUTEX_USED and this can be used at unlock to
399++ wake them.
400++
401++ If we installed the write phase then all other readers are looping
402++ here:
403++
404++ In __pthread_rwlock_rdlock_full:
405++
406++ 437 for (;;)
407++ 438 {
408++ 439 while (((wpf = atomic_load_relaxed (&rwlock->__data.__wrphase_futex))
409++ 440 | PTHREAD_RWLOCK_FUTEX_USED) == (1 | PTHREAD_RWLOCK_FUTEX_USED))
410++ 441 {
411++ ...
412++ 508 }
413++
414++ waiting for the write phase to be installed or removed before they
415++ can begin waiting on __wrphase_futex (part of the algorithm), or
416++ taking a concurrent read lock, and thus we can safely write 1 to
417++ __wrphase_futex.
418++
419++ If we did not install the write phase then the readers may already
420++ be waiting on the futex, the original writer wrote 1 to __wrphase_futex
421++ as part of starting the write phase, and we cannot also write 1
422++ without loosing the PTHREAD_RWLOCK_FUTEX_USED bit.
423++
424++ ---
425++
426++ Summary for the pthread_rwlock_tryrdlock() stall:
427++
428++ The stall is caused by pthread_rwlock_tryrdlock failing to check
429++ that PTHREAD_RWLOCK_FUTEX_USED is set in the __wrphase_futex futex
430++ and then waking the futex.
431++
432++ The fix for bug 23844 ensures that waiters on __wrphase_futex are
433++ correctly woken. Before the fix the test stalls as readers can
434++ wait forever on __wrphase_futex. */
435++
436++#include <stdio.h>
437++#include <stdlib.h>
438++#include <unistd.h>
439++#include <pthread.h>
440++#include <support/xthread.h>
441++#include <errno.h>
442++
443++/* We need only one lock to reproduce the issue. We will need multiple
444++ threads to get the exact case where we have a read, try, and unlock
445++ all interleaving to produce the case where the readers are waiting
446++ and the try fails to wake them. */
447++pthread_rwlock_t onelock;
448++
449++/* The number of threads is arbitrary but empirically chosen to have
450++ enough threads that we see the condition where waiting readers are
451++ not woken by a successful tryrdlock. */
452++#define NTHREADS 32
453++
454++_Atomic int do_exit;
455++
456++void *
457++run_loop (void *arg)
458++{
459++ int i = 0, ret;
460++ while (!do_exit)
461++ {
462++ /* Arbitrarily choose if we are the writer or reader. Choose a
463++ high enough ratio of readers to writers to make it likely
464++ that readers block (and eventually are susceptable to
465++ stalling).
466++
467++ If we are a writer, take the write lock, and then unlock.
468++ If we are a reader, try the lock, then lock, then unlock. */
469++ if ((i % 8) != 0)
470++ xpthread_rwlock_wrlock (&onelock);
471++ else
472++ {
473++ if ((ret = pthread_rwlock_tryrdlock (&onelock)) != 0)
474++ {
475++ if (ret == EBUSY)
476++ xpthread_rwlock_rdlock (&onelock);
477++ else
478++ exit (EXIT_FAILURE);
479++ }
480++ }
481++ /* Thread does some work and then unlocks. */
482++ xpthread_rwlock_unlock (&onelock);
483++ i++;
484++ }
485++ return NULL;
486++}
487++
488++int
489++do_test (void)
490++{
491++ int i;
492++ pthread_t tids[NTHREADS];
493++ xpthread_rwlock_init (&onelock, NULL);
494++ for (i = 0; i < NTHREADS; i++)
495++ tids[i] = xpthread_create (NULL, run_loop, NULL);
496++ /* Run for some amount of time. Empirically speaking exercising
497++ the stall via pthread_rwlock_tryrdlock is much harder, and on
498++ a 3.5GHz 4 core x86_64 VM system it takes somewhere around
499++ 20-200s to stall, approaching 100% stall past 200s. We can't
500++ wait that long for a regression test so we just test for 20s,
501++ and expect the stall to happen with a 5-10% chance (enough for
502++ developers to see). */
503++ sleep (20);
504++ /* Then exit. */
505++ printf ("INFO: Exiting...\n");
506++ do_exit = 1;
507++ /* If any readers stalled then we will timeout waiting for them. */
508++ for (i = 0; i < NTHREADS; i++)
509++ xpthread_join (tids[i]);
510++ printf ("INFO: Done.\n");
511++ xpthread_rwlock_destroy (&onelock);
512++ printf ("PASS: No pthread_rwlock_tryrdlock stalls detected.\n");
513++ return 0;
514++}
515++
516++#define TIMEOUT 30
517++#include <support/test-driver.c>
518+diff --git a/nptl/tst-rwlock-trywrlock-stall.c b/nptl/tst-rwlock-trywrlock-stall.c
519+new file mode 100644
520+index 0000000000..14d27cbcbc
521+--- /dev/null
522++++ b/nptl/tst-rwlock-trywrlock-stall.c
523+@@ -0,0 +1,108 @@
524++/* Bug 23844: Test for pthread_rwlock_trywrlock stalls.
525++ Copyright (C) 2019 Free Software Foundation, Inc.
526++ This file is part of the GNU C Library.
527++
528++ The GNU C Library is free software; you can redistribute it and/or
529++ modify it under the terms of the GNU Lesser General Public
530++ License as published by the Free Software Foundation; either
531++ version 2.1 of the License, or (at your option) any later version.
532++
533++ The GNU C Library is distributed in the hope that it will be useful,
534++ but WITHOUT ANY WARRANTY; without even the implied warranty of
535++ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
536++ Lesser General Public License for more details.
537++
538++ You should have received a copy of the GNU Lesser General Public
539++ License along with the GNU C Library; if not, see
540++ <http://www.gnu.org/licenses/>. */
541++
542++/* For a full analysis see comments in tst-rwlock-tryrdlock-stall.c.
543++
544++ Summary for the pthread_rwlock_trywrlock() stall:
545++
546++ The stall is caused by pthread_rwlock_trywrlock setting
547++ __wrphase_futex futex to 1 and loosing the
548++ PTHREAD_RWLOCK_FUTEX_USED bit.
549++
550++ The fix for bug 23844 ensures that waiters on __wrphase_futex are
551++ correctly woken. Before the fix the test stalls as readers can
552++ wait forever on __wrphase_futex. */
553++
554++#include <stdio.h>
555++#include <stdlib.h>
556++#include <unistd.h>
557++#include <pthread.h>
558++#include <support/xthread.h>
559++#include <errno.h>
560++
561++/* We need only one lock to reproduce the issue. We will need multiple
562++ threads to get the exact case where we have a read, try, and unlock
563++ all interleaving to produce the case where the readers are waiting
564++ and the try clears the PTHREAD_RWLOCK_FUTEX_USED bit and a
565++ subsequent unlock fails to wake them. */
566++pthread_rwlock_t onelock;
567++
568++/* The number of threads is arbitrary but empirically chosen to have
569++ enough threads that we see the condition where waiting readers are
570++ not woken by a successful unlock. */
571++#define NTHREADS 32
572++
573++_Atomic int do_exit;
574++
575++void *
576++run_loop (void *arg)
577++{
578++ int i = 0, ret;
579++ while (!do_exit)
580++ {
581++ /* Arbitrarily choose if we are the writer or reader. Choose a
582++ high enough ratio of readers to writers to make it likely
583++ that readers block (and eventually are susceptable to
584++ stalling).
585++
586++ If we are a writer, take the write lock, and then unlock.
587++ If we are a reader, try the lock, then lock, then unlock. */
588++ if ((i % 8) != 0)
589++ {
590++ if ((ret = pthread_rwlock_trywrlock (&onelock)) != 0)
591++ {
592++ if (ret == EBUSY)
593++ xpthread_rwlock_wrlock (&onelock);
594++ else
595++ exit (EXIT_FAILURE);
596++ }
597++ }
598++ else
599++ xpthread_rwlock_rdlock (&onelock);
600++ /* Thread does some work and then unlocks. */
601++ xpthread_rwlock_unlock (&onelock);
602++ i++;
603++ }
604++ return NULL;
605++}
606++
607++int
608++do_test (void)
609++{
610++ int i;
611++ pthread_t tids[NTHREADS];
612++ xpthread_rwlock_init (&onelock, NULL);
613++ for (i = 0; i < NTHREADS; i++)
614++ tids[i] = xpthread_create (NULL, run_loop, NULL);
615++ /* Run for some amount of time. The pthread_rwlock_tryrwlock stall
616++ is very easy to trigger and happens in seconds under the test
617++ conditions. */
618++ sleep (10);
619++ /* Then exit. */
620++ printf ("INFO: Exiting...\n");
621++ do_exit = 1;
622++ /* If any readers stalled then we will timeout waiting for them. */
623++ for (i = 0; i < NTHREADS; i++)
624++ xpthread_join (tids[i]);
625++ printf ("INFO: Done.\n");
626++ xpthread_rwlock_destroy (&onelock);
627++ printf ("PASS: No pthread_rwlock_tryrwlock stalls detected.\n");
628++ return 0;
629++}
630++
631++#include <support/test-driver.c>
632+diff --git a/support/Makefile b/support/Makefile
633+index 550fdba0f7..b43fa524a7 100644
634+--- a/support/Makefile
635++++ b/support/Makefile
636+@@ -123,6 +123,7 @@ libsupport-routines = \
637+ xpthread_mutexattr_settype \
638+ xpthread_once \
639+ xpthread_rwlock_init \
640++ xpthread_rwlock_destroy \
641+ xpthread_rwlock_rdlock \
642+ xpthread_rwlock_unlock \
643+ xpthread_rwlock_wrlock \
644+diff --git a/support/xpthread_rwlock_destroy.c b/support/xpthread_rwlock_destroy.c
645+new file mode 100644
646+index 0000000000..6d6e953569
647+--- /dev/null
648++++ b/support/xpthread_rwlock_destroy.c
649+@@ -0,0 +1,26 @@
650++/* pthread_rwlock_destroy with error checking.
651++ Copyright (C) 2019 Free Software Foundation, Inc.
652++ This file is part of the GNU C Library.
653++
654++ The GNU C Library is free software; you can redistribute it and/or
655++ modify it under the terms of the GNU Lesser General Public
656++ License as published by the Free Software Foundation; either
657++ version 2.1 of the License, or (at your option) any later version.
658++
659++ The GNU C Library is distributed in the hope that it will be useful,
660++ but WITHOUT ANY WARRANTY; without even the implied warranty of
661++ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
662++ Lesser General Public License for more details.
663++
664++ You should have received a copy of the GNU Lesser General Public
665++ License along with the GNU C Library; if not, see
666++ <http://www.gnu.org/licenses/>. */
667++
668++#include <support/xthread.h>
669++
670++void
671++xpthread_rwlock_destroy (pthread_rwlock_t *rwlock)
672++{
673++ xpthread_check_return ("pthread_rwlock_destroy",
674++ pthread_rwlock_destroy (rwlock));
675++}
676+diff --git a/support/xthread.h b/support/xthread.h
677+index 623f5ad0ac..1af7728087 100644
678+--- a/support/xthread.h
679++++ b/support/xthread.h
680+@@ -84,6 +84,7 @@ void xpthread_rwlockattr_setkind_np (pthread_rwlockattr_t *attr, int pref);
681+ void xpthread_rwlock_wrlock (pthread_rwlock_t *rwlock);
682+ void xpthread_rwlock_rdlock (pthread_rwlock_t *rwlock);
683+ void xpthread_rwlock_unlock (pthread_rwlock_t *rwlock);
684++void xpthread_rwlock_destroy (pthread_rwlock_t *rwlock);
685+
686+ __END_DECLS
687+
688+--
689+2.18.2
690+
691diff --git a/debian/patches/series b/debian/patches/series
692index 5f4e9e3..48d1165 100644
693--- a/debian/patches/series
694+++ b/debian/patches/series
695@@ -147,6 +147,7 @@ sh4/local-fpscr_values.diff
696
697 sparc/submitted-sparc64-socketcall.diff
698
699+all/branch-pthread_rwlock_trywrlock-hang-23844.patch
700 all/local-alias-et_EE.diff
701 all/local-remove-manual.diff
702 all/local-ru_RU.diff

Subscribers

People subscribed via source and target branches