From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128200 invoked by alias); 16 Dec 2016 14:11:22 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 128141 invoked by uid 89); 16 Dec 2016 14:11:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=if, blocked, died X-HELO: mx1.redhat.com Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up. To: Torvald Riegel , GLIBC Devel References: <1481840825.14990.586.camel@redhat.com> <1481840946.14990.588.camel@redhat.com> Cc: vl@samba.org, Michael Adam , Rich Felker From: Florian Weimer Message-ID: <7415d701-9dab-3c99-2716-2dcd3a4ac731@redhat.com> Date: Fri, 16 Dec 2016 14:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1481840946.14990.588.camel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2016-12/txt/msg00612.txt.bz2 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