Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up.

[Florian Weimer <fweimer at redhat dot com>]

On 12/15/2016 11:29 PM, Torvald Riegel wrote:
>     Assume that Thread 1 waits to acquire a robust mutex using futexes to
>     block (and thus sets the FUTEX_WAITERS flag), and is unblocked when this
>     mutex is released.  If Thread 2 concurrently acquires the lock and is
>     killed, Thread 1 can recover from the died owner but fail to restore the
>     FUTEX_WAITERS flag.  This can lead to a Thread 3 that also blocked using
>     futexes at the same time as Thread 1 to not get woken up because
>     FUTEX_WAITERS is not set anymore.
>
>     The fix for this is to ensure that we continue to preserve the
>     FUTEX_WAITERS flag whenever we may have set it or shared it with another
>     thread.  This is the same requirement as in the algorithm for normal
>     mutexes, only that the robust mutexes need additional handling for died
>     owners and thus preserving the FUTEX_WAITERS flag cannot be done just in
>     the futex slowpath code.

Description and change look good to me in general.

>     	[BZ #20973]
>     	* nptl/pthread_mutex_lock.c (__pthread_mutex_lock_full): Fix lost
>     	wake-up in robust mutexes.
>     	* nptl/pthread_mutex_timedlock.c (pthread_mutex_timedlock): Likewise.
>
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index bdfa529..01ac75e 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -182,6 +182,11 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>  		     &mutex->__data.__list.__next);
>
>        oldval = mutex->__data.__lock;
> +      /* This is set to FUTEX_WAITERS iff we might have shared the

“iff” doesn't seem to be correct here because it's not an exact 
equivalence, “if” is sufficient.

> +	 FUTEX_WAITERS flag with other threads, and therefore need to keep it
> +	 set to avoid lost wake-ups.  We have the same requirement in the
> +	 simple mutex algorithm.  */
> +      unsigned int assume_other_futex_waiters = 0;
>        do
>  	{
>  	again:
> @@ -190,9 +195,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>  	      /* The previous owner died.  Try locking the mutex.  */
>  	      int newval = id;
>  #ifdef NO_INCR
> -	      newval |= FUTEX_WAITERS;
> +	      newval |= FUTEX_WAITERS | assume_other_futex_waiters;
>  #else
> -	      newval |= (oldval & FUTEX_WAITERS);
> +	      newval |= (oldval & FUTEX_WAITERS) | assume_other_futex_waiters;
>  #endif

The NO_INCR change is quite confusing.  Perhaps drop it and add a comment?

VL, what is the copyright status of your test case?

Thanks,
Florian