From: Torvald Riegel <triegel@redhat.com>
To: GLIBC Devel <libc-alpha@sourceware.org>
Cc: vl@samba.org, Michael Adam <madam@redhat.com>,
Rich Felker <dalias@libc.org>
Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up.
Date: Thu, 15 Dec 2016 22:29:00 -0000 [thread overview]
Message-ID: <1481840946.14990.588.camel@redhat.com> (raw)
In-Reply-To: <1481840825.14990.586.camel@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 273 bytes --]
On Thu, 2016-12-15 at 23:27 +0100, Torvald Riegel wrote:
> See patch for a description.
>
> Tested on x86_64-linux with our tests and the test case from the
> original bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1401665
>
> OK?
Now with an attached patch...
[-- Attachment #2: robust-mutex-lwu.patch --]
[-- Type: text/x-patch, Size: 4364 bytes --]
commit 74be3b28d962a5a6d2eeff51b93d61ddf91e2d49
Author: Torvald Riegel <triegel@redhat.com>
Date: Thu Dec 15 16:06:28 2016 +0100
Robust mutexes: Fix lost wake-up.
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.
[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
+ 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
newval
@@ -253,7 +258,11 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
}
}
- oldval = LLL_ROBUST_MUTEX_LOCK (mutex, id);
+ oldval = LLL_ROBUST_MUTEX_LOCK (mutex,
+ id | assume_other_futex_waiters);
+ /* See above. We set FUTEX_WAITERS and might have shared this flag
+ with other threads; thus, we need to preserve it. */
+ assume_other_futex_waiters = FUTEX_WAITERS;
if (__builtin_expect (mutex->__data.__owner
== PTHREAD_MUTEX_NOTRECOVERABLE, 0))
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index 07f0901..21e9eea 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -142,13 +142,19 @@ pthread_mutex_timedlock (pthread_mutex_t *mutex,
&mutex->__data.__list.__next);
oldval = mutex->__data.__lock;
+ /* This is set to FUTEX_WAITERS iff we might have shared the
+ 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:
if ((oldval & FUTEX_OWNER_DIED) != 0)
{
/* The previous owner died. Try locking the mutex. */
- int newval = id | (oldval & FUTEX_WAITERS);
+ int newval = id | (oldval & FUTEX_WAITERS)
+ | assume_other_futex_waiters;
newval
= atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
@@ -203,8 +209,12 @@ pthread_mutex_timedlock (pthread_mutex_t *mutex,
}
}
- result = lll_robust_timedlock (mutex->__data.__lock, abstime, id,
+ result = lll_robust_timedlock (mutex->__data.__lock, abstime,
+ id | assume_other_futex_waiters,
PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
+ /* See above. We set FUTEX_WAITERS and might have shared this flag
+ with other threads; thus, we need to preserve it. */
+ assume_other_futex_waiters = FUTEX_WAITERS;
if (__builtin_expect (mutex->__data.__owner
== PTHREAD_MUTEX_NOTRECOVERABLE, 0))
next prev parent reply other threads:[~2016-12-15 22:29 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 [this message]
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
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=1481840946.14990.588.camel@redhat.com \
--to=triegel@redhat.com \
--cc=dalias@libc.org \
--cc=libc-alpha@sourceware.org \
--cc=madam@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).