Home » Mailing list archives » linux-kernel » 2010 » April » 16

Re: Possible bug with mutex adaptative spinning

Previous thread: [PATCH][resend] namei.c : update mnt when it needed by Huang Shijie on Tuesday, April 13, 2010 - 7:16 pm. (2 messages)

Next thread: Security: Replace dac_mmap_min_addr to mmap_min_addr in cap_file_mmap() by wzt.wzt on Tuesday, April 13, 2010 - 7:48 pm. (4 messages)
[view original message]
From: Benjamin Herrenschmidt
Subject: Possible bug with mutex adaptative spinning
Date: Tuesday, April 13, 2010 - 7:35 pm

Hi Peter !

I -may- have found a bug with mutex adaptative spinning. We hit it when
torture testing CPU unplug.

Basically, what happens is mutex_spin_on_owner() returns 1 if the owner
CPU is offline. That means that the caller (__mutex_lock_common()) will
spin until the mutex is released since there's a valid owner (so the
need_resched() test doesn't trigger). This will deadlock if this is the
only remaining CPU online.

In fact, it even deadlocks with more than 1 CPU online, for example, I
have a case where the 2 remaining online CPUs got into
__mutex_lock_common() at the same time, and so got into that spin loop
before any of them added itself to the wait_list, thus never hitting the
exist condition there.

I believe your test against nr_cpumask_bits is also a bit fragile for
similar reasons. IE. You have what looks like a valid owner but it seems
to be on an invalid CPU. It could be a freed thread_info, in which case
returning 1 is fine, but if it's anything else, kaboom. I think it's
better to be safe than sorry here and just go to sleep (ie return 0).

Same comment with the DEBUG_PAGE_ALLOC case. In fact, this (untested)
patch makes the function simpler and I don't think will have any
negative effect on performances. Let me know what you think:

---- cut here ----

[PATCH] mutex: Don't spin when the owner CPU is offline or other weird cases

The current code might spin forever if the CPU owning the mutex has been
offlined, and the last CPU in the system is trying to acquire it, since
mutex_spin_on_owner() will always return 1, telling the caller to spin
until the mutex has been released.

This patch changes mutex_spin_on_owner() to return 0 (don't spin) in any
case where we aren't sure about the owner struct validity or CPU number,
and if the said CPU is offline. There is no point going back &
re-evaluate spinning in corner cases like that, let's just go to sleep.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

diff --git ...
[ message continues ]
[view original message]
From: Benjamin Herrenschmidt
Subject: Re: Possible bug with mutex adaptative spinning
Date: Tuesday, April 13, 2010 - 7:56 pm

.../...

In fact, I wonder if there's another potential problem:

If the owner is actually running, it may do so for a very long time. It
looks to me that everybody trying to take the mutex will thus spin and
never get out of the spin loop until the owner stops running.

That sounds very wrong to me :-)

Shouldn't you at least remove the test for !owner when testing
need_resched() in __mutex_lock_common() ?

Cheers,
Ben.

--


[ message continues ]
[view original message]
From: Peter Zijlstra
Subject: Re: Possible bug with mutex adaptative spinning
Date: Friday, April 16, 2010 - 2:27 pm

The inner-most spin loop breaks out on need_resched():

                if (task_thread_info(rq->curr) != owner || need_resched())
                        return 0;


--


[ message continues ]
[view original message]
From: Benjamin Herrenschmidt
Subject: Re: Possible bug with mutex adaptative spinning
Date: Friday, April 16, 2010 - 3:01 pm

You are right, this was only a problem in conjunction with the other bug
returning 1 all the time, causing us to ignore need_resched(). With
that fixed, it looks fine now.

Thanks !

Cheers,
Ben.


--


[ message continues ]
[view original message]
From: Peter Zijlstra
Subject: Re: Possible bug with mutex adaptative spinning
Date: Friday, April 16, 2010 - 2:26 pm

Right, patch looks good, comments are a tad misleading.


I've queued the below patch

---
Subject: mutex: Don't spin when the owner CPU is offline or other weird cases
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Fri Apr 16 23:20:00 CEST 2010

Due to recent load-balancer changes that delay the task migration to
the next wakeup, the adaptive mutex spinning ends up in a live lock
when the owner's CPU gets offlined because the cpu_online() check
lives before the owner running check.

