Bisected KVM hang on x86-32 between v3.12 and v3.13

PATCH In review Michele Ballabio 7 participants 23 messages Apr 6, 2014 – May 13, 2014

Subsystems (2)

x86 Scheduler

lore

Share & tools

lore

Michele Ballabio

author

2014-04-06 15:29 UTC #0 lore raw

Toralf Förster reported this in
http://article.gmane.org/gmane.linux.kernel/1662567
http://article.gmane.org/gmane.linux.kernel/1658422
http://article.gmane.org/gmane.linux.kernel/1657962

"The issue happens here at a 32 bit stable Gentoo Linux if
I try to start a KVM image. Kernels 3.12.X works fine, kernel >= v3.13 will hang shortly after I started the image with the virtual-manager. The last syslog messages are something like:

   Feb 28 16:22:00 n22 kernel: INFO: rcu_sched detected stalls
       on CPUs/tasks: {} (detected by 2, t=60002 jiffies,
       g=14689, c=14688, q=21051)
   Feb 28 16:22:00 n22 kernel: INFO: Stall ended before state
       dump start"

He correctly pointed out that the bisection blamed the merge commit 37bf06375c90a42fe07b9bebdb07bc316ae5a0ce "Merge tag 'v3.12-rc4' into sched/core".

This bug is obviously caused by at least two patches, one on each side of the merge, that only when combined together (at that merge point) cause the bug in kvm. By rebasing the "sched/core" branch on "master" before the merge and going on with the bisection, I found commit 3e8e42c69bb7d9fc12ebc23ff308e8523a2a59a0
"sched: Revert need_resched() to look at TIF_NEED_RESCHED" as one of the causes. The other patch that contributes to the bug is commit ded797547548a5b8e7b92383a41e4c0e6b0ecb7f "irq: Force hardirq exit's softirq processing on its own stack".

Reverting either one of them solves the problem reported with kvm, but revert is probably not the correct answer.

I wonder if the solution is as simple as this:

--->8---

arch/x86/Kconfig +1 -0

index 0af5250..f3b985d 100644
@@ -126,6 +126,7 @@ config X86
 	select RTC_LIB
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
+	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_32
 	select HAVE_CC_STACKPROTECTOR
 
 config INSTRUCTION_DECODER

---8<---

Toralf Förster

Applied

2014-04-06 15:53 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Quoted 43 lines · Michele Ballabio

Toralf Förster reported this in
http://article.gmane.org/gmane.linux.kernel/1662567
http://article.gmane.org/gmane.linux.kernel/1658422
http://article.gmane.org/gmane.linux.kernel/1657962
"The issue happens here at a 32 bit stable Gentoo Linux if
I try to start a KVM image. Kernels 3.12.X works fine, kernel >= v3.13 will hang shortly after I started the image with the virtual-manager. The last syslog messages are something like:
   Feb 28 16:22:00 n22 kernel: INFO: rcu_sched detected stalls
       on CPUs/tasks: {} (detected by 2, t=60002 jiffies,
       g=14689, c=14688, q=21051)
   Feb 28 16:22:00 n22 kernel: INFO: Stall ended before state
       dump start"
He correctly pointed out that the bisection blamed the merge commit 37bf06375c90a42fe07b9bebdb07bc316ae5a0ce "Merge tag 'v3.12-rc4' into sched/core".
This bug is obviously caused by at least two patches, one on each side of the merge, that only when combined together (at that merge point) cause the bug in kvm. By rebasing the "sched/core" branch on "master" before the merge and going on with the bisection, I found commit 3e8e42c69bb7d9fc12ebc23ff308e8523a2a59a0
"sched: Revert need_resched() to look at TIF_NEED_RESCHED" as one of the causes. The other patch that contributes to the bug is commit ded797547548a5b8e7b92383a41e4c0e6b0ecb7f "irq: Force hardirq exit's softirq processing on its own stack".
Reverting either one of them solves the problem reported with kvm, but revert is probably not the correct answer.
I wonder if the solution is as simple as this:
--->8---
arch/x86/Kconfig +0 -0
index 0af5250..f3b985d 100644

arch/x86/Kconfig config X86

Quoted 8 lines

arch/x86/Kconfig +1 -1

@@ -126,6 +126,7 @@ config X86
 	select RTC_LIB
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
+	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_32
 	select HAVE_CC_STACKPROTECTOR
 
 config INSTRUCTION_DECODER
---8<---

applied both to 3.13.9 and 3.14.0 - issue does not happened any longer

Thanks !

P.S..
'By rebasing the "sched/core" branch on "master" before the merge and going on with the bisection'

