linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu()
@ 2015-04-26 22:04 Bobby Powers
  2015-04-27 14:46 ` Dave Hansen
  2015-04-27 14:49 ` Oleg Nesterov
  0 siblings, 2 replies; 12+ messages in thread
From: Bobby Powers @ 2015-04-26 22:04 UTC (permalink / raw)
  To: linux-kernel
  Cc: Bobby Powers, x86, Oleg Nesterov, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Fenghua Yu, Linus Torvalds,
	Pekka Riikonen, Quentin Casasnovas, Rik van Riel, Suresh Siddha

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 <bobbypowers@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Pekka Riikonen <priikone@iki.fi>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>

---
 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu()
  2015-04-26 22:04 [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu() Bobby Powers
@ 2015-04-27 14:46 ` Dave Hansen
  2015-04-27 15:00   ` Oleg Nesterov
  2015-05-01 18:14   ` Dave Hansen
  2015-04-27 14:49 ` Oleg Nesterov
  1 sibling, 2 replies; 12+ messages in thread
From: Dave Hansen @ 2015-04-27 14:46 UTC (permalink / raw)
  To: Bobby Powers, linux-kernel
  Cc: x86, Oleg Nesterov, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Fenghua Yu, Linus Torvalds, Pekka Riikonen,
	Quentin Casasnovas, Rik van Riel, Suresh Siddha, Andi Kleen

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".


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu()
  2015-04-26 22:04 [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu() Bobby Powers
  2015-04-27 14:46 ` Dave Hansen
@ 2015-04-27 14:49 ` Oleg Nesterov
  2015-04-27 14:58   ` Bobby Powers
  2015-04-27 15:10   ` [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu() Bobby Powers
  1 sibling, 2 replies; 12+ messages in thread
From: Oleg Nesterov @ 2015-04-27 14:49 UTC (permalink / raw)
  To: Bobby Powers
  Cc: linux-kernel, x86, Borislav Petkov, Ingo Molnar, Andy Lutomirski,
	Dave Hansen, Fenghua Yu, Linus Torvalds, Pekka Riikonen,
	Quentin Casasnovas, Rik van Riel, Suresh Siddha

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
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu()
  2015-04-27 14:49 ` Oleg Nesterov
@ 2015-04-27 14:58   ` Bobby Powers
  2015-04-27 15:10   ` [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu() Bobby Powers
  1 sibling, 0 replies; 12+ messages in thread
From: Bobby Powers @ 2015-04-27 14:58 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Kernel development list, x86, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Fenghua Yu, Linus Torvalds,
	Pekka Riikonen, Quentin Casasnovas, Rik van Riel, Suresh Siddha

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu()
  2015-04-27 14:46 ` Dave Hansen
@ 2015-04-27 15:00   ` Oleg Nesterov
  2015-05-01 18:14   ` Dave Hansen
  1 sibling, 0 replies; 12+ messages in thread
From: Oleg Nesterov @ 2015-04-27 15:00 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Bobby Powers, linux-kernel, x86, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Fenghua Yu, Linus Torvalds, Pekka Riikonen,
	Quentin Casasnovas, Rik van Riel, Suresh Siddha, Andi Kleen

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
  2015-04-27 14:49 ` Oleg Nesterov
  2015-04-27 14:58   ` Bobby Powers
@ 2015-04-27 15:10   ` Bobby Powers
  2015-04-27 15:19     ` Oleg Nesterov
                       ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Bobby Powers @ 2015-04-27 15:10 UTC (permalink / raw)
  To: linux-kernel
  Cc: Bobby Powers, x86, Oleg Nesterov, Andi Kleen, Borislav Petkov,
	Ingo Molnar, Andy Lutomirski, Dave Hansen, Fenghua Yu,
	Linus Torvalds, Pekka Riikonen, Quentin Casasnovas, Rik van Riel,
	Suresh Siddha

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 <bobbypowers@gmail.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Pekka Riikonen <priikone@iki.fi>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
  2015-04-27 15:10   ` [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu() Bobby Powers
@ 2015-04-27 15:19     ` Oleg Nesterov
  2015-05-02 20:42       ` Bobby Powers
  2015-05-04  8:11     ` Borislav Petkov
  2015-05-06 10:14     ` [tip:x86/urgent] x86/fpu: Always " tip-bot for Bobby Powers
  2 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2015-04-27 15:19 UTC (permalink / raw)
  To: Bobby Powers
  Cc: linux-kernel, x86, Andi Kleen, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Fenghua Yu, Linus Torvalds,
	Pekka Riikonen, Quentin Casasnovas, Rik van Riel, Suresh Siddha

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 <bobbypowers@gmail.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Pekka Riikonen <priikone@iki.fi>
> Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Suresh Siddha <sbsiddha@gmail.com>
> ---
>  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
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu()
  2015-04-27 14:46 ` Dave Hansen
  2015-04-27 15:00   ` Oleg Nesterov
@ 2015-05-01 18:14   ` Dave Hansen
  1 sibling, 0 replies; 12+ messages in thread
From: Dave Hansen @ 2015-05-01 18:14 UTC (permalink / raw)
  To: Bobby Powers, linux-kernel
  Cc: x86, Oleg Nesterov, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Fenghua Yu, Linus Torvalds, Pekka Riikonen,
	Quentin Casasnovas, Rik van Riel, Suresh Siddha, Andi Kleen

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
  2015-04-27 15:19     ` Oleg Nesterov
@ 2015-05-02 20:42       ` Bobby Powers
  2015-05-03 17:39         ` Oleg Nesterov
  0 siblings, 1 reply; 12+ messages in thread
From: Bobby Powers @ 2015-05-02 20:42 UTC (permalink / raw)
  To: Oleg Nesterov, Ingo Molnar
  Cc: Kernel development list, x86, Andi Kleen, Borislav Petkov,
	Andy Lutomirski, Dave Hansen, Fenghua Yu, Linus Torvalds,
	Pekka Riikonen, Quentin Casasnovas, Rik van Riel, Suresh Siddha

Hello,

Oleg Nesterov <oleg@redhat.com> wrote:
> Thanks!

Dave gave this his:

Tested-By: Dave Hansen <dave.hansen@intel.com>

over under v1 of this patch:
https://lkml.kernel.org/g/5543C277.9070208@intel.com

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 <oleg@redhat.com> 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 <bobbypowers@gmail.com>
>> Acked-by: Oleg Nesterov <oleg@redhat.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Dave Hansen <dave.hansen@intel.com>
>> Cc: Fenghua Yu <fenghua.yu@intel.com>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Pekka Riikonen <priikone@iki.fi>
>> Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: Suresh Siddha <sbsiddha@gmail.com>
>> ---
>>  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
>>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
  2015-05-02 20:42       ` Bobby Powers
@ 2015-05-03 17:39         ` Oleg Nesterov
  0 siblings, 0 replies; 12+ messages in thread
From: Oleg Nesterov @ 2015-05-03 17:39 UTC (permalink / raw)
  To: Bobby Powers
  Cc: Ingo Molnar, Kernel development list, x86, Andi Kleen,
	Borislav Petkov, Andy Lutomirski, Dave Hansen, Fenghua Yu,
	Linus Torvalds, Pekka Riikonen, Quentin Casasnovas, Rik van Riel,
	Suresh Siddha

On 05/02, Bobby Powers wrote:
>
> Hello,
>
> Oleg Nesterov <oleg@redhat.com> wrote:
> > Thanks!
>
> Dave gave this his:
>
> Tested-By: Dave Hansen <dave.hansen@intel.com>
>
> over under v1 of this patch:
> https://lkml.kernel.org/g/5543C277.9070208@intel.com
>
> 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 <oleg@redhat.com> 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 <bobbypowers@gmail.com>
> >> Acked-by: Oleg Nesterov <oleg@redhat.com>
> >> Cc: Andi Kleen <ak@linux.intel.com>
> >> Cc: Borislav Petkov <bp@suse.de>
> >> Cc: Ingo Molnar <mingo@kernel.org>
> >> Cc: Andy Lutomirski <luto@amacapital.net>
> >> Cc: Dave Hansen <dave.hansen@intel.com>
> >> Cc: Fenghua Yu <fenghua.yu@intel.com>
> >> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> >> Cc: Pekka Riikonen <priikone@iki.fi>
> >> Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> >> Cc: Rik van Riel <riel@redhat.com>
> >> Cc: Suresh Siddha <sbsiddha@gmail.com>
> >> ---
> >>  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
> >>
> >


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu()
  2015-04-27 15:10   ` [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu() Bobby Powers
  2015-04-27 15:19     ` Oleg Nesterov
@ 2015-05-04  8:11     ` Borislav Petkov
  2015-05-06 10:14     ` [tip:x86/urgent] x86/fpu: Always " tip-bot for Bobby Powers
  2 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2015-05-04  8:11 UTC (permalink / raw)
  To: Bobby Powers
  Cc: linux-kernel, x86, Oleg Nesterov, Andi Kleen, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Fenghua Yu, Linus Torvalds,
	Pekka Riikonen, Quentin Casasnovas, Rik van Riel, Suresh Siddha

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 <bobbypowers@gmail.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Pekka Riikonen <priikone@iki.fi>
> Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Suresh Siddha <sbsiddha@gmail.com>
> ---
>  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.
--

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tip:x86/urgent] x86/fpu: Always restore_xinit_state() when use_eager_cpu()
  2015-04-27 15:10   ` [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu() Bobby Powers
  2015-04-27 15:19     ` Oleg Nesterov
  2015-05-04  8:11     ` Borislav Petkov
@ 2015-05-06 10:14     ` tip-bot for Bobby Powers
  2 siblings, 0 replies; 12+ messages in thread
From: tip-bot for Bobby Powers @ 2015-05-06 10:14 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: sbsiddha, mingo, quentin.casasnovas, bp, torvalds, bp,
	fenghua.yu, linux-kernel, tglx, riel, dave.hansen, priikone, hpa,
	luto, bobbypowers, oleg

Commit-ID:  c88d47480d300eaad80c213d50c9bf6077fc49bc
Gitweb:     http://git.kernel.org/tip/c88d47480d300eaad80c213d50c9bf6077fc49bc
Author:     Bobby Powers <bobbypowers@gmail.com>
AuthorDate: Mon, 27 Apr 2015 08:10:41 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
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 <dave.hansen@intel.com>
Signed-off-by: Bobby Powers <bobbypowers@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Pekka Riikonen <priikone@iki.fi>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: f893959b ("x86/fpu: Don't abuse drop_init_fpu() in flush_thread()")
Link: http://lkml.kernel.org/r/1430147441-9820-1-git-send-email-bobbypowers@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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();
 	}
 }

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-05-06 10:14 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-26 22:04 [PATCH] x86/fpu: always restore_xinit_state() when !use_eager_cpu() Bobby Powers
2015-04-27 14:46 ` Dave Hansen
2015-04-27 15:00   ` Oleg Nesterov
2015-05-01 18:14   ` Dave Hansen
2015-04-27 14:49 ` Oleg Nesterov
2015-04-27 14:58   ` Bobby Powers
2015-04-27 15:10   ` [PATCH v2] x86/fpu: always restore_xinit_state() when use_eager_cpu() Bobby Powers
2015-04-27 15:19     ` Oleg Nesterov
2015-05-02 20:42       ` Bobby Powers
2015-05-03 17:39         ` Oleg Nesterov
2015-05-04  8:11     ` Borislav Petkov
2015-05-06 10:14     ` [tip:x86/urgent] x86/fpu: Always " tip-bot for Bobby Powers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).