This patch changes mutex_spin_on_owner() to return 0 (don't spin) in
any case where we aren't sure about the owner struct validity or CPU
number, and if the said CPU is offline. There is no point going back &
re-evaluate spinning in corner cases like that, let's just go to
sleep.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1271212509.13059.135.camel@pasglop>
---
 kernel/sched.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -3647,7 +3647,7 @@ int mutex_spin_on_owner(struct mutex *lo
 	 * the mutex owner just released it and exited.
 	 */
 	if (probe_kernel_address(&owner->cpu, cpu))
-		goto out;
+		return 0;
 #else
 	cpu = owner->cpu;
 #endif
@@ -3657,14 +3657,14 @@ int mutex_spin_on_owner(struct mutex *lo
 	 * the cpu field may no longer be valid.
 	 */
 	if (cpu >= nr_cpumask_bits)
-		goto out;
+		return 0;
 
 	/*
 	 * We need to validate that we can do a
 	 * get_cpu() and that we have the percpu area.
 	 */
 	if (!cpu_online(cpu))
-		goto out;
+		return 0;
 
 	rq = cpu_rq(cpu);
 
@@ -3683,7 +3683,7 @@ int mutex_spin_on_owner(struct mutex *lo
 
 		cpu_relax();
 	}
-out:
+
 	return 1;
 }
 #endif


--


[ message continues ]
[view original message]
From: Benjamin Herrenschmidt
Subject: Re: Possible bug with mutex adaptative spinning
Date: Friday, April 16, 2010 - 3:00 pm

Thanks. Should it make -stable as well ?

Cheers,


--


[ message continues ]
[view original message]
From: tip-bot for Benjamin Herrenschmidt
Subject: [tip:core/urgent] mutex: Don't spin when the owner CPU is offline or other weird cases
Date: Friday, April 23, 2010 - 3:49 am

Commit-ID:  4b402210486c6414fe5fbfd85934a0a22da56b04
Gitweb:     http://git.kernel.org/tip/4b402210486c6414fe5fbfd85934a0a22da56b04
Author:     Benjamin Herrenschmidt <benh@kernel.crashing.org>
AuthorDate: Fri, 16 Apr 2010 23:20:00 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 23 Apr 2010 11:00:28 +0200

mutex: Don't spin when the owner CPU is offline or other weird cases

Due to recent load-balancer changes that delay the task migration to
the next wakeup, the adaptive mutex spinning ends up in a live lock
when the owner's CPU gets offlined because the cpu_online() check
lives before the owner running check.

This patch changes mutex_spin_on_owner() to return 0 (don't spin) in
any case where we aren't sure about the owner struct validity or CPU
number, and if the said CPU is offline. There is no point going back &
re-evaluate spinning in corner cases like that, let's just go to
sleep.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1271212509.13059.135.camel@pasglop>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 6af210a..de0bd26 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3780,7 +3780,7 @@ int mutex_spin_on_owner(struct mutex *lock, struct thread_info *owner)
 	 * the mutex owner just released it and exited.
 	 */
 	if (probe_kernel_address(&owner->cpu, cpu))
-		goto out;
+		return 0;
 #else
 	cpu = owner->cpu;
 #endif
@@ -3790,14 +3790,14 @@ int mutex_spin_on_owner(struct mutex *lock, struct thread_info *owner)
 	 * the cpu field may no longer be valid.
 	 */
 	if (cpu >= nr_cpumask_bits)
-		goto out;
+		return 0;
 
 	/*
 	 * We need to validate that we can do a
 	 * get_cpu() and that we have the percpu area.
 	 */
 	if (!cpu_online(cpu))
-		goto out;
+		return 0;
 
 	rq = cpu_rq(cpu);
 
@@ -3816,7 ...
[ message continues ]
Previous thread: [PATCH][resend] namei.c : update mnt when it needed by Huang Shijie on Tuesday, April 13, 2010 - 7:16 pm. (2 messages)

Next thread: Security: Replace dac_mmap_min_addr to mmap_min_addr in cap_file_mmap() by wzt.wzt on Tuesday, April 13, 2010 - 7:48 pm. (4 messages)