Probably off-topic but I'm really interested what did you do in detail ? I'm asking b/c using git for my own and to bisect a remote tree, but I'm not too familiar in bisecting bugs of this kind. Furthermore probably worth an own section in one of the TODO's ?

- --
MfG/Sincerely
Toralf Förster
pgp finger print:1A37 6F99 4A9D 026F 13E2 4DCF C4EA CDDE 0076 E94E -----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlNBeE4ACgkQxOrN3gB26U4zpgD/bEaIS17/FIxmsyHZvL15RoX6 Z0dLwOoPcIRJyi2pn44A/0qh9YmB9Bv2yIf7qsUaEZA+lpJ+ikWMZSVEW2JtZMV0 =QOaP
-----END PGP SIGNATURE-----

Michele Ballabio

author

2014-04-06 17:39 UTC #2 lore raw

Quoted 17 lines · Toralf Förster

Quoted 17 lines
Quoted 17 lines
Reverting either one of them solves the problem reported with kvm, but revert is probably not the correct answer.
I wonder if the solution is as simple as this:
--->8---
arch/x86/Kconfig +1 -0
index 0af5250..f3b985d 100644
@@ -126,6 +126,7 @@ config X86
 	select RTC_LIB
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
+	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_32
 	select HAVE_CC_STACKPROTECTOR
 

Quoted 6 lines

Quoted 3 lines
arch/x86/Kconfig config X86
Quoted 3 lines
arch/x86/Kconfig +0 -1
 config INSTRUCTION_DECODER
---8<---
applied both to 3.13.9 and 3.14.0 - issue does not happened any longer
Thanks !

You're welcome, but I'm not sure it's bug-free, I've only glanced at the code. Let's hear what other people think.

Quoted 6 lines

P.S.. 'By rebasing the "sched/core" branch on "master" before the merge and going on with the bisection'
Probably off-topic but I'm really interested what did you do in detail ? I'm asking b/c using git for my own and to bisect a remote tree, but I'm not too familiar in bisecting bugs of this kind.

Let's say bisection blames the merge commit 'M' as the first bad commit:

R---S---T---P---X---Y---Z---M

             \             /
              A---B---C---'

First, let's search for the merge base P:
git merge-base Z C

Now: we know there are at least 2 patches (one on both sides of the merge) that are responsible for the regression.
One is either X, Y or Z; the other is either A, B or C.

To find the first one (between X, Y and Z), we can rebase Z on top of C, either with

git rebase C Z

