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.
Signed-off-by: Bobby Powers <[email protected]>
Cc: Oleg Nesterov <[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(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 8213da6..c820baf5 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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();
}
}
--
2.3.6
On 04/26/2015 03:04 PM, Bobby Powers wrote:
> 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).
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?
> 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().
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.
> 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".
On 04/26, Bobby Powers wrote:
>
> 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).
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.
> Note: used_math() looks at current, and should be switch to
> tsk_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!
> arch/x86/kernel/process.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 8213da6..c820baf5 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -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();
> }
> }
> --
> 2.3.6
>
Hello,
On 4/27, Oleg Nesterov wrote:
> 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
On 04/27, Dave Hansen wrote:
>
> On 04/26/2015 03:04 PM, Bobby Powers wrote:
>
> > 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().
>
> 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.
v2: 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.
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(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 8213da6..1a6fcf8 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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
Thanks!
On 04/27, Bobby Powers wrote:
>
> v2: 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.
>
> 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(-)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 8213da6..1a6fcf8 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -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
>
On 04/27/2015 07:46 AM, Dave Hansen wrote:
>> > 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.
Hello,
Oleg Nesterov <[email protected]> wrote:
> Thanks!
Dave gave this his:
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?
yours,
Bobby
On Mon, Apr 27, 2015 at 11:19 AM, Oleg Nesterov <[email protected]> wrote:
> Thanks!
>
> On 04/27, Bobby Powers wrote:
>>
>> v2: 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.
>>
>> 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(-)
>>
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 8213da6..1a6fcf8 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -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
>>
>
On 05/02, Bobby Powers wrote:
>
> Hello,
>
> Oleg Nesterov <[email protected]> wrote:
> > Thanks!
>
> Dave gave this his:
>
> 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 ?
>
> yours,
> Bobby
>
> On Mon, Apr 27, 2015 at 11:19 AM, Oleg Nesterov <[email protected]> wrote:
> > Thanks!
> >
> > On 04/27, Bobby Powers wrote:
> >>
> >> v2: 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.
> >>
> >> 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(-)
> >>
> >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> >> index 8213da6..1a6fcf8 100644
> >> --- a/arch/x86/kernel/process.c
> >> +++ b/arch/x86/kernel/process.c
> >> @@ -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
> >>
> >
On Mon, Apr 27, 2015 at 08:10:41AM -0700, Bobby Powers wrote:
> v2: 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.
>
> 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(-)
Applied, thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
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().
Tested-by: Dave Hansen <[email protected]>
Signed-off-by: Bobby Powers <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: H. Peter Anvin <[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]>
Cc: Thomas Gleixner <[email protected]>
Fixes: f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/process.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index bfc99b3..6e338e3 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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();
}
}