This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PING][RESEND][BZ #14958] Fix Concurrent reader deadlock in pthread_rwlock_rdlock().


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]