or with
git format-patch Z ^C
git am 00*
(It's almost the same thing, sometimes you might want to do this to edit out some patches without using git bisect --interactive).

The result should be:

P---X---Y---Z---M
\ /
A---B---C---'

            \
             X'---Y'---Z'

At this point
git diff M Y'
should show nothing.

Now we can further our bisection:
git bisect start Z' C
... should give us the first patch to blame: let's say it's Y'.

Now we search for the other one: the sequence I used is

# We know the other guilty commit is A, B or C
git bisect start C P
# We will be brought on commit B.
# We know the regression triggers only with Y, so we merge with it
git merge --no-commit Y
# The result will be something like this:
P---X---Y---Z---M
\ \ /
A---B---N /

        \     /
         C---'
          \
           X'---Y'---Z'
# Where N is a merge, but it's not a commit.
# 1st compile, install, reboot, test
# We are about to search another commit to test, so let's remove the
# temporary changes of the previous test
  git reset --hard
# What is the result of the 1st test?
  git bisect good|bad
# Here is another iteration
  git merge --no-commit Y
# 2nd compile, install, reboot, test

... and so on. Of course, there are some variations: if Y does not depend on X, instead of a merge we could do:
git cherry-pick --no-commit Y

In this case I used a merge because it was easier, and actually I didn't merge _exactly_ with the blamed commit, but with the last commit before the merge M, i.e. Z (I assumed it would be better this way).

Quoted 1 line

Furthermore probably worth an own section in one of the TODO's ?

I think this (a regression blamed on a merge) is quite unusual, and as such it could be useful on the man page of git-bisect, but it should be written better and shorter than what I did here.

Peter Zijlstra

maintainer

2014-04-07 15:03 UTC #3 lore raw

In reply to Michele Ballabio

Quoted 42 lines · Michele Ballabio

Toralf Förster reported this in
http://article.gmane.org/gmane.linux.kernel/1662567
http://article.gmane.org/gmane.linux.kernel/1658422
http://article.gmane.org/gmane.linux.kernel/1657962
"The issue happens here at a 32 bit stable Gentoo Linux if
I try to start a KVM image. Kernels 3.12.X works fine, kernel >= v3.13 will hang shortly after I started the image with the virtual-manager. The last syslog messages are something like:
   Feb 28 16:22:00 n22 kernel: INFO: rcu_sched detected stalls
       on CPUs/tasks: {} (detected by 2, t=60002 jiffies,
       g=14689, c=14688, q=21051)
   Feb 28 16:22:00 n22 kernel: INFO: Stall ended before state
       dump start"
He correctly pointed out that the bisection blamed the merge commit 37bf06375c90a42fe07b9bebdb07bc316ae5a0ce "Merge tag 'v3.12-rc4' into sched/core".
This bug is obviously caused by at least two patches, one on each side of the merge, that only when combined together (at that merge point) cause the bug in kvm. By rebasing the "sched/core" branch on "master" before the merge and going on with the bisection, I found commit 3e8e42c69bb7d9fc12ebc23ff308e8523a2a59a0
"sched: Revert need_resched() to look at TIF_NEED_RESCHED" as one of the causes. The other patch that contributes to the bug is commit ded797547548a5b8e7b92383a41e4c0e6b0ecb7f "irq: Force hardirq exit's softirq processing on its own stack".
Reverting either one of them solves the problem reported with kvm, but revert is probably not the correct answer.
I wonder if the solution is as simple as this:
--->8---
arch/x86/Kconfig +0 -0
index 0af5250..f3b985d 100644

arch/x86/Kconfig config X86 · line 130

Quoted 5 lines

arch/x86/Kconfig +1 -0

@@ -126,6 +126,7 @@ config X86
 	select RTC_LIB
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
+	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_32
 	select HAVE_CC_STACKPROTECTOR

Ohh ahh.. shiney!

So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.

Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.

Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.

Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.

Peter Zijlstra

maintainer

2014-04-07 15:07 UTC #4 lore raw

Quoted 8 lines · Peter Zijlstra

So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.
Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.

Quoted 7 lines

Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.
Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.

Ohh, goodie, they're already in Linus' tree. Could you see if current git still suffers this problem?

Toralf Förster

2014-04-07 18:16 UTC #5 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Peter Zijlstra
So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.
Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.
Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.

Quoted 7 lines

Quoted 3 lines
Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.
Ohh, goodie, they're already in Linus' tree. Could you see if current git still suffers this problem?

v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)

iF4EAREIAAYFAlNC63gACgkQxOrN3gB26U6MoQD/QlC0K92TivMFR6H35G9m1QzK dkdGzHDVn5HtgIswDToA/RhmhblKnqM7bWYzOxwypF7oZGdKRnBQocWbbuMi5lf0 =tw9y
-----END PGP SIGNATURE-----

Peter Zijlstra

maintainer

2014-04-07 18:57 UTC #6 lore raw

Quoted 2 lines · Toralf Förster

v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)

I'm fairly sure its not; its a rather invasive series; see:

2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32

Let me see if there's anything 'simpler' we can stuff in .13+

Frederic Weisbecker

2014-04-07 19:00 UTC #19 lore raw

In reply to Toralf Förster

Quoted 21 lines · Toralf Förster

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Quoted 17 lines · Peter Zijlstra
Quoted 15 lines · Peter Zijlstra
So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.
Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.
Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.
Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.

Quoted 5 lines

Quoted 3 lines
Ohh, goodie, they're already in Linus' tree. Could you see if current git still suffers this problem?
v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)

If we are reffering to 198d208df4371734ac4728f69cb585c284d20a15 (x86: Keep thread_info on thread stack in x86_32) it doesn't carry a stable tag.

So to be clear, you are saying that v3.14 is fine but other release are buggy? Which ones are these?

If a backport is needed, 198d208df4371734ac4728f69cb585c284d20a15 looks too large...

Michele Ballabio

author

2014-04-07 19:49 UTC #22 lore raw

In reply to Peter Zijlstra

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines · Peter Zijlstra
So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.

Quoted 7 lines

Quoted 7 lines
Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.
Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.

Understood.

Quoted 6 lines

Quoted 3 lines
Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.
Ohh, goodie, they're already in Linus' tree. Could you see if current git still suffers this problem?

It is fixed on current -master (3.14.0-10773-gb8780c3).

Thanks.

Toralf Förster

2014-04-07 19:57 UTC #20 lore raw

In reply to Frederic Weisbecker

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Quoted 29 lines · Frederic Weisbecker

