Sourceware Bugzilla – Full Text Bug Listing
|Summary:||pthread condvars are not priority inheritance aware|
|Product:||glibc||Reporter:||Darren Hart <dvhltc>|
|Component:||nptl||Assignee:||Not yet assigned to anyone <unassigned>|
|Severity:||normal||CC:||davidh, dino, dvhart, dvhltc, glibc-bugs, gratian.crisan, jan.kiszka, pasky, scot4spam, stano, triegel|
Unbounded priority inversion testcase pthread_cond_hang.c
pi condvar: pthread_condattr_setprotocol_np 2.12 port
pi condvar: add tst-condpi1.c basic API test
pi condvar patch impact on normal path testcase
Unbounded priority inversion test case
[PATCH 1/4 V2] pi-condvars: add protocol support to pthread_condattr_t
[PATCH 2/4 V2] pi-condvars: add tst-condpi1.c, basic API test
[PATCH 3/4 V2] pi-condvars: add tst-condpi2.c, priority inversion avoidance test
[PATCH 4/4 V2] pi-condvars: tst-condpi2.c, detect priority inversion
pi condvar patch impact on normal path testcase
[PATCH 1/3 V3] pi-condvars: add protocol support to pthread_condattr_t
[PATCH 2/3 V3] pi-condvars: add tst-condpi1.c, basic API test
[PATCH 3/3 V3] pi-condvars: add tst-condpi2.c, priority inversion avoidance test
[PATCH 1/3 V3] pi-condvars: add protocol support to pthread_condattr_t
Description Darren Hart 2010-05-11 18:45:58 UTC
When using a PTHREAD_PRIO_INHERIT mutex with a condvar, the pthread_cond* calls can still cause an unbounded priority inversion via the internal condvar lock. The POSIX specification doesn't provide a mechanism to specify the protocol of the condvar. We would like to do this at runtime, but unfortunately it is legal to call pthread_cond_signal() or pthread_cond_broadcast() without first waiting on the lock, so the mutex type may not be known the first time the condvar is used. A new API, pthread_condattr_setprotocol_np() and pthread_condattr_getprotocol_np(), would allow the user to create a PTHREAD_PRIO_INHERIT condvar. This would use a PTHREAD_PRIO_INHERIT mutex for the internal condvar lock, eliminating the potential for hitting an unbounded priority inversion on that lock. This topic was initially discussed here: http://sources.redhat.com/ml/libc-alpha/2010-01/msg00011.html And again here: http://sources.redhat.com/ml/libc-alpha/2010-02/msg00089.html Test cases and patches for evaluation will be attached.
Comment 1 Darren Hart 2010-05-11 18:48:14 UTC
Created attachment 4779 [details] Unbounded priority inversion testcase pthread_cond_hang.c Using vanilla cond vars results in a pthread_cond_wait priority inversion deadlock. With PI cond vars it works fine. This test uses dlsym to use the new API if it exists.
Comment 2 Darren Hart 2010-05-11 18:50:02 UTC
Created attachment 4780 [details] pi condvar: pthread_condattr_setprotocol_np 2.12 port
Comment 3 Darren Hart 2010-05-11 18:50:55 UTC
Created attachment 4781 [details] pi condvar: add tst-condpi1.c basic API test
Comment 4 Darren Hart 2010-05-11 18:55:41 UTC
Created attachment 4782 [details] pi condvar patch impact on normal path testcase This test is designed to test the efficiency of the pthread_cond_wait() and pthread_cond_signal() code paths within glibc. Ulrich has expressed concern about these patches negatively impacting the non-pi condvar paths. I felt the changes to the common case (when PI is not involved) were pretty minimal. Certainly any significant regression there would be unacceptable. I built three versions of glibc 2.11.1 from git: 1) git: unmodified git sources 2) c_only: pthread_cond*.S files deleted 3) pi_condvar: same as c_only with the pi_condvar patches applied Comparing #3 against #2 allows us to eliminate any gains #1 would have solely from the hand written asm. 3 will eventually contain hand written asm, but until the non-posix API is agreed upon, it doesn't make sense to expend the effort of writing the asm code in my opinion. I then ran 10 runs of 10M iterations each at SCHED_FIFO 1 priority on each of the three glibcs, the results (following) suggest no significant change in the non PI condvar performance, sitting right at ~270k (avg) cycles/sec for each glibc. build-x86_64-2.11.1-git Cycles/Second: 279831.187500 Cycles/Second: 261911.421875 Cycles/Second: 277664.125000 Cycles/Second: 284847.718750 Cycles/Second: 285067.281250 Cycles/Second: 267918.718750 Cycles/Second: 284785.656250 Cycles/Second: 277402.843750 Cycles/Second: 202379.703125 Cycles/Second: 266421.718750 Min: 202379.703125 us Max: 285067.28125 us Avg: 268823.0375 us build-x86_64-2.11.1-c_only Cycles/Second: 277931.781250 Cycles/Second: 275614.093750 Cycles/Second: 271194.125000 Cycles/Second: 280155.093750 Cycles/Second: 284708.156250 Cycles/Second: 190936.031250 Cycles/Second: 264253.468750 Cycles/Second: 281354.281250 Cycles/Second: 290366.218750 Cycles/Second: 279962.000000 Min: 190936.03125 us Max: 290366.21875 us Avg: 269647.525 us build-x86_64-2.11.1-pi_condvar Cycles/Second: 263975.937500 Cycles/Second: 279577.281250 Cycles/Second: 276504.531250 Cycles/Second: 266163.562500 Cycles/Second: 262115.796875 Cycles/Second: 279219.406250 Cycles/Second: 265263.812500 Cycles/Second: 262226.468750 Cycles/Second: 284592.687500 Cycles/Second: 278975.875000 Min: 262115.796875 us Max: 284592.6875 us Avg: 271861.535938 us This is only the cond_signal case, and doesn't account for cond_timedwait or cond_broadcast, but I wouldn't expect those to experience any additional impact from this patch. Are there scenarios you can think of that are likely to suffer greater impact that are not covered by this rather simple test case?
Comment 5 Darren Hart 2010-05-13 01:55:07 UTC
While working up tst-condpi2.c (make check version of pthread_cond_hang.c), I found something went wrong with my port attached as "0001-pi-condvar- pthread_condattr_setprotocol_np-2.12-port.patch" and the test case still hangs when affined to CPU 0. I'll look into why and get an updated patch posted as soon as possible.
Comment 6 Darren Hart 2010-05-13 08:02:54 UTC
Turns out the trouble in comment 5 was due to my forgetting to remove the nptl/pthread_cond*.S files before building the library this time around. The original 0001 patch did indeed function correctly. I have reworked the patches, improved the patch headers and naming as well as added a new tst-condpi2.c testcase which tests for priority inversion avoidance. A follow-on patch detects an unbounded inversion and exits gracefully. Without the last patch, the glibc test framework will kill the test after the 2 second timeout - which may be the preferred approach. I'll attach the updated patches now.
Comment 7 Darren Hart 2010-05-13 08:05:03 UTC
Created attachment 4785 [details] Unbounded priority inversion test case The first version (attachment 4779 [details]) was missing the "_np" in the dl lookup. This fixes that.
Comment 8 Darren Hart 2010-05-13 08:07:07 UTC
Created attachment 4786 [details] [PATCH 1/4 V2] pi-condvars: add protocol support to pthread_condattr_t Updated patch header. Functionally equivalent to attachment 4780 [details].
Comment 9 Darren Hart 2010-05-13 08:08:18 UTC
Created attachment 4787 [details] [PATCH 2/4 V2] pi-condvars: add tst-condpi1.c, basic API test Updated patch header.
Comment 10 Darren Hart 2010-05-13 08:10:40 UTC
Created attachment 4788 [details] [PATCH 3/4 V2] pi-condvars: add tst-condpi2.c, priority inversion avoidance test New "make check" test, tst-condpi2.c, to test priority inversion avoidance.
Comment 11 Darren Hart 2010-05-13 08:13:15 UTC
Created attachment 4789 [details] [PATCH 4/4 V2] pi-condvars: tst-condpi2.c, detect priority inversion Optional patch. This goes beyond relying on the glibc 2 second timeout to abort the test. Instead, it detects the unbounded priority inversion and exits gracefully. Depending on the preferred approach, this patch can be ignored without a problem.
Comment 12 Darren Hart 2010-05-13 18:44:41 UTC
It was brought to my attention that the Signed-off-by's in the patches attached and sent to libc-alpha are not appropriate for glibc development. This was just an artifact of my git usage, and I will correct in subsequent versions. May we discuss these as is, as we our foremost concern is agreeing on the proposed new condattr protocol APIs?
Comment 13 Darren Hart 2010-05-27 23:18:55 UTC
I have posted V3 of the patches to libc-alpha. Rather than bog bugzilla down with additional attachments, I'll wait until an agreement has been made on libc-alpha. These patches have proper ChangeLogs and have removed the requeue-pi C implementation which the previous versions were carrying along.
Comment 14 Darren Hart 2010-05-28 20:34:43 UTC
Created attachment 4821 [details] pi condvar patch impact on normal path testcase Updated condvar_perf testcase which runs the previous test multiple times and calculates various statistics to aid in determining the impact of the pi_condvar patches.
Comment 15 Darren Hart 2010-06-17 20:59:45 UTC
Ulrich, before I spend more time on the test cases and possibly trying to optimize the current implementation, can you comment on if you have any objection to the proposed API changes themselves?
Comment 16 Thomas Gleixner 2010-06-17 22:17:01 UTC
(In reply to comment #15) > Ulrich, before I spend more time on the test cases and possibly trying to > optimize the current implementation, can you comment on if you have any objection > to the proposed API changes themselves? Is there any chance that this issue is going to make any progress ? It affects real users and we have a working solution for it. Are there any issues with the approach itself ? If not, what needs to be done to get this resolved ? Thanks, tglx
Comment 17 Jan Kiszka 2011-02-14 17:28:26 UTC
(In reply to comment #16) > Is there any chance that this issue is going to make any progress ? It affects > real users and we have a working solution for it. I can only confirm this. We just saw that unfortunate deadlock again in real application (something to be shipped in products). We ran into it before, but this time we finally realized that it's not due to an outdated glibc but rather a still unfixed bug. Our applications consist of dozens of real-time threads at different prio levels and practically can't be reworked to avoid cond variables in RT threads. So we really need a solution, and that preferably without having to carry our own patched glibc versions. Thanks, Jan
Comment 18 Jan Kiszka 2012-06-05 16:36:45 UTC
Ping on this unfixed bug. Is there a chance to reconsider the extension Darren proposed? We are stuck with unhandy and inefficient PRIO_PROTECT here due to this issue. And I'm sure we aren't alone...
Comment 19 Torvald Riegel 2012-10-25 11:03:02 UTC
Comment 20 Gratian Crisan 2013-10-17 21:16:18 UTC
Created attachment 7238 [details] [PATCH 1/3 V3] pi-condvars: add protocol support to pthread_condattr_t Changes since V2: * Ported to glibc 2.19 * Dropped the changes related to making the external mutex associated with the condvar PI-aware since they have been already committed. * Moved lll_pi_lock()/lll_pi_unlock() to lowlevelpilock.c * Changed the value bitmap in pthread_condattr to use only 1 bit for clock ID This matches the equivalent bitmap in the __nwaiters field. * Used the new condattr masks and shifts where appropriate
Comment 21 Gratian Crisan 2013-10-17 21:22:33 UTC
Created attachment 7239 [details] [PATCH 2/3 V3] pi-condvars: add tst-condpi1.c, basic API test Changes since V2: * updated copyright to 2013 and tested on glibc 2.19 * functional equivalent to V2
Comment 22 Gratian Crisan 2013-10-17 21:24:37 UTC
Created attachment 7240 [details] [PATCH 3/3 V3] pi-condvars: add tst-condpi2.c, priority inversion avoidance test Changes since V2: * updated copyright to 2013 and tested on glibc 2.19 * functional equivalent to V2
Comment 23 Gratian Crisan 2013-11-15 17:36:24 UTC
Created attachment 7282 [details] [PATCH 1/3 V3] pi-condvars: add protocol support to pthread_condattr_t Updated with lates changes suggested on libc-alpha: https://sourceware.org/ml/libc-alpha/2013-10/msg00731.html