This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PING][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: Wed, 19 Feb 2014 14:00:37 +0100
- Subject: [PING][RESEND][BZ #14958] Fix Concurrent reader deadlock in pthread_rwlock_rdlock().
- Authentication-results: sourceware.org; auth=none
- References: <20140213155231 dot GA16546 at domone dot podge>
ping
On Thu, Feb 13, 2014 at 04:52:31PM +0100, OndÅej BÃlka wrote:
> 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;
> }
>
--
manager in the cable duct