Quoted 22 lines · Toralf Förster
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Quoted 19 lines · Peter Zijlstra
Quoted 16 lines · Peter Zijlstra
So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.
Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.
Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.
Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.
Quoted 5 lines
Quoted 3 lines
Ohh, goodie, they're already in Linus' tree. Could you see if current git still suffers this problem?
v3.14-10353-g2b3a8fd works fine AFAICS (BTW the fix is stable material, right ?)

Quoted 7 lines

If we are reffering to 198d208df4371734ac4728f69cb585c284d20a15 (x86: Keep thread_info on thread stack in x86_32) it doesn't carry a stable tag.
So to be clear, you are saying that v3.14 is fine but other release are buggy? Which ones are these?

No, 3.13.x and 3.14 have the issue, latest git is fine and 3.12.x

Quoted 3 lines

If a backport is needed, 198d208df4371734ac4728f69cb585c284d20a15 looks too large...

iF4EAREIAAYFAlNDAwwACgkQxOrN3gB26U5Z7AD+OMcd+262s1fJV9Fg8NjL5TaI Y4hZHzdv76fU/9P1lrkA/RUbQFYn6r9fZz61UrEIZ5lg5dUX04DDcQkfPUMZRow3 =Ifk8
-----END PGP SIGNATURE-----

Frederic Weisbecker

2014-04-07 22:43 UTC #21 lore raw

Quoted 33 lines · Toralf Förster

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Quoted 29 lines · Frederic Weisbecker
Quoted 27 lines · Toralf Förster
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Quoted 15 lines · Peter Zijlstra
Quoted 13 lines · Peter Zijlstra
So what I suspect at this point is that because i386 and x86_64 have a difference in current_thread_info() (i386 is stack based), we end up setting the TIF_NEED_RESCHED bit on the wrong stack.
Now I have some vague memories of propagating the TIF flags on stack switch, but I cannot remember what arch we did that for. Let me stare at this a little more.
Also, IFF this is the case, then the fingered patch above (and your suggested 'fix') aren't the real curlpit/cure but simply make it more/less likely to happen.
Quoted 7 lines
Quoted 3 lines
Now, Steve had a patch somewhere that would make i386 use per-cpu variables for current_thread_info() just like x86_64 already does I think. Let me go find them too.
Ohh, goodie, they're already in Linus' tree. Could you see if current git still suffers this problem?
v3.14-10353-g2b3a8fd works fine AFAICS (BTW the fix is stable material, right ?)

Quoted 9 lines

Quoted 7 lines
If we are reffering to 198d208df4371734ac4728f69cb585c284d20a15 (x86: Keep thread_info on thread stack in x86_32) it doesn't carry a stable tag.
So to be clear, you are saying that v3.14 is fine but other release are buggy? Which ones are these?
No, 3.13.x and 3.14 have the issue, latest git is fine and 3.12.x

Ah ok. Hmm, Peter seemed to have an idea on a fix to backport so I'll let him handle that :)

Thanks.

Peter Zijlstra

maintainer

2014-04-08 14:49 UTC #7 lore raw

In reply to Peter Zijlstra

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Toralf Förster
v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)

Quoted 8 lines

I'm fairly sure its not; its a rather invasive series; see:
2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32
Let me see if there's anything 'simpler' we can stuff in .13+

OK.. so could someone test the below patch to see if that makes things work for .13 and .14?

---
 arch/x86/include/asm/preempt.h | 11 +++++++++++
 include/linux/preempt.h        |  4 ++++
 include/linux/thread_info.h    |  2 --
 3 files changed, 15 insertions(+), 2 deletions(-)

arch/x86/include/asm/preempt.h +11 -0

index c8b051933b1b..7ada46d8d4d8 100644
@@ -5,6 +5,17 @@
 #include <asm/percpu.h>
 #include <linux/thread_info.h>
 
+#ifdef CONFIG_X86_32
+/*
+ * i386's current_thread_info() depends on ESP and for interrupt/exception
+ * stacks this doesn't yield the actual task thread_info.
+ *
+ * We hard rely on the fact that all the TIF_NEED_RESCHED bits are
+ * the same, therefore use the slightly more expensive version below.
+ */
+#define tif_need_resched() test_tsk_thread_flag(current, TIF_NEED_RESCHED)
+#endif
+
 DECLARE_PER_CPU(int, __preempt_count);
 
 /*

include/linux/preempt.h +4 -0

index de83b4eb1642..891c8f8b52fe 100644
@@ -17,6 +17,10 @@
 
 #include <asm/preempt.h>
 
+#ifndef tif_need_resched
+#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
+#endif
+
 #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
 extern void preempt_count_add(int val);
 extern void preempt_count_sub(int val);

include/linux/thread_info.h +0 -2

index fddbe2023a5d..a629e4b23217 100644
@@ -118,8 +118,6 @@ static inline __deprecated void set_need_resched(void)
 	 */
 }
 
