Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1619214 > unrolled thread

Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Darren Hart <dvhart@infradead.org> - 2017-04-08 04:30 +0200
    Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Mike Galbraith <efault@gmx.de> - 2017-04-08 07:30 +0200
    Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-04-10 10:50 +0200
    Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Peter Zijlstra <peterz@infradead.org> - 2017-04-10 11:10 +0200

#1619214 — Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL

On Wed, Mar 22, 2017 at 11:36:00AM +0100, Peter Zijlstra wrote:
> When PREEMPT_RT_FULL does the spinlock -> rt_mutex substitution the PI
> chain code will (falsely) report a deadlock and BUG.
> 
> The problem is that we hold hb->lock (now an rt_mutex) while doing
> task_blocks_on_rt_mutex on the futex's pi_state::rtmutex. This, when
> interleaved just right with futex_unlock_pi() leads it to believe we
> have an AB-BA deadlock.
> 
>   Task1 (holds rt_mutex,	Task2 (does FUTEX_LOCK_PI)
>          does FUTEX_UNLOCK_PI)
> 
> 				lock hb->lock
> 				lock rt_mutex (as per start_proxy)
>   lock hb->lock
> 
> Which is a trivial AB-BA.
> 
> It is not an actual deadlock, because we won't be holding hb->lock by
> the time we actually block on rt_mutex, but the chainwalk code doesn't
> know that.
> 
> To avoid this problem, do the same thing we do in futex_unlock_pi()
> and drop hb->lock after acquiring wait_lock. This still fully
> serializes against futex_unlock_pi(), since adding to the wait_list
> does the very same lock dance, and removing it holds both locks.
> 
> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I have gone through each of these carefully, and while I'm not naive enough to
say "there are no possible locking problems", each of Peter's claims were
supported by my review. I went down a number of paths which concerned me, but
eventually they each proved not to be a problem, and it was impressive to see
the knot of locks loosen and come free in the last few patches. That's a really
nice piece of work Peter.

I've made several comments on the comment blocks and commit messages to clarify
things where I think they would have saved me time or were inconsistent. I've
only made one code change recommendation iirc, which was the simple type
declaration of a new uval from int to u32.

I would like to see more testing because... well... futexes. But, we don't have
a futex torture suite yet, but that is something I'm hoping to be looking into
in the near future. What testing we do have available has passed between my
futex selftests, the LTP suite, the pi_stress, and the RT runs by Sebastian.

Peter, I presume there will be a v7 with the u32 change and hopefully a couple
text updates?

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [next] | [standalone]

#1619230

On Fri, 2017-04-07 at 19:26 -0700, Darren Hart wrote:

> I would like to see more testing because... well... futexes. But, we don't have
> a futex torture suite yet, but that is something I'm hoping to be looking into
> in the near future. What testing we do have available has passed between my
> futex selftests, the LTP suite, the pi_stress, and the RT runs by Sebastian.

Ditto, tip-rt shows no signs of trouble.

[toc] | [prev] | [next] | [standalone]

#1619725

On 2017-04-07 19:26:10 [-0700], Darren Hart wrote:
> I would like to see more testing because... well... futexes. But, we don't have
> a futex torture suite yet, but that is something I'm hoping to be looking into
> in the near future. What testing we do have available has passed between my
> futex selftests, the LTP suite, the pi_stress, and the RT runs by Sebastian.

It is also part of the latest v4.9-rt release which exposes it to
greater audience.

> Thanks,

Sebastian

[toc] | [prev] | [next] | [standalone]

#1619743

On Fri, Apr 07, 2017 at 07:26:10PM -0700, Darren Hart wrote:
> Peter, I presume there will be a v7 with the u32 change and hopefully a couple
> text updates?

Well, tglx already committed these here patches, so no -v7. What I can
do however is do a follow up patch that fixes some of the in-code things
you mentioned.

Something like the below is what I had lying about from your earlier
emails; I've not looked to see if there's anything else from your later
emails.

---
Subject: futex: Small misc fixes..
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri Apr 7 09:04:07 CEST 2017

Feedback from Darren's review.

Reported-by: Darren Hart (VMWare) <dvhart@infradead.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/futex.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1025,7 +1025,8 @@ static int attach_to_pi_state(u32 __user
 			      struct futex_pi_state **ps)
 {
 	pid_t pid = uval & FUTEX_TID_MASK;
-	int ret, uval2;
+	u32 uval2;
+	int ret;
 
 	/*
 	 * Userspace might have messed up non-PI and PI futexes [3]
@@ -1441,6 +1442,11 @@ static int wake_futex_pi(u32 __user *uad
 	if (ret)
 		goto out_unlock;
 
+	/*
+	 * This is a point of no return; once we modify the uval there is no
+	 * going back and subsequent operations must not fail.
+	 */
+
 	raw_spin_lock(&pi_state->owner->pi_lock);
 	WARN_ON(list_empty(&pi_state->list));
 	list_del_init(&pi_state->list);
@@ -1452,9 +1458,6 @@ static int wake_futex_pi(u32 __user *uad
 	pi_state->owner = new_owner;
 	raw_spin_unlock(&new_owner->pi_lock);
 
-	/*
-	 * We've updated the uservalue, this unlock cannot fail.
-	 */
 	postunlock = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
 
 out_unlock:

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web