Re: [patch -rt] Fix infinite loop with 2.6.31.4-rt14 V2

[Darren Hart <dvhltc@us.ibm.com>]

Darren Hart wrote:
> Dinakar Guniguntala wrote:
>  > Application threads calling futex_wait_requeue_pi run in an infinite 
> loop
>  > in the kernel if the futex value changes during the call. The following
>  > patch fixes the problem.
> 
> The key bit here being that EAGAIN == EWOULDBLOCK - who thought that was 
> a good idea?

And now that I think about it, when I reviewed this original patch I
remember mentioning that this isn't even needed for
futex_wait_requeue_pi() because we don't have the same wake-up race as
futex_wait() suffers from - since we don't use the same lock_ptr == NULL
test (nor do we use the wake_list in the requeue code). I suspect the
only case where -EAGAIN is being used here is when the uval doesn't
match val - no spurious wakeups.

Dino, can you try with the following patch which just reverts the
spurious wakeup handling for the requeue_pi path.


>From c21e762bf384e0a559fdf964e0ba7576550d90ec Mon Sep 17 00:00:00 2001
From: Darren Hart <dvhltc@us.ibm.com>
Date: Fri, 23 Oct 2009 16:18:48 -0700
Subject: [PATCH] futex: revert spurious wakeup fix for requeue_pi

The requeue_pi path doesn't use unqueue_me() (and the racy lock_ptr ==
NULL test) nor does it use the wake_list of futex_wake() which led to
the following fix.

41890f2... futex: Handle spurious wake up

See debugging discussing on LKML Message-ID: <4AD4080C.20703@us.ibm.com>

The changes in this fix to the requeue_pi path were considered to be a
likely unecessary, but harmless safety net. Since they are in fact
causing a problem, just remove them and insert a warning in their place.
We can remove the warning later, or even in this commit if folks would
rather.

Signed-off-by: Darren Hart <dvhltc@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Dinakar Guniguntala <dino@in.ibm.com>
CC: John Stultz <johnstul@us.ibm.com>

Witholding CC to stable for further discussion.
---
 kernel/futex.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 7c4a6ac..7e4e8b2 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2085,12 +2085,19 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
 		 */
 		plist_del(&q->list, &q->list.plist);
 
-		/* Handle spurious wakeups gracefully */
-		ret = -EAGAIN;
 		if (timeout && !timeout->task)
 			ret = -ETIMEDOUT;
 		else if (signal_pending(current))
 			ret = -ERESTARTNOINTR;
+		else {
+			/*
+			 * We don't use the racy unqueue_me() path with the
+			 * q.lock_ptr NULL test, nor does requeue use a
+			 * wake_list. All wakeups here should be accounted for.
+			 */
+			printk(KERN_ERR "Spurious wakeup in %s\n",
+			       __FUNCTION__);
+		}
 	}
 	return ret;
 }
@@ -2171,7 +2178,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, int fshared,
 	q.bitset = bitset;
 	q.rt_waiter = &rt_waiter;
 
-retry:
 	key2 = FUTEX_KEY_INIT;
 	ret = get_futex_key(uaddr2, fshared, &key2, VERIFY_WRITE);
 	if (unlikely(ret != 0))
@@ -2264,9 +2270,6 @@ out_put_keys:
 out_key2:
 	put_futex_key(fshared, &key2);
 
-	/* Spurious wakeup ? */
-	if (ret == -EAGAIN)
-		goto retry;
 out:
 	if (to) {
 		hrtimer_cancel(&to->timer);
-- 
1.6.0.4


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team