-#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
-
 #if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
 /*
  * An arch can define its own version of set_restore_sigmask() to get the

Michele Ballabio

author

2014-04-08 19:14 UTC #8 lore raw

Quoted 67 lines · Peter Zijlstra

Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Toralf Förster
v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)
Quoted 8 lines
I'm fairly sure its not; its a rather invasive series; see:
2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32
Let me see if there's anything 'simpler' we can stuff in .13+
OK.. so could someone test the below patch to see if that makes things work for .13 and .14?
---
 arch/x86/include/asm/preempt.h | 11 +++++++++++
 include/linux/preempt.h        |  4 ++++
 include/linux/thread_info.h    |  2 --
 3 files changed, 15 insertions(+), 2 deletions(-)
arch/x86/include/asm/preempt.h +11 -0
index c8b051933b1b..7ada46d8d4d8 100644
@@ -5,6 +5,17 @@
 #include <asm/percpu.h>
 #include <linux/thread_info.h>
 
+#ifdef CONFIG_X86_32
+/*
+ * i386's current_thread_info() depends on ESP and for interrupt/exception
+ * stacks this doesn't yield the actual task thread_info.
+ *
+ * We hard rely on the fact that all the TIF_NEED_RESCHED bits are
+ * the same, therefore use the slightly more expensive version below.
+ */
+#define tif_need_resched() test_tsk_thread_flag(current, TIF_NEED_RESCHED)
+#endif
+
 DECLARE_PER_CPU(int, __preempt_count);
 
 /*
include/linux/preempt.h +4 -0
index de83b4eb1642..891c8f8b52fe 100644
@@ -17,6 +17,10 @@
 
 #include <asm/preempt.h>
 
+#ifndef tif_need_resched
+#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
+#endif
+
 #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
 extern void preempt_count_add(int val);
 extern void preempt_count_sub(int val);
include/linux/thread_info.h +0 -0
index fddbe2023a5d..a629e4b23217 100644

include/linux/thread_info.h static inline __deprecated void set_need_resched(void) · line 123

Quoted 8 lines

include/linux/thread_info.h +0 -2

@@ -118,8 +118,6 @@ static inline __deprecated void set_need_resched(void)
 	 */
 }
 
-#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
-
 #if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
 /*
  * An arch can define its own version of set_restore_sigmask() to get the

It works fine for me on top of v3.14.

Michele Ballabio

author

2014-04-08 19:51 UTC #9 lore raw

Quoted 70 lines · Michele Ballabio

Quoted 69 lines · Peter Zijlstra
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Toralf Förster
v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)
Quoted 8 lines
I'm fairly sure its not; its a rather invasive series; see:
2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32
Let me see if there's anything 'simpler' we can stuff in .13+
OK.. so could someone test the below patch to see if that makes things work for .13 and .14?
---
 arch/x86/include/asm/preempt.h | 11 +++++++++++
 include/linux/preempt.h        |  4 ++++
 include/linux/thread_info.h    |  2 --
 3 files changed, 15 insertions(+), 2 deletions(-)
arch/x86/include/asm/preempt.h +11 -0
index c8b051933b1b..7ada46d8d4d8 100644
@@ -5,6 +5,17 @@
 #include <asm/percpu.h>
 #include <linux/thread_info.h>
 
+#ifdef CONFIG_X86_32
+/*
+ * i386's current_thread_info() depends on ESP and for interrupt/exception
+ * stacks this doesn't yield the actual task thread_info.
+ *
+ * We hard rely on the fact that all the TIF_NEED_RESCHED bits are
+ * the same, therefore use the slightly more expensive version below.
+ */
+#define tif_need_resched() test_tsk_thread_flag(current, TIF_NEED_RESCHED)
+#endif
+
 DECLARE_PER_CPU(int, __preempt_count);
 
 /*
include/linux/preempt.h +4 -0
index de83b4eb1642..891c8f8b52fe 100644
@@ -17,6 +17,10 @@
 
 #include <asm/preempt.h>
 
+#ifndef tif_need_resched
+#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
+#endif
+
 #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
 extern void preempt_count_add(int val);
 extern void preempt_count_sub(int val);
include/linux/thread_info.h +0 -0
index fddbe2023a5d..a629e4b23217 100644
@@ -118,8 +118,6 @@ static inline __deprecated void set_need_resched(void)
 	 */
 }
 

