From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12522 invoked by alias); 19 Dec 2016 20:30:51 -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 12508 invoked by uid 89); 19 Dec 2016 20:30:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy=loose, restructuring, concrete X-HELO: mail-it0-f54.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-transfer-encoding; bh=UGvXPr6YYCMKjRzzmO0FfUuYS5D4XdiX0KU1ssmrYd0=; b=tY64fB8C4NSfnjH/RApp37/5TYfOA0vdXfvCwC4tg/NCt+sD8TRdRIrjngLxj4wn9B s75VEc8RRU3seJ1ebfZGHOWVUK1wl/kRMaKxp6Jli9B0GCghEidNsGDD7wlZxkjaNCD9 Xo3Wxdxp32aot82hunWsxJ76XjEi6eVjG52NJohrBDyPJ4hHzJleLmLM4JXoZc9ikOsH f7fkU5EuR2iDs07LlN/HmY4T8gNAyXbDTUtTWQwE1sv+IujF/EhorFy+MYyp3w7ITLl/ hvJmBnJX+jFBm1p5CB2yq893ksM8WhbHHQfZCAnwAxjBziuJCvQkJRYSADYfiNz4VSYG NnSQ== X-Gm-Message-State: AKaTC018ymsxTUEWxlzBbOcTWRDPVf4jasUVc8pVq2YrFGuqWo5WAveQ3pq+eLHttxuOwgk+ X-Received: by 10.36.68.130 with SMTP id o124mr17676705ita.62.1482179448292; Mon, 19 Dec 2016 12:30:48 -0800 (PST) Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up. To: Florian Weimer , 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> Cc: GNU C Library , vl@samba.org, Michael Adam , "dalias@libc.org >> Rich Felker" From: Carlos O'Donell Message-ID: <2a3eb419-18ec-13a1-11c5-5b475379a5f3@redhat.com> Date: Mon, 19 Dec 2016 20:30: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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-12/txt/msg00742.txt.bz2 On 12/19/2016 02:47 PM, 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. OK. I agree I'm also not sure it's an exact equivalence, I'd also probably say "This _will_ be set to..." since technically we share FUTEX_WAITERS in the inner __lll_robust_lock_wait before we come back and set assume_other_futex_waiters. > The part which confuses me is the unconditional assignment > assume_other_futex_waiters = FUTEX_WAITERS further below. It _must_ be assigned unconditionally exactly because of the problem outlined in this issue (and expanded in my review). Because the value of FUTEX_WAITERS is shared, and because of the recovery semantics of the mutex, there is a path through __lll_robust_lock_wait which needs to avoid the loss of FUTEX_WAITERS (and the required wakeup which happens on unlock). Because the unlock and wakeup by T0 clears the FUTEX_WAITERS flag, and you don't know if other threads queued up after you, you must assume the worst and do the wakeup also. I don't see a way to avoid the spurious wakeup. > 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. Yes, this is a case where assume_other_futex_waiters is zero and we have potentially shared FUTEX_WAITERS via the inner __lll_robust_lock_wait code, so it would appear this example contradicts using "iff". I don't know about restructuring this loop. I'd have to see a concrete example of the cleanup, and I think I'd like to avoid such a cleanup to keep this patch minimal for now. Future cleanup could happen in a lot of ways. > 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. The precondition is a bit loose about this, but I agree the language could be tightened up. -- Cheers, Carlos.