From: Florian Weimer <fweimer@redhat.com>
To: Torvald Riegel <triegel@redhat.com>
Cc: GNU C Library <libc-alpha@sourceware.org>,
vl@samba.org, Michael Adam <madam@redhat.com>,
"dalias@libc.org >> Rich Felker" <dalias@libc.org>
Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up.
Date: Mon, 19 Dec 2016 19:47:00 -0000 [thread overview]
Message-ID: <f44ec699-6157-c275-c7de-e88a119e5f3a@redhat.com> (raw)
In-Reply-To: <1481926413.14990.628.camel@redhat.com>
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
next prev parent reply other threads:[~2016-12-19 19:47 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-15 22:27 Torvald Riegel
2016-12-15 22:29 ` Torvald Riegel
2016-12-16 14:11 ` Florian Weimer
2016-12-16 14:45 ` Volker Lendecke
[not found] ` <CAJ+X7mT1cU1_2ON2JZM9oYMP_cak734tkf+PZJeo4MZg1i4gmw@mail.gmail.com>
2016-12-19 17:15 ` Florian Weimer
2016-12-16 22:13 ` Torvald Riegel
2016-12-19 19:47 ` Florian Weimer [this message]
2016-12-19 20:30 ` Carlos O'Donell
2016-12-20 15:03 ` Torvald Riegel
2016-12-19 18:20 ` Carlos O'Donell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f44ec699-6157-c275-c7de-e88a119e5f3a@redhat.com \
--to=fweimer@redhat.com \
--cc=dalias@libc.org \
--cc=libc-alpha@sourceware.org \
--cc=madam@redhat.com \
--cc=triegel@redhat.com \
--cc=vl@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).