Quoted 8 lines

include/linux/thread_info.h static inline __deprecated void set_need_resched(void) · line 123
Quoted 6 lines
include/linux/thread_info.h +0 -2
-#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
-
 #if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
 /*
  * An arch can define its own version of set_restore_sigmask() to get the
It works fine for me on top of v3.14.

And on top of v3.13, too.

Thanks.

Toralf Förster

2014-04-08 20:31 UTC #10 lore raw

In reply to Peter Zijlstra

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Quoted 14 lines · Peter Zijlstra

Quoted 13 lines · Peter Zijlstra
Quoted 2 lines · Toralf Förster
v3.14-10353-g2b3a8fd works fine AFAICS (BTW the fix is stable material, right ?)
I'm fairly sure its not; its a rather invasive series; see:
2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32

Quoted 5 lines

Quoted 1 line
Let me see if there's anything 'simpler' we can stuff in .13+
OK.. so could someone test the below patch to see if that makes things work for .13 and .14?

patch applied on 3.1.3.9 and 3.14.0 it fixed the issue AFAICT

Quoted 37 lines

--- arch/x86/include/asm/preempt.h | 11 +++++++++++
include/linux/preempt.h        |  4 ++++ 
include/linux/thread_info.h    |  2 -- 3 files changed, 15
insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/arch/x86/include/asm/preempt.h index c8b051933b1b..7ada46d8d4d8 100644 --- a/arch/x86/include/asm/preempt.h +++ b/arch/x86/include/asm/preempt.h @@ -5,6 +5,17 @@ #include <asm/percpu.h> #include <linux/thread_info.h>
+#ifdef CONFIG_X86_32 +/* + * i386's current_thread_info() depends on ESP and for interrupt/exception + * stacks this doesn't yield the actual task thread_info. + * + * We hard rely on the fact that all the TIF_NEED_RESCHED bits are + * the same, therefore use the slightly more expensive version below. + */ +#define tif_need_resched() test_tsk_thread_flag(current, TIF_NEED_RESCHED) +#endif + DECLARE_PER_CPU(int, __preempt_count);
/* diff --git a/include/linux/preempt.h b/include/linux/preempt.h index de83b4eb1642..891c8f8b52fe 100644 --- a/include/linux/preempt.h +++ b/include/linux/preempt.h @@ -17,6 +17,10 @@
#include <asm/preempt.h>
+#ifndef tif_need_resched +#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED) +#endif + #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER) extern void preempt_count_add(int val); extern void preempt_count_sub(int val); diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h index fddbe2023a5d..a629e4b23217 100644 --- a/include/linux/thread_info.h +++ b/include/linux/thread_info.h @@ -118,8 +118,6 @@ static inline __deprecated void set_need_resched(void) */ }

Quoted 5 lines

-#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED) - #if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK /* * An arch can define its own version of set_restore_sigmask() to get the

iF0EAREIAAYFAlNEW+EACgkQxOrN3gB26U5RkAD4ysUp9CrCajM7zXLsdWRLTZFi oX0lYgrKo/gCrcS9UwD/ZdaRPlP6vTKApfxQ7F48KTTA+tNcyTD4xSyCdwI8l0Q= =WeKZ
-----END PGP SIGNATURE-----

Stefan Bader

2014-04-09 09:14 UTC #11 lore raw

In reply to Peter Zijlstra

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Peter Zijlstra
Quoted 2 lines · Toralf Förster
v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)
I'm fairly sure its not; its a rather invasive series; see:
2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32

Quoted 4 lines

Quoted 1 line
Let me see if there's anything 'simpler' we can stuff in .13+
OK.. so could someone test the below patch to see if that makes things work for .13 and .14?

Hi Peter,

sorry I only had time to try 3.13 but that looks good. I was able to get kvm up and running for a longer than usual (without the patch) amount of time without seeing any softlockups.

-Stefan

Quoted 58 lines

---
 arch/x86/include/asm/preempt.h | 11 +++++++++++
 include/linux/preempt.h        |  4 ++++
 include/linux/thread_info.h    |  2 --
 3 files changed, 15 insertions(+), 2 deletions(-)

arch/x86/include/asm/preempt.h +11 -0

index c8b051933b1b..7ada46d8d4d8 100644
@@ -5,6 +5,17 @@
 #include <asm/percpu.h>
 #include <linux/thread_info.h>
 
