From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14212 invoked by alias); 19 Dec 2016 19:47:26 -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 14198 invoked by uid 89); 19 Dec 2016 19:47:25 -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=switched, riegel, Riegel, oldval X-HELO: mx1.redhat.com Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up. To: Torvald Riegel 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> From: Florian Weimer Cc: GNU C Library , vl@samba.org, Michael Adam , "dalias@libc.org >> Rich Felker" Message-ID: Date: Mon, 19 Dec 2016 19:47: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: <1481926413.14990.628.camel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2016-12/txt/msg00740.txt.bz2 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. 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. Thanks, Florian