From: Darren Hart <dvhart@infradead.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: tglx@linutronix.de, mingo@kernel.org, juri.lelli@arm.com,
rostedt@goodmis.org, xlpang@redhat.com, bigeasy@linutronix.de,
linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
jdesfossez@efficios.com, bristot@redhat.com
Subject: Re: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from under hb->lock
Date: Wed, 5 Apr 2017 16:52:25 -0700 [thread overview]
Message-ID: <20170405235225.GD13494@fury> (raw)
In-Reply-To: <20170322104151.900002056@infradead.org>
On Wed, Mar 22, 2017 at 11:35:55AM +0100, Peter Zijlstra wrote:
> There's a number of 'interesting' problems, all caused by holding
> hb->lock while doing the rt_mutex_unlock() equivalient.
>
> Notably:
>
> - a PI inversion on hb->lock; and,
>
> - a DL crash because of pointer instability.
A DL crash? What is this? Can you elaborate a bit?
>
> Because of all the previous patches that:
>
> - allow us to do rt_mutex_futex_unlock() without dropping wait_lock;
> which in turn allows us to rely on wait_lock atomicy.
>
> - changed locking rules to cover {uval,pi_state} with wait_lock.
>
> - simplified the waiter conundrum.
>
> We can now quite simply pull rt_mutex_futex_unlock() out from under
> hb->lock, a pi_state reference and wait_lock are sufficient.
OK, owe. I think I've traced most of this through. I have a few gray areas
still, and will continue through the series to see if that addresses them.
A few thoughts as they occurred to me below.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> kernel/futex.c | 154 +++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 100 insertions(+), 54 deletions(-)
>
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
...
> @@ -1380,48 +1387,40 @@ static void mark_wake_futex(struct wake_
> smp_store_release(&q->lock_ptr, NULL);
> }
>
> -static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *top_waiter,
> - struct futex_hash_bucket *hb)
> +/*
> + * Caller must hold a reference on @pi_state.
> + */
> +static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state)
> {
> - struct task_struct *new_owner;
> - struct futex_pi_state *pi_state = top_waiter->pi_state;
> u32 uninitialized_var(curval), newval;
> + struct task_struct *new_owner;
> + bool deboost = false;
> DEFINE_WAKE_Q(wake_q);
> - bool deboost;
Nit: Based on what I've seen from Thomas and others, I ask for declarations in
decreasing order of line length. So deboost should have stayed where it was.
>
> /*
> @@ -2232,7 +2229,8 @@ static int fixup_pi_state_owner(u32 __us
> /*
> * We are here either because we stole the rtmutex from the
> * previous highest priority waiter or we are the highest priority
> - * waiter but failed to get the rtmutex the first time.
> + * waiter but have failed to get the rtmutex the first time.
> + *
> * We have to replace the newowner TID in the user space variable.
> * This must be atomic as we have to preserve the owner died bit here.
> *
> @@ -2249,7 +2247,7 @@ static int fixup_pi_state_owner(u32 __us
> if (get_futex_value_locked(&uval, uaddr))
> goto handle_fault;
>
> - while (1) {
> + for (;;) {
As far as I'm aware, there is no difference and both are used throughout the
kernel (with the while version having 50% more instances). Is there more to this
than personal preference?
> newval = (uval & FUTEX_OWNER_DIED) | newtid;
>
> if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
> @@ -2345,6 +2343,10 @@ static int fixup_owner(u32 __user *uaddr
> /*
> * Got the lock. We might not be the anticipated owner if we
> * did a lock-steal - fix up the PI-state in that case:
> + *
> + * We can safely read pi_state->owner without holding wait_lock
> + * because we now own the rt_mutex, only the owner will attempt
> + * to change it.
This seems to contradict the Serialization and lifetime rules:
+ * pi_mutex->wait_lock:
+ *
+ * {uval, pi_state}
+ *
+ * (and pi_mutex 'obviously')
It would seem that simply holding pi_mutex is sufficient for serialization on
pi_state->owner then.
...
> @@ -2738,10 +2748,36 @@ static int futex_unlock_pi(u32 __user *u
> */
> top_waiter = futex_top_waiter(hb, &key);
> if (top_waiter) {
> - ret = wake_futex_pi(uaddr, uval, top_waiter, hb);
> + struct futex_pi_state *pi_state = top_waiter->pi_state;
> +
> + ret = -EINVAL;
> + if (!pi_state)
> + goto out_unlock;
> +
> + /*
> + * If current does not own the pi_state then the futex is
> + * inconsistent and user space fiddled with the futex value.
> + */
> + if (pi_state->owner != current)
> + goto out_unlock;
> +
> + /*
> + * Grab a reference on the pi_state and drop hb->lock.
> + *
> + * The reference ensures pi_state lives, dropping the hb->lock
> + * is tricky.. wake_futex_pi() will take rt_mutex::wait_lock to
> + * close the races against futex_lock_pi(), but in case of
> + * _any_ fail we'll abort and retry the whole deal.
s/fail/failure/
--
Darren Hart
VMware Open Source Technology Center
next prev parent reply other threads:[~2017-04-05 23:52 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-22 10:35 [PATCH -v6 00/13] The arduous story of FUTEX_UNLOCK_PI Peter Zijlstra
2017-03-22 10:35 ` [PATCH -v6 01/13] futex: Cleanup variable names for futex_top_waiter() Peter Zijlstra
2017-03-23 18:19 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-24 21:11 ` [PATCH -v6 01/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 02/13] futex: Use smp_store_release() in mark_wake_futex() Peter Zijlstra
2017-03-23 18:19 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-24 21:16 ` [PATCH -v6 02/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 03/13] futex: Remove rt_mutex_deadlock_account_*() Peter Zijlstra
2017-03-23 18:20 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-24 21:29 ` [PATCH -v6 03/13] " Darren Hart
2017-03-24 21:31 ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific rt_mutex API Peter Zijlstra
2017-03-23 18:20 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-25 0:37 ` [PATCH -v6 04/13] " Darren Hart
2017-04-06 12:15 ` Peter Zijlstra
2017-04-06 17:02 ` Darren Hart
2017-04-05 15:02 ` Darren Hart
2017-04-06 12:17 ` Peter Zijlstra
2017-04-06 17:08 ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 05/13] futex: Change locking rules Peter Zijlstra
2017-03-23 18:21 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 21:18 ` [PATCH -v6 05/13] " Darren Hart
2017-04-06 12:28 ` Peter Zijlstra
2017-04-06 15:58 ` Joe Perches
2017-04-06 17:21 ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 06/13] futex: Cleanup refcounting Peter Zijlstra
2017-03-23 18:21 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 21:29 ` [PATCH -v6 06/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 07/13] futex: Rework inconsistent rt_mutex/futex_q state Peter Zijlstra
2017-03-23 18:22 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 21:58 ` [PATCH -v6 07/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from under hb->lock Peter Zijlstra
2017-03-23 18:22 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 23:52 ` Darren Hart [this message]
2017-04-06 12:42 ` [PATCH -v6 08/13] " Peter Zijlstra
2017-04-06 17:42 ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 09/13] futex,rt_mutex: Introduce rt_mutex_init_waiter() Peter Zijlstra
2017-03-23 18:23 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 23:57 ` [PATCH -v6 09/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 10/13] futex,rt_mutex: Restructure rt_mutex_finish_proxy_lock() Peter Zijlstra
2017-03-23 18:23 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-07 23:30 ` [PATCH -v6 10/13] " Darren Hart
2017-04-07 23:35 ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 11/13] futex: Rework futex_lock_pi() to use rt_mutex_*_proxy_lock() Peter Zijlstra
2017-03-23 18:24 ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-08 0:55 ` [PATCH -v6 11/13] " Darren Hart
2017-04-10 15:51 ` alexander.levin
2017-04-10 16:03 ` Thomas Gleixner
2017-04-14 9:30 ` [tip:locking/core] futex: Avoid freeing an active timer tip-bot for Thomas Gleixner
2017-03-22 10:35 ` [PATCH -v6 12/13] futex: futex_unlock_pi() determinism Peter Zijlstra
2017-03-23 18:24 ` [tip:locking/core] futex: Futex_unlock_pi() determinism tip-bot for Peter Zijlstra
2017-04-08 1:27 ` [PATCH -v6 12/13] futex: futex_unlock_pi() determinism Darren Hart
2017-03-22 10:36 ` [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Peter Zijlstra
2017-03-23 18:25 ` [tip:locking/core] futex: Drop hb->lock before enqueueing on the rtmutex tip-bot for Peter Zijlstra
2017-04-08 2:26 ` [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Darren Hart
2017-04-08 5:22 ` Mike Galbraith
2017-04-10 8:43 ` Sebastian Andrzej Siewior
2017-04-10 9:08 ` Peter Zijlstra
2017-04-10 16:05 ` Darren Hart
2017-03-24 1:45 ` [PATCH -v6 00/13] The arduous story of FUTEX_UNLOCK_PI Darren Hart
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=20170405235225.GD13494@fury \
--to=dvhart@infradead.org \
--cc=bigeasy@linutronix.de \
--cc=bristot@redhat.com \
--cc=jdesfossez@efficios.com \
--cc=juri.lelli@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=xlpang@redhat.com \
/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 NNTP newsgroup(s).