+#ifdef CONFIG_X86_32
+/*
+ * i386's current_thread_info() depends on ESP and for interrupt/exception
+ * stacks this doesn't yield the actual task thread_info.
+ *
+ * We hard rely on the fact that all the TIF_NEED_RESCHED bits are
+ * the same, therefore use the slightly more expensive version below.
+ */
+#define tif_need_resched() test_tsk_thread_flag(current, TIF_NEED_RESCHED)
+#endif
+
 DECLARE_PER_CPU(int, __preempt_count);
 
 /*

include/linux/preempt.h +4 -0

index de83b4eb1642..891c8f8b52fe 100644
@@ -17,6 +17,10 @@
 
 #include <asm/preempt.h>
 
+#ifndef tif_need_resched
+#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
+#endif
+
 #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
 extern void preempt_count_add(int val);
 extern void preempt_count_sub(int val);

include/linux/thread_info.h +0 -2

index fddbe2023a5d..a629e4b23217 100644
@@ -118,8 +118,6 @@ static inline __deprecated void set_need_resched(void)
 	 */
 }
 
-#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
-
 #if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
 /*
  * An arch can define its own version of set_restore_sigmask() to get the

Peter Zijlstra

maintainer

2014-04-09 09:45 UTC #12 lore raw

Quoted 16 lines · Stefan Bader

Quoted 15 lines · Peter Zijlstra
Quoted 13 lines · Peter Zijlstra
Quoted 2 lines · Toralf Förster
v3.14-10353-g2b3a8fd works fine AFAICS
(BTW the fix is stable material, right ?)
I'm fairly sure its not; its a rather invasive series; see:
2432e1364bbe x86: Nuke the supervisor_stack field in i386 thread_info b807902a88c4 x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386 0788aa6a23cb x86: Prepare removal of previous_esp from i386 thread_info structure 198d208df437 x86: Keep thread_info on thread stack in x86_32
Let me see if there's anything 'simpler' we can stuff in .13+

Quoted 8 lines

Quoted 2 lines
OK.. so could someone test the below patch to see if that makes things work for .13 and .14?
Hi Peter,
sorry I only had time to try 3.13 but that looks good. I was able to get kvm up and running for a longer than usual (without the patch) amount of time without seeing any softlockups.

Great, I suppose I should go write a changelog and talk to Greg about how to get this into .13 and .14 stable.

Peter Zijlstra

maintainer

2014-04-09 14:25 UTC #13 lore raw

PATCH 1/1 x86,preempt: Fix preemption for i386

Greg, Linus,

I'm not entirely clear on how acceptable it is to propose a different patch for -stable than what we have upstream.

The below Changelog explain, but in short, we should either backport 4 rather invasive patches to .13-stable and .14-stable or do the small patch provided.

The 4 patches by Steven are the right thing to do, but I feel they might be too invasive to propose for -stable, therefore the alternative approach.

---
Subject: x86,preempt: Fix preemption for i386
From: Peter Zijlstra <[email protected]>
Date: Tue, 8 Apr 2014 14:21:28 +0200

Many people reported preemption/reschedule problems with i386 kernels for .13 and .14. After Michele bisected this to a combination of

3e8e42c69bb ("sched: Revert need_resched() to look at TIF_NEED_RESCHED") ded79754754 ("irq: Force hardirq exit's softirq processing on its own stack")

it finally dawned on me that i386's current_thread_info() was to blame.

When we are on interrupt/exception stacks, we fail to observe the right TIF_NEED_RESCHED bit and therefore the PREEMPT_NEED_RESCHED folding malfunctions.

Current upstream fixes this by making i386 behave the same as x86_64 already did:

2432e1364bbe ("x86: Nuke the supervisor_stack field in i386 thread_info") b807902a88c4 ("x86: Nuke GET_THREAD_INFO_WITH_ESP() macro for i386") 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure") 198d208df437 ("x86: Keep thread_info on thread stack in x86_32")

However, that is far too much to stuff into -stable. Therefore I propose we merge the below patch which uses task_thread_info(current) for tif_need_resched() instead of the ESP based current_thread_info().

This makes sure we always observe the one true TIF_NEED_RESCHED bit and things will work as expected again.

Cc [email protected]

Cc Steven Rostedt <[email protected]>

Cc David Cohen <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Borislav Petkov <[email protected]>

Tested-by Stefan Bader <[email protected]>

Tested-by Toralf Förster <[email protected]>

Tested-by Michele Ballabio <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/x86/include/asm/preempt.h |   11 +++++++++++
 include/linux/preempt.h        |    4 ++++
 include/linux/thread_info.h    |    2 --
 3 files changed, 15 insertions(+), 2 deletions(-)

arch/x86/include/asm/preempt.h +11 -0

@@ -5,6 +5,17 @@
 #include <asm/percpu.h>
 #include <linux/thread_info.h>
 
+#ifdef CONFIG_X86_32
+/*
+ * i386's current_thread_info() depends on ESP and for interrupt/exception
+ * stacks this doesn't yield the actual task thread_info.
+ *
+ * We hard rely on the fact that all the TIF_NEED_RESCHED bits are
+ * the same, therefore use the slightly more expensive version below.
+ */
+#define tif_need_resched() test_tsk_thread_flag(current, TIF_NEED_RESCHED)
+#endif
+
 DECLARE_PER_CPU(int, __preempt_count);
 
 /*

include/linux/preempt.h +4 -0

@@ -17,6 +17,10 @@
 
 #include <asm/preempt.h>
 
+#ifndef tif_need_resched
+#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
+#endif
+
 #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
 extern void preempt_count_add(int val);
 extern void preempt_count_sub(int val);

include/linux/thread_info.h +0 -2

@@ -118,8 +118,6 @@ static inline __deprecated void set_need
 	 */
 }
 
