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]

Re: [PATCH] Fix missing wake-ups in pthread_rwlock_rdlock.


Looks good to me barring a couple of minor nits I've noted below that
you can fix up before committing.

Siddhesh

On Wed, Apr 29, 2015 at 06:25:26PM +0200, Torvald Riegel wrote:
> commit ba34831e62b6041da4f49a2816ea5cbde270332c
> Author: Torvald Riegel <triegel@redhat.com>
> Date:   Tue Apr 28 23:24:36 2015 +0200
> 
>     Fix missing wake-ups in pthread_rwlock_rdlock.
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 7e1897c..422dfb8 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -224,7 +224,7 @@ tests = tst-typesizes \
>  	tst-rwlock1 tst-rwlock2 tst-rwlock2a tst-rwlock3 tst-rwlock4 \
>  	tst-rwlock5 tst-rwlock6 tst-rwlock7 tst-rwlock8 tst-rwlock9 \
>  	tst-rwlock10 tst-rwlock11 tst-rwlock12 tst-rwlock13 tst-rwlock14 \
> -	tst-rwlock15 \
> +	tst-rwlock15 tst-rwlock16 \
>  	tst-once1 tst-once2 tst-once3 tst-once4 \
>  	tst-key1 tst-key2 tst-key3 tst-key4 \
>  	tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \
> diff --git a/nptl/pthread_rwlock_rdlock.c b/nptl/pthread_rwlock_rdlock.c
> index 0edca65..563e841 100644
> --- a/nptl/pthread_rwlock_rdlock.c
> +++ b/nptl/pthread_rwlock_rdlock.c
> @@ -30,6 +30,7 @@ static int __attribute__((noinline))
>  __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
>  {
>    int result = 0;
> +  int wake = 0;

Make this bool.

>  
>    /* Lock is taken in caller.  */
>  
> @@ -81,7 +82,17 @@ __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
>  	      result = EAGAIN;
>  	    }
>  	  else
> -	    LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> +	    {
> +	      LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> +	      /* See pthread_rwlock_rdlock.  */
> +	      if (rwlock->__data.__nr_readers == 1
> +		  && rwlock->__data.__nr_readers_queued > 0
> +		  && rwlock->__data.__nr_writers_queued > 0)
> +		{
> +		  ++rwlock->__data.__readers_wakeup;
> +		  wake = 1;
> +		}
> +	    }
>  
>  	  break;
>  	}
> @@ -90,6 +101,10 @@ __pthread_rwlock_rdlock_slow (pthread_rwlock_t *rwlock)
>    /* We are done, free the lock.  */
>    lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>  
> +  if (wake)
> +    lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> +		    rwlock->__data.__shared);
> +
>    return result;
>  }
>  
> @@ -100,6 +115,7 @@ int
>  __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock)
>  {
>    int result = 0;
> +  int wake = 0;
>  

Likewise.

>    LIBC_PROBE (rdlock_entry, 1, rwlock);
>  
> @@ -126,11 +142,30 @@ __pthread_rwlock_rdlock (pthread_rwlock_t *rwlock)
>  	  result = EAGAIN;
>  	}
>        else
> -	LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> +	{
> +	  LIBC_PROBE (rdlock_acquire_read, 1, rwlock);
> +	  /* If we are the first reader, and there are blocked readers and
> +	     writers (which we don't prefer, see above), then it can be the
> +	     case that we stole the lock from a writer that was already woken
> +	     to acquire it.  That means that we need to take over the writer's
> +	     responsibility to wake all readers (see pthread_rwlock_unlock).
> +	     Thus, wake all readers in this case.  */
> +	  if (rwlock->__data.__nr_readers == 1
> +	      && rwlock->__data.__nr_readers_queued > 0
> +	      && rwlock->__data.__nr_writers_queued > 0)
> +	    {
> +	      ++rwlock->__data.__readers_wakeup;
> +	      wake = 1;
> +	    }
> +	}
>  
>        /* We are done, free the lock.  */
>        lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>  
> +      if (wake)
> +	lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> +			rwlock->__data.__shared);
> +
>        return result;
>      }
>  
> diff --git a/nptl/pthread_rwlock_timedrdlock.c b/nptl/pthread_rwlock_timedrdlock.c
> index 0212b49..f984103 100644
> --- a/nptl/pthread_rwlock_timedrdlock.c
> +++ b/nptl/pthread_rwlock_timedrdlock.c
> @@ -32,6 +32,7 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
>       const struct timespec *abstime;
>  {
>    int result = 0;
> +  int wake = 0;
>

Likewise.

>    /* Make sure we are alone.  */
>    lll_lock(rwlock->__data.__lock, rwlock->__data.__shared);
> @@ -53,6 +54,17 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
>  	      --rwlock->__data.__nr_readers;
>  	      result = EAGAIN;
>  	    }
> +	  else
> +	    {
> +	      /* See pthread_rwlock_rdlock.  */
> +	      if (rwlock->__data.__nr_readers == 1
> +		  && rwlock->__data.__nr_readers_queued > 0
> +		  && rwlock->__data.__nr_writers_queued > 0)
> +		{
> +		  ++rwlock->__data.__readers_wakeup;
> +		  wake = 1;
> +		}
> +	    }
>  
>  	  break;
>  	}
> @@ -153,5 +165,9 @@ pthread_rwlock_timedrdlock (rwlock, abstime)
>    /* We are done, free the lock.  */
>    lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>  
> +  if (wake)
> +    lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> +		    rwlock->__data.__shared);
> +
>    return result;
>  }
> diff --git a/nptl/pthread_rwlock_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c
> index 958d6db..2f71ea2 100644
> --- a/nptl/pthread_rwlock_timedwrlock.c
> +++ b/nptl/pthread_rwlock_timedwrlock.c
> @@ -140,8 +140,16 @@ pthread_rwlock_timedwrlock (rwlock, abstime)
>  	  /* If we prefer writers, it can have happened that readers blocked
>  	     for us to acquire the lock first.  If we have timed out, we need
>  	     to wake such readers if there are any, and if there is no writer
> -	     currently (otherwise, the writer will take care of wake-up).  */
> -	  if (!PTHREAD_RWLOCK_PREFER_READER_P (rwlock)
> +	     currently (otherwise, the writer will take care of wake-up).
> +	     Likewise, even if we prefer readers, we can be responsible for
> +	     wake-up (see pthread_rwlock_unlock) if no reader or writer has
> +	     acquired the lock.  We have timed out and thus not consumed a
> +	     futex wake-up; therefore, if there is no other blocked writer
> +	     that would consume the wake-up and thus take over responsibility,
> +	     we need to wake blocked readers.  */
> +	  if ((!PTHREAD_RWLOCK_PREFER_READER_P (rwlock)
> +	      || ((rwlock->__data.__nr_readers == 0)
> +		  && (rwlock->__data.__nr_writers_queued == 0)))

Indentation of the conditional is incorrect.

>  	      && (rwlock->__data.__nr_readers_queued > 0)
>  	      && (rwlock->__data.__writer == 0))
>  	    {
> diff --git a/nptl/pthread_rwlock_tryrdlock.c b/nptl/pthread_rwlock_tryrdlock.c
> index d51d9aa..1127b35 100644
> --- a/nptl/pthread_rwlock_tryrdlock.c
> +++ b/nptl/pthread_rwlock_tryrdlock.c
> @@ -26,6 +26,7 @@ int
>  __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
>  {
>    int result = EBUSY;
> +  int wake = 0;

Make this bool.

>  
>    if (ELIDE_TRYLOCK (rwlock->__data.__rwelision,
>  		     rwlock->__data.__lock == 0
> @@ -45,11 +46,25 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock)
>  	  result = EAGAIN;
>  	}
>        else
> -	result = 0;
> +	{
> +	  result = 0;
> +	  /* See pthread_rwlock_rdlock.  */
> +	  if (rwlock->__data.__nr_readers == 1
> +	      && rwlock->__data.__nr_readers_queued > 0
> +	      && rwlock->__data.__nr_writers_queued > 0)
> +	    {
> +	      ++rwlock->__data.__readers_wakeup;
> +	      wake = 1;
> +	    }
> +	}
>      }
>  
>    lll_unlock (rwlock->__data.__lock, rwlock->__data.__shared);
>  
> +  if (wake)
> +    lll_futex_wake (&rwlock->__data.__readers_wakeup, INT_MAX,
> +		    rwlock->__data.__shared);
> +
>    return result;
>  }
>  strong_alias (__pthread_rwlock_tryrdlock, pthread_rwlock_tryrdlock)
> diff --git a/nptl/pthread_rwlock_unlock.c b/nptl/pthread_rwlock_unlock.c
> index b8336f6..d2ad4b0 100644
> --- a/nptl/pthread_rwlock_unlock.c
> +++ b/nptl/pthread_rwlock_unlock.c
> @@ -40,8 +40,13 @@ __pthread_rwlock_unlock (pthread_rwlock_t *rwlock)
>      rwlock->__data.__writer = 0;
>    else
>      --rwlock->__data.__nr_readers;
> +  /* If there are still readers present, we do not yet need to wake writers
> +     nor are responsible to wake any readers.  */
>    if (rwlock->__data.__nr_readers == 0)
>      {
> +      /* Note that if there is a blocked writer, we effectively make it
> +	 responsible for waking any readers because we don't wake readers in
> +	 this case.  */
>        if (rwlock->__data.__nr_writers_queued)
>  	{
>  	  ++rwlock->__data.__writer_wakeup;
> diff --git a/nptl/tst-rwlock16.c b/nptl/tst-rwlock16.c
> new file mode 100644
> index 0000000..8e661db
> --- /dev/null
> +++ b/nptl/tst-rwlock16.c
> @@ -0,0 +1,183 @@
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* This tests that with a reader-preferring rwlock, all readers are woken if
> +   one reader "steals" lock ownership from a blocked writer.  */
> +
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <semaphore.h>
> +#include <unistd.h>
> +
> +/* If we strictly prefer writers over readers, a program must not expect
> +   that, in the presence of concurrent writers, one reader will also acquire
> +   the lock when another reader has already done so.  Thus, use the
> +   default rwlock type that does not strictly prefer writers.  */
> +static pthread_rwlock_t r = PTHREAD_RWLOCK_INITIALIZER;
> +
> +static pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t cv = PTHREAD_COND_INITIALIZER;
> +
> +/* Avoid using glibc-internal atomic operations.  */
> +static sem_t stop;
> +static int consumer_stop = 0;
> +
> +static void *
> +writer (void *arg)
> +{
> +  int s;
> +
> +  do
> +    {
> +      if (pthread_rwlock_wrlock (&r) != 0)
> +	{
> +	  puts ("wrlock failed");
> +	  exit (EXIT_FAILURE);
> +	}
> +      if (pthread_rwlock_unlock (&r) != 0)
> +	{
> +	  puts ("unlock failed");
> +	  exit (EXIT_FAILURE);
> +	}
> +      sem_getvalue (&stop, &s);
> +    }
> +  while (s == 0);
> +  return NULL;
> +}
> +
> +static void *
> +reader_producer (void *arg)
> +{
> +  int s;
> +
> +  do
> +    {
> +      if (pthread_rwlock_rdlock (&r) != 0)
> +	{
> +	  puts ("rdlock reader failed");
> +	  exit (EXIT_FAILURE);
> +	}
> +
> +      sem_getvalue (&stop, &s);
> +
> +      pthread_mutex_lock (&m);
> +      if (s != 0)
> +	consumer_stop = 1;
> +      pthread_cond_signal (&cv);
> +      pthread_mutex_unlock (&m);
> +
> +      if (pthread_rwlock_unlock (&r) != 0)
> +	{
> +	  puts ("unlock reader failed");
> +	  exit (EXIT_FAILURE);
> +	}
> +    }
> +  while (s == 0);
> +  puts ("producer finished");
> +  return NULL;
> +}
> +
> +static void *
> +reader_consumer (void *arg)
> +{
> +  int s;
> +
> +  do
> +    {
> +      if (pthread_rwlock_rdlock (&r) != 0)
> +	{
> +	  puts ("rdlock reader failed");
> +	  exit (EXIT_FAILURE);
> +	}
> +
> +      pthread_mutex_lock (&m);
> +      s = consumer_stop;
> +      if (s == 0)
> +	pthread_cond_wait (&cv, &m);
> +      pthread_mutex_unlock (&m);
> +
> +      if (pthread_rwlock_unlock (&r) != 0)
> +	{
> +	  puts ("unlock reader failed");
> +	  exit (EXIT_FAILURE);
> +	}
> +    }
> +  while (s == 0);
> +    puts ("consumer finished");
> +  return NULL;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  pthread_t w1, w2, rp, rc;
> +
> +  if (pthread_create (&w1, NULL, writer, NULL) != 0)
> +    {
> +      puts ("create failed");
> +      return 1;
> +    }
> +  if (pthread_create (&w2, NULL, writer, NULL) != 0)
> +    {
> +      puts ("create failed");
> +      return 1;
> +    }
> +  if (pthread_create (&rc, NULL, reader_consumer, NULL) != 0)
> +    {
> +      puts ("create failed");
> +      return 1;
> +    }
> +  if (pthread_create (&rp, NULL, reader_producer, NULL) != 0)
> +    {
> +      puts ("create failed");
> +      return 1;
> +    }
> +
> +  sleep (2);
> +  sem_post (&stop);
> +
> +  if (pthread_join (w1, NULL) != 0)
> +    {
> +      puts ("w1 join failed");
> +      return 1;
> +    }
> +  if (pthread_join (w2, NULL) != 0)
> +    {
> +      puts ("w2 join failed");
> +      return 1;
> +    }
> +  if (pthread_join (rp, NULL) != 0)
> +    {
> +      puts ("reader_producer join failed");
> +      return 1;
> +    }
> +  if (pthread_join (rc, NULL) != 0)
> +    {
> +      puts ("reader_consumer join failed");
> +      return 1;
> +    }
> +
> +  return 0;
> +}
> +
> +
> +#define TIMEOUT 3
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"

Attachment: pgpXLd6SZ49qx.pgp
Description: PGP signature


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