From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38188 invoked by alias); 20 Dec 2016 15:03:28 -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 36910 invoked by uid 89); 20 Dec 2016 15:03:27 -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= X-HELO: mx1.redhat.com Message-ID: <1482246196.14990.698.camel@redhat.com> Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up. From: Torvald Riegel To: Florian Weimer Cc: GNU C Library , vl@samba.org, Michael Adam , "dalias@libc.org >> Rich Felker" Date: Tue, 20 Dec 2016 15:03:00 -0000 In-Reply-To: References: <1481840825.14990.586.camel@redhat.com> <1481840946.14990.588.camel@redhat.com> <7415d701-9dab-3c99-2716-2dcd3a4ac731@redhat.com> <1481926413.14990.628.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-12/txt/msg00765.txt.bz2 On Mon, 2016-12-19 at 20:47 +0100, Florian Weimer wrote: > On 12/16/2016 11:13 PM, Torvald Riegel wrote: > > On Fri, 2016-12-16 at 15:11 +0100, Florian Weimer wrote: > >> On 12/15/2016 11:29 PM, Torvald Riegel wrote: > >>> 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. > > > > No, I think the iff is correct. We do only set it if we may have shared > > the flag. > > Then please change it to “This is set to FUTEX_WAITERS iff we have > shared” (i.e. drop the “might”). Based on the source code, I'm still > not sure if this is an exact equivalence. I still think the original comment is correct. When we start, we will not have shared, so assume_... starts as false. After we have set F_W, we *might* have shared, so we set assume_... to true. You're right that we might have just succeeded to acquire the lock, and then we might not have shared, but then we're not using assume_... anymore. > The part which confuses me is the unconditional assignment > assume_other_futex_waiters = FUTEX_WAITERS further below. But I think > lll_robust_lock returns 0 if we did not share FUTEX_WAITERS, and the > code never retries with the assigned assume_other_futex_waiters value, > ensuring the equivalence. I think it would be clearer if you switched > from a do-while loop to a loop with an exit condition in the middle, > right after the call to lll_robust_lock. > > Putting the FUTEX_WAITERS into the ID passed to lll_robust_lock is a > violation of its precondition documented in sysdeps/nptl/lowlevellock.h, > so please update the comment. I've already committed the patch after what sounded like approval. As I've said before, I plan to follow this up with a cleanup of the whole robust mutex code. In that cleanup patch we can fine-tune the documentation.