-#define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
-
 #if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
 /*
  * An arch can define its own version of set_restore_sigmask() to get the

Linus Torvalds

2014-04-09 14:36 UTC #14 lore raw

Re: [PATCH -stable] x86,preempt: Fix preemption for i386

Quoted 3 lines · Peter Zijlstra

I'm not entirely clear on how acceptable it is to propose a different patch for -stable than what we have upstream.

It's not all that common, but it certainly happens.

It's fine, as long as it mentions the commits that fix it upstream. And as long as it's well tested, of course.

Linus

Greg KH

2014-04-09 19:17 UTC #15 lore raw

Re: [PATCH -stable] x86,preempt: Fix preemption for i386

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · Peter Zijlstra
I'm not entirely clear on how acceptable it is to propose a different patch for -stable than what we have upstream.
It's not all that common, but it certainly happens.
It's fine, as long as it mentions the commits that fix it upstream. And as long as it's well tested, of course.

I agree, I can take this as long as you say it's correct and tested...

thanks,

greg k-h

Peter Zijlstra

maintainer

2014-04-09 19:39 UTC #16 lore raw

Re: [PATCH -stable] x86,preempt: Fix preemption for i386

Quoted 6 lines · Greg KH

Quoted 5 lines · Linus Torvalds
Quoted 3 lines · Peter Zijlstra
I'm not entirely clear on how acceptable it is to propose a different patch for -stable than what we have upstream.

Quoted 6 lines

Quoted 4 lines
It's not all that common, but it certainly happens.
It's fine, as long as it mentions the commits that fix it upstream. And as long as it's well tested, of course.
I agree, I can take this as long as you say it's correct and tested...

As far as I understand the issue the patch is indeed correct and I have 3 independent people who confirm their previously reported issues are now cured (as testified by the Tested-by tags).

There has also been confirmation that upstream does no longer suffer the problem.

Greg KH

2014-04-09 19:55 UTC #17 lore raw

Re: [PATCH -stable] x86,preempt: Fix preemption for i386

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Greg KH
Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Peter Zijlstra
I'm not entirely clear on how acceptable it is to propose a different patch for -stable than what we have upstream.
It's not all that common, but it certainly happens.
It's fine, as long as it mentions the commits that fix it upstream. And as long as it's well tested, of course.

Quoted 8 lines

Quoted 1 line
I agree, I can take this as long as you say it's correct and tested...
As far as I understand the issue the patch is indeed correct and I have 3 independent people who confirm their previously reported issues are now cured (as testified by the Tested-by tags).
There has also been confirmation that upstream does no longer suffer the problem.

Thanks for that, I'll queue it up in a bit.

greg k-h

Greg KH

2014-05-13 23:53 UTC #18 lore raw

Re: [PATCH -stable] x86,preempt: Fix preemption for i386

In reply to Peter Zijlstra

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Greg KH
Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Peter Zijlstra
I'm not entirely clear on how acceptable it is to propose a different patch for -stable than what we have upstream.
It's not all that common, but it certainly happens.
It's fine, as long as it mentions the commits that fix it upstream. And as long as it's well tested, of course.

Quoted 8 lines

Quoted 1 line
I agree, I can take this as long as you say it's correct and tested...
As far as I understand the issue the patch is indeed correct and I have 3 independent people who confirm their previously reported issues are now cured (as testified by the Tested-by tags).
There has also been confirmation that upstream does no longer suffer the problem.

Now applied, thanks.

greg k-h