This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[RESEND][BZ #14958] Fix Concurrent reader deadlock in pthread_rwlock_rdlock().
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: libc-alpha at sourceware dot org
- Cc: daniel dot stodden at gmail dot com
- Date: Thu, 13 Feb 2014 16:52:31 +0100
- Subject: [RESEND][BZ #14958] Fix Concurrent reader deadlock in pthread_rwlock_rdlock().
- Authentication-results: sourceware.org; auth=none
Hi, this is another bug with patch in bugzilla. I changed there few
style issues. Original description follows:
Rich, I pushed a candidate fix to
https://github.com/dns42/glibc/commit/b7725621b5e2101d4218be0d09bd55a67b4d3512
It explores variant 1 for upstream.
Code presently looks correct to me, but I won't get to test it before
later. I'll post an update accordingly :)
Additional comments for discussion:
- Note that prerequisites can be checked comparatively precisely.
Possibly more precisely than you anticipated, taking
__nr_readers_queued, __nr_writers_queued and __nr_readers
into account, all at the same time.
- This works because both queues need to come together, then
preempted, any other case looks clean to me.
All queued loops take the ll lock and atomically decrement, then
succeed. So the whole combination of __nr_*_queued and __nr_readers
can work as a discriminant.
- As you mentioned, the timedlock cases indeed deserve special
attention.
I think the way they've been written would render the assumptions
made by the above patch correct as well.
But only if rwlock_timedlock calls are really correct.
Can it be that lll_futex_timed_wait does *not*
leave an inherent race between timeout and a (too-)late wakeup call?
In the same way pthread_cond_timedwait must?
I'm not a futex-expert, but the problem here is that I currently
don't believe this is even possible. In other words: How does
present pthread_rwlock_unlock tell that any of __nr_writers_queued
have been woken up before timing out?
I almost suspect it can't, doesn't, and -- since timedrwlock makes
no attempt to restart the queue on timeout either,
suffers from a similar race. But this would probably belong into
another ticket.
Apologies in advance if I'm mistaken.
* nptl/pthread_rwlock_rdlock.c (__pthread_rwlock_rdlock):
Wake threads when necessary.
diff --git a/nptl/pthread_rwlock_rdlock.c b/nptl/pthread_rwlock_rdlock.c
index 3773f7d..b2b679f 100644
--- a/nptl/pthread_rwlock_rdlock.c
+++ b/nptl/pthread_rwlock_rdlock.c
@@ -29,7 +29,7 @@ int
__pthread_rwlock_rdlock (rwlock)
pthread_rwlock_t *rwlock;
{
- int result = 0;
+ int result = 0, wake = 0;
LIBC_PROBE (rdlock_entry, 1, rwlock);
@@ -44,6 +44,14 @@ __pthread_rwlock_rdlock (rwlock)
&& (!rwlock->__data.__nr_writers_queued
|| PTHREAD_RWLOCK_PREFER_READER_P (rwlock)))
{
+ /* May have prempted a queued writer. Since readers are
+ preferred, proceed. But wake any queued readers, too. */
+ wake = !rwlock->__data.__nr_readers
+ && rwlock->__data.__nr_writers_queued
+ && rwlock->__data.__nr_readers_queued;
+ if (__glibc_unlikely (wake))
+ ++rwlock->__data.__readers_wakeup;
+
/* Increment the reader counter. Avoid overflow. */
if (__glibc_unlikely (++rwlock->__data.__nr_readers == 0))
{
@@ -93,6 +101,10 @@ __pthread_rwlock_rdlock (rwlock)
/* We are done, free the lock. */
lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
+ if (__glibc_unlikely (wake))
+ lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
+ rwlock->__data.__shared);
+
return result;
}