x86/fpu: always restore_xinit_state() when !use_eager_cpu()
PATCH 1/1 x86/fpu: always restore_xinit_state() when !use_eager_cpu()
Oleg's commit f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()") removed drop_init_fpu() usage from flush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).
Note: used_math() looks at current, and should be switch to tsk_used_math(tsk), but even with this I see test suite breakage.
The functional change was that flush_thread after f893959b only calls restore_init_xstate when both !use_eager_fpu and !used_math are true. drop_init_fpu (now fpu_reset_state) calls restore_init_xstate() regardless of whether current used_math().
There was a lot of commentary on the initial patch - not sure I understand it all. Happy to get some pointers or be pointed to a better fix.
--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
arch/x86/kernel/process.c +7 -5
@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!used_math ()) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } }
Signature · 1 line
2.3.6
Quoted 5 lines · Bobby Powers
Oleg's commit
f893959b("x86/fpu: Don't abusedrop_init_fpu()inflush_thread()") removeddrop_init_fpu()usage fromflush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).
First of all, I really appreciate the bug report and the bisection. Thanks!
Which hardware was this seen on? Do you have any idea which part of the test suite failed, or what actually _caused_ it to fail?
Quoted 4 lines
The functional change was that
flush_threadafterf893959bonly callsrestore_init_xstatewhen both !use_eager_fpu and !used_math are true.drop_init_fpu(nowfpu_reset_state) callsrestore_init_xstate()regardless of whether currentused_math().
This is really interesting. We were seeing some issues where the xstate was not getting cleared across an exec, which seemed silly, but we just assumed it was something that had always been there.
BTW, I think restore_init_xstate() is probably buggy for the !fxsave and softfpu cases.
Quoted 7 lines
static inline void
restore_init_xstate(void){ if (use_xsave()) xrstor_state(init_xstate_buf, -1); else fxrstor_checking(&init_xstate_buf->i387); }
I'll do some testing of this today and make sure it doesn't break the things that I saw Oleg's patch "fix".
Quoted 11 lines · Dave Hansen
Quoted 4 lines · Bobby Powers
The functional change was that
flush_threadafterf893959bonly callsrestore_init_xstatewhen both !use_eager_fpu and !used_math are true.drop_init_fpu(nowfpu_reset_state) callsrestore_init_xstate()regardless of whether currentused_math().This is really interesting. We were seeing some issues where the xstate was not getting cleared across an exec, which seemed silly, but we just assumed it was something that had always been there.
This is because I am stupid.
Without this Bobby's fix flush_thread() simply does nothing if a user-space task execs (if eagerfpu).
Oleg.
Quoted 9 lines · Dave Hansen
Quoted 7 lines
Quoted 7 lines
static inline void
restore_init_xstate(void){ if (use_xsave()) xrstor_state(init_xstate_buf, -1); else fxrstor_checking(&init_xstate_buf->i387); }I'll do some testing of this today and make sure it doesn't break the things that I saw Oleg's patch "fix".
This looks OK to me. You can add my tested-by if you like.
Quoted 6 lines · Bobby Powers
Oleg's commit
f893959b("x86/fpu: Don't abusedrop_init_fpu()inflush_thread()") removeddrop_init_fpu()usage fromflush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).
Ah, sorry. And thanks a lot!
I simply can't understand what I was thinking about. Of course, we need to reset FPU state unconditionally, exec should start with the new FPU state.
ACK, but could you please fix the subject and update the changelog?
The subject says "when !use_eager_cpu()", but we need to do this if use_eager_cpu() == T.
Quoted 2 lines
Note:
used_math()looks at current, and should be switch totsk_used_math(tsk), but even with this I see test suite breakage.
Agreed, tsk_used_math(tsk) looks more consistent even if used_math() is technically correct. You can change this as well.
Thanks!
Quoted 29 lines
arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process.c +7
-5@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!used_math ()) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } }Signature · 1 line
2.3.6
Hello,
Quoted 4 lines · Oleg Nesterov
ACK, but could you please fix the subject and update the changelog?
The subject says "when !use_eager_cpu()", but we need to do this if
use_eager_cpu()== T.
Whoops, now its my turn to not know what I was thinking. I will update the subject/changelog, switch to tsk_used_math(), and send out a v2 shortly. Thanks for the quick reply + ack.
yours,
Bobby
PATCH v2 1/1 x86/fpu: always restore_xinit_state() when use_eager_cpu()
In reply to Oleg Nesterovv2: switch used_math() -> tsk_used_math(tsk) to consistently use the grabbed tsk instead of current, like in the rest of flush_thread().
Oleg's commit f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()") removed drop_init_fpu() usage from flush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).
The functional change was that flush_thread after f893959b only calls restore_init_xstate when both use_eager_fpu() and !used_math() are true. drop_init_fpu (now fpu_reset_state) calls restore_init_xstate() regardless of whether current used_math() - apply the same logic here.
--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
arch/x86/kernel/process.c +7 -5
@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!tsk_used_math (tsk)) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } }
Signature · 1 line
2.3.6
Re: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
Thanks!
Quoted 59 lines · Bobby Powers
v2: switch
used_math()->tsk_used_math(tsk)to consistently use the grabbed tsk instead of current, like in the rest offlush_thread().Oleg's commit
f893959b("x86/fpu: Don't abusedrop_init_fpu()inflush_thread()") removeddrop_init_fpu()usage fromflush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).The functional change was that
flush_threadafterf893959bonly callsrestore_init_xstatewhen bothuse_eager_fpu()and !used_math() are true.drop_init_fpu(nowfpu_reset_state) callsrestore_init_xstate()regardless of whether currentused_math()- apply the same logic here.Fixes f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")Signed-off-by Bobby Powers <[email protected]>Acked-by Oleg Nesterov <[email protected]>Cc Andi Kleen <[email protected]>Cc Borislav Petkov <[email protected]>Cc Ingo Molnar <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc Fenghua Yu <[email protected]>Cc Linus Torvalds <[email protected]>Cc Pekka Riikonen <[email protected]>Cc Quentin Casasnovas <[email protected]>Cc Rik van Riel <[email protected]>Cc Suresh Siddha <[email protected]>--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process.c +7
-6@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!tsk_used_math (tsk)) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } } --2.3.6
Bobby Powers
Tested-by Dave HansenRe: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
Hello,
Quoted 1 line · Oleg Nesterov
Thanks!
Dave gave this his:
over under v1 of this patch:
https://lkml.kernel.org/g/[email protected]
Anything else anyone sees, or anyone in particular I have to poke at to get this in? Ingo?
yours,
Bobby
Quoted 63 lines · Oleg Nesterov
Thanks!
Quoted 59 lines · Bobby Powers
v2: switch
used_math()->tsk_used_math(tsk)to consistently use the grabbed tsk instead of current, like in the rest offlush_thread().Oleg's commit
f893959b("x86/fpu: Don't abusedrop_init_fpu()inflush_thread()") removeddrop_init_fpu()usage fromflush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).The functional change was that
flush_threadafterf893959bonly callsrestore_init_xstatewhen bothuse_eager_fpu()and !used_math() are true.drop_init_fpu(nowfpu_reset_state) callsrestore_init_xstate()regardless of whether currentused_math()- apply the same logic here.Fixes f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")Signed-off-by Bobby Powers <[email protected]>Acked-by Oleg Nesterov <[email protected]>Cc Andi Kleen <[email protected]>Cc Borislav Petkov <[email protected]>Cc Ingo Molnar <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc Fenghua Yu <[email protected]>Cc Linus Torvalds <[email protected]>Cc Pekka Riikonen <[email protected]>Cc Quentin Casasnovas <[email protected]>Cc Rik van Riel <[email protected]>Cc Suresh Siddha <[email protected]>--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process.c +7
-6@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!tsk_used_math (tsk)) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } } --2.3.6
Re: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
Quoted 8 lines · Bobby Powers
Hello,
Quoted 1 line · Oleg Nesterov
Thanks!
Dave gave this his:
Quoted 7 lines
Tested-by Dave Hansen <[email protected]>over under v1 of this patch:
https://lkml.kernel.org/g/[email protected]Anything else anyone sees, or anyone in particular I have to poke at to get this in? Ingo?
Yes, please, 4.1 needs this patch.
Borislav, perhaps you can help ?
Quoted 68 lines
yours,
BobbyQuoted 63 lines · Oleg Nesterov
Thanks!
Quoted 59 lines · Bobby Powers
v2: switch
used_math()->tsk_used_math(tsk)to consistently use the grabbed tsk instead of current, like in the rest offlush_thread().Oleg's commit
f893959b("x86/fpu: Don't abusedrop_init_fpu()inflush_thread()") removeddrop_init_fpu()usage fromflush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).The functional change was that
flush_threadafterf893959bonly callsrestore_init_xstatewhen bothuse_eager_fpu()and !used_math() are true.drop_init_fpu(nowfpu_reset_state) callsrestore_init_xstate()regardless of whether currentused_math()- apply the same logic here.Fixes f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")Signed-off-by Bobby Powers <[email protected]>Acked-by Oleg Nesterov <[email protected]>Cc Andi Kleen <[email protected]>Cc Borislav Petkov <[email protected]>Cc Ingo Molnar <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc Fenghua Yu <[email protected]>Cc Linus Torvalds <[email protected]>Cc Pekka Riikonen <[email protected]>Cc Quentin Casasnovas <[email protected]>Cc Rik van Riel <[email protected]>Cc Suresh Siddha <[email protected]>--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)arch/x86/kernel/process.c +7
-6@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!tsk_used_math (tsk)) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } } --2.3.6
Borislav Petkov
AppliedRe: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
In reply to Bobby PowersQuoted 28 lines · Bobby Powers
v2: switch
used_math()->tsk_used_math(tsk)to consistently use the grabbed tsk instead of current, like in the rest offlush_thread().Oleg's commit
f893959b("x86/fpu: Don't abusedrop_init_fpu()inflush_thread()") removeddrop_init_fpu()usage fromflush_thread. This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).The functional change was that
flush_threadafterf893959bonly callsrestore_init_xstatewhen bothuse_eager_fpu()and !used_math() are true.drop_init_fpu(nowfpu_reset_state) callsrestore_init_xstate()regardless of whether currentused_math()- apply the same logic here.Fixes f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")Signed-off-by Bobby Powers <[email protected]>Acked-by Oleg Nesterov <[email protected]>Cc Andi Kleen <[email protected]>Cc Borislav Petkov <[email protected]>Cc Ingo Molnar <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc Fenghua Yu <[email protected]>Cc Linus Torvalds <[email protected]>Cc Pekka Riikonen <[email protected]>Cc Quentin Casasnovas <[email protected]>Cc Rik van Riel <[email protected]>Cc Suresh Siddha <[email protected]>
Quoted 3 lines
--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Applied, thanks.
Signature · 5 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--tip-bot for Bobby Powers
merge bot[tip:x86/urgent] x86/fpu: Always restore_xinit_state() when use_eager_cpu()
In reply to Bobby Powersx86/fpu: Always restore_xinit_state() when use_eager_cpu()
Commit-ID: c88d47480d300eaad80c213d50c9bf6077fc49bc Gitweb: http://git.kernel.org/tip/c88d47480d300eaad80c213d50c9bf6077fc49bc Author: Bobby Powers <[email protected]> AuthorDate: Mon, 27 Apr 2015 08:10:41 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 May 2015 11:22:03 +0200
x86/fpu: Always restore_xinit_state() when use_eager_cpu()
The following commit:
f893959b0898 ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")
removed drop_init_fpu() usage from flush_thread(). This seems to break things for me - the Go 1.4 test suite fails all over the place with floating point comparision errors (offending commit found through bisection).
The functional change was that flush_thread() after this commit only calls restore_init_xstate() when both use_eager_fpu() and !used_math() are true. drop_init_fpu() (now fpu_reset_state()) calls restore_init_xstate() regardless of whether current used_math() - apply the same logic here.
Switch used_math() -> tsk_used_math(tsk) to consistently use the grabbed tsk instead of current, like in the rest of flush_thread().
--- arch/x86/kernel/process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
arch/x86/kernel/process.c +7 -5
@@ -156,11 +156,13 @@ void flush_thread(void)/* FPU state will be reallocated lazily at the first use. */ drop_fpu (tsk);free_thread_xstate (tsk); - }else if (!used_math ()) { -/* kthread execs. TODO: cleanup this horror. */ -if (WARN_ON (init_fpu (tsk))) -force_sig (SIGKILL , tsk); -user_fpu_begin (); + }else { +if (!tsk_used_math (tsk)) { +/* kthread execs. TODO: cleanup this horror. */ +if (WARN_ON (init_fpu (tsk))) +force_sig (SIGKILL , tsk); +user_fpu_begin (); + }restore_init_xstate (); } }