question about save_xstate_sig() - WHY DOES THIS WORK?

question about save_xstate_sig() - WHY DOES THIS WORK?

[Rik van Riel]

While working on a patch series to defer FPU state loading until
kernel -> user space transition, and be more lazy with FPU state
while in the kernel, I came across this code in save_xstate_sig().

Not only is this broken with my new code, but it looks like it may
be broken with the current code, too...

Specifically, save_user_xstate() may page fault and sleep. After
returning from the page fault, there is no guarantee that the
FPU state will be restored into the CPU, when the system is not
running with eager fpu mode.

In that case, what prevents us from saving random FPU register state
to the user's stack frame?  Potentially state containing data from
other programs...

        if (user_has_fpu()) {
                /* Save the live register state to the user directly. */
                if (save_user_xstate(buf_fx))
                        return -1;
                /* Update the thread's fxstate to save the fsave header. */
                if (ia32_fxstate)
                        fpu_fxsave(&tsk->thread.fpu);
        } else {
                sanitize_i387_state(tsk);
                if (__copy_to_user(buf_fx, xsave, xstate_size))
                        return -1;
        }

Is this code safe for some reason I have overlooked?

If not, should I post the patch turning the above into "save FPU
state atomically, then copy it to user space" independently from
my optimizations, and submit it for inclusion in -stable as well?

[PATCH, RFC] x86,fpu: make signal handling xstate save & restore preemption safe

[Rik van Riel]

Saving xstate directly to userspace, or restoring it directly from
userspace could result in a page fault, which can lead to the task
context switching and sleeping.

There is no guarantee that after the context switch the FPU state
for the task will be loaded back into the FPU registers, and it
looks like that could potentially cause corruption of the FPU
state.

The straightforward solution is to do the FPU save and restore
with preemption disabled, which requires using a kernel buffer.
That in turn allows us to use the same save and restore routines
that are used at context switch and math exception time, allowing
us to remove the direct-to-userspace variants.

I have only tested this code as part of my larger FPU optimization
series, not yet stand-alone.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 14 ++++----
 arch/x86/kernel/xsave.c             | 72 ++++++++++---------------------------
 2 files changed, 26 insertions(+), 60 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 68089ef86907..724f3e4faf34 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -508,16 +508,18 @@ static inline void __save_fpu(struct task_struct *tsk)
  */
 static inline void save_init_fpu(struct task_struct *tsk)
 {
-	WARN_ON_ONCE(!__thread_has_fpu(tsk));
+	preempt_disable();
+
+	if (!__thread_has_fpu(tsk))
+		goto out;
 
-	if (use_eager_fpu()) {
+	if (use_eager_fpu())
 		__save_fpu(tsk);
-		return;
-	}
+	else
+		__save_init_fpu(tsk);
 
-	preempt_disable();
-	__save_init_fpu(tsk);
 	__thread_fpu_end(tsk);
+ out:
 	preempt_enable();
 }
 
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index de9dcf89a302..fc2d30c75e64 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -198,22 +198,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 	return err;
 }
 
-static inline int save_user_xstate(struct xsave_struct __user *buf)
-{
-	int err;
-
-	if (use_xsave())
-		err = xsave_user(buf);
-	else if (use_fxsr())
-		err = fxsave_user((struct i387_fxsave_struct __user *) buf);
-	else
-		err = fsave_user((struct i387_fsave_struct __user *) buf);
-
-	if (unlikely(err) && __clear_user(buf, xstate_size))
-		err = -EFAULT;
-	return err;
-}
-
 /*
  * Save the fpu, extended register state to the user signal frame.
  *
@@ -251,18 +235,10 @@ int save_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_ia32 __user *) buf) ? -1 : 1;
 
-	if (user_has_fpu()) {
-		/* Save the live register state to the user directly. */
-		if (save_user_xstate(buf_fx))
-			return -1;
-		/* Update the thread's fxstate to save the fsave header. */
-		if (ia32_fxstate)
-			fpu_fxsave(&tsk->thread.fpu);
-	} else {
-		sanitize_i387_state(tsk);
-		if (__copy_to_user(buf_fx, xsave, xstate_size))
-			return -1;
-	}
+	/* Then copy it to userspace. */
+	sanitize_i387_state(tsk);
+	if (__copy_to_user(buf_fx, xsave, xstate_size))
+		return -1;
 
 	/* Save the fsave header for the 32-bit frames. */
 	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))
@@ -307,28 +283,6 @@ sanitize_restored_xstate(struct task_struct *tsk,
 	}
 }
 
-/*
- * Restore the extended state if present. Otherwise, restore the FP/SSE state.
- */
-static inline int restore_user_xstate(void __user *buf, u64 xbv, int fx_only)
-{
-	if (use_xsave()) {
-		if ((unsigned long)buf % 64 || fx_only) {
-			u64 init_bv = pcntxt_mask & ~XSTATE_FPSSE;
-			xrstor_state(init_xstate_buf, init_bv);
-			return fxrstor_user(buf);
-		} else {
-			u64 init_bv = pcntxt_mask & ~xbv;
-			if (unlikely(init_bv))
-				xrstor_state(init_xstate_buf, init_bv);
-			return xrestore_user(buf, xbv);
-		}
-	} else if (use_fxsr()) {
-		return fxrstor_user(buf);
-	} else
-		return frstor_user(buf);
-}
-
 int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 {
 	int ia32_fxstate = (buf != buf_fx);
@@ -408,15 +362,25 @@ int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 
 		return err;
 	} else {
+		struct xsave_struct *xsave = &tsk->thread.fpu.state->xsave;
 		/*
-		 * For 64-bit frames and 32-bit fsave frames, restore the user
-		 * state to the registers directly (with exceptions handled).
+		 * Copy the xstate from user space into the kernel buffer.
+		 * Clear task used math during the operation, to ensure the
+		 * context switching code does not overwrite the xstate buffer
+		 * with whatever is in the FPU registers.
 		 */
-		user_fpu_begin();
-		if (restore_user_xstate(buf_fx, xstate_bv, fx_only)) {
+		drop_fpu(tsk);
+		if (__copy_from_user(xsave, buf_fx, state_size)) {
 			drop_init_fpu(tsk);
 			return -1;
 		}
+		set_used_math();
+
+		if (use_eager_fpu()) {
+			preempt_disable();
+			math_state_restore();
+			preempt_enable();
+		}
 	}
 
 	return 0;

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[H. Peter Anvin]

On 01/23/2015 11:34 AM, Rik van Riel wrote:
> While working on a patch series to defer FPU state loading until
> kernel -> user space transition, and be more lazy with FPU state
> while in the kernel, I came across this code in save_xstate_sig().
> 
> Not only is this broken with my new code, but it looks like it may
> be broken with the current code, too...
> 
> Specifically, save_user_xstate() may page fault and sleep. After
> returning from the page fault, there is no guarantee that the
> FPU state will be restored into the CPU, when the system is not
> running with eager fpu mode.
> 
> In that case, what prevents us from saving random FPU register state
> to the user's stack frame?  Potentially state containing data from
> other programs...
> 

If the FPU state is not current, we'll have CR0.TS = 1 and the XSAVE
will cause an #NM exception, which will cause the FPU state to be
swapped in.

	-hpa

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/23/2015 04:07 PM, H. Peter Anvin wrote:
> On 01/23/2015 11:34 AM, Rik van Riel wrote:
>> While working on a patch series to defer FPU state loading until 
>> kernel -> user space transition, and be more lazy with FPU state 
>> while in the kernel, I came across this code in
>> save_xstate_sig().
>> 
>> Not only is this broken with my new code, but it looks like it
>> may be broken with the current code, too...
>> 
>> Specifically, save_user_xstate() may page fault and sleep. After 
>> returning from the page fault, there is no guarantee that the FPU
>> state will be restored into the CPU, when the system is not 
>> running with eager fpu mode.
>> 
>> In that case, what prevents us from saving random FPU register
>> state to the user's stack frame?  Potentially state containing
>> data from other programs...
>> 
> 
> If the FPU state is not current, we'll have CR0.TS = 1 and the
> XSAVE will cause an #NM exception, which will cause the FPU state
> to be swapped in.

OK, so the code works as it is right now, but I still need
to have the patch in my "defer FPU state loading and CR0.TS
switching to kernel -> user space transition" patch series.

Thanks for explaining why the current code is safe, Peter.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUw6CNAAoJEM553pKExN6DIkIH/jYpLRfVSbx+JBuFtOibA6jk
wwWnwNdPRUiWf0v0lSNX4phk9f7VCy8WKbrFtC0IYdGLL0zCY94dN7us4hNYo9fE
btdb/qsvqq8w2esPuUczAgaxwipYVliDUDOHSXTRbPS2upBuLHl/CVIVb1A+HVC7
gB6GF4ZLa2P1ygcjuWDNDNtx04QWTi7K80eWtWAQ7U4OK9Y4fSgR3hOIJS79M/aJ
P9YGaTyo0zDQcKx1rFXTjl9A0slCFZ+QkAeCwLZRkXc1K9n50GANuYeg53/ATYey
7x0FjZEBD2ipX60FqZ+mYAiA7rqw4RYKgAttszy9b9bgKY/fzHy87z2YS8Umplg=
=55wt
-----END PGP SIGNATURE-----

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Oleg Nesterov]

Let me abuse this thread to ask more questions.

Peter, could you help?

On 01/23, Rik van Riel wrote:
>
> Not only is this broken with my new code, but it looks like it may
> be broken with the current code, too...

As I already mentioned, at least math_error()->save_init_fpu() looks
buggy. And unlazy_fpu() doesn't look right too.

Note that save_init_fpu() is calles after conditional_sti(), so unless
I missed something the task can be preempted and we can actually hit
WARN_ON_ONCE(!__thread_has_fpu()) if !use_eager_fpu() && .fpu_counter == 0.

Worse, the unconditional __save_init_fpu() is obviously wrong in this case.

I already have a patch which (like the patch from Rik) turns it into

	static inline void save_init_fpu(struct task_struct *tsk)
	{
		preempt_disable();
		if (__thread_has_fpu(tsk)) {
			if (use_eager_fpu()) {
				__save_fpu(tsk);
			} else {
				__save_init_fpu(tsk);
				__thread_fpu_end(tsk);
			}
		}
		preempt_enable();
	}

and I think this fix needs the separate patch/changelog.

Now the questions:

- This doesn't hurt, but does it really need __thread_fpu_end?

  Perhaps this is because we do not check the error code returned
  by __save_init_fpu? although I am not sure I understand the comment
  above fpu_save_init correctly...

- What about do_bounds() ? Should not it use save_init_fpu() rather
  than fpu_save_init() ?

- Why unlazy_fpu() always does __save_init_fpu() even if use_eager_fpu?

  and note that in this case __thread_fpu_end() is wrong if use_eager_fpu,
  but fortunately the only possible caller of unlazy_fpu() is coredump.
  fpu_copy() checks use_eager_fpu().

- Is unlazy_fpu()->__save_init_fpu() safe wrt __kernel_fpu_begin() from
  irq?

  I mean, is it safe if __save_init_fpu() path is interrupted by another
  __save_init_fpu() + restore_fpu_checking() from __kernel_fpu_begin/end?

Thanks,

Oleg.

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/24/2015 03:20 PM, Oleg Nesterov wrote:
> Let me abuse this thread to ask more questions.
> 
> Peter, could you help?
> 
> On 01/23, Rik van Riel wrote:
>> 
>> Not only is this broken with my new code, but it looks like it
>> may be broken with the current code, too...
> 
> As I already mentioned, at least math_error()->save_init_fpu()
> looks buggy. And unlazy_fpu() doesn't look right too.
> 
> Note that save_init_fpu() is calles after conditional_sti(), so
> unless I missed something the task can be preempted and we can
> actually hit WARN_ON_ONCE(!__thread_has_fpu()) if !use_eager_fpu()
> && .fpu_counter == 0.
> 
> Worse, the unconditional __save_init_fpu() is obviously wrong in
> this case.
> 
> I already have a patch which (like the patch from Rik) turns it
> into
> 
> static inline void save_init_fpu(struct task_struct *tsk) { 
> preempt_disable(); if (__thread_has_fpu(tsk)) { if
> (use_eager_fpu()) { __save_fpu(tsk); } else { 
> __save_init_fpu(tsk); __thread_fpu_end(tsk); } } preempt_enable(); 
> }

> Now the questions:
> 
> - This doesn't hurt, but does it really need __thread_fpu_end?
> 
> Perhaps this is because we do not check the error code returned by
> __save_init_fpu? although I am not sure I understand the comment 
> above fpu_save_init correctly...

Looking at the code some more, I do not see any call site of
save_init_fpu() that actually needs or wants __thread_fpu_end(),
with or without eager fpu mode.

It looks like we can get rid of that.

> - What about do_bounds() ? Should not it use save_init_fpu()
> rather than fpu_save_init() ?

I suppose do_bounds() probably should save the fpu context while
not preemptible, but that may also mean moving conditional_sti()
until after save_init_fpu() or __save_init_fpu() has been called.

> - Why unlazy_fpu() always does __save_init_fpu() even if
> use_eager_fpu?
> 
> and note that in this case __thread_fpu_end() is wrong if
> use_eager_fpu, but fortunately the only possible caller of
> unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().
> 
> - Is unlazy_fpu()->__save_init_fpu() safe wrt __kernel_fpu_begin()
> from irq?
> 
> I mean, is it safe if __save_init_fpu() path is interrupted by
> another __save_init_fpu() + restore_fpu_checking() from
> __kernel_fpu_begin/end?

I got lost in the core dump code trying to figure out whether this is
safe or broken. I'll need some more time to look through that code...

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUxs1kAAoJEM553pKExN6DacoH/jlSeftktuzKNN1lc8f1o1Uw
3f4i/SLjleHa00xayaG2RMrYpRtMAVMHqgG+3ltmF9cHZj3LUrYl8p5QlQTO+jMS
53B/U/GCHrBWyziQgUHvGmw6WyVSDlTEej0gb91WW0pKEvuUrDdCTTwhNFqp649b
jRw5F+LGIvYB99ICI5hLEMzbbKhMOpyiG4c3qmU41xsfnEWly50YdFKfetXm79E0
MF1xN4trwqv7JOoBGfKwH8aUGe/n6B9e/QHAu7JMIuryjZK/cSug/4lH0QR0xMni
NUzqKaE8xCDW5LQMLAg+7ZYhvdR/o3EbV4Lk90RCBF1KTTSFKorhUavwZLu/M3M=
=QlMj
-----END PGP SIGNATURE-----

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Oleg Nesterov]

On 01/26, Rik van Riel wrote:
>
> On 01/24/2015 03:20 PM, Oleg Nesterov wrote:
>
> > Now the questions:
> >
> > - This doesn't hurt, but does it really need __thread_fpu_end?
> >
> > Perhaps this is because we do not check the error code returned by
> > __save_init_fpu? although I am not sure I understand the comment
> > above fpu_save_init correctly...
>
> Looking at the code some more, I do not see any call site of
> save_init_fpu() that actually needs or wants __thread_fpu_end(),
> with or without eager fpu mode.

Yes. But probably it is needed if __save_init_fpu() returns 0.
But this is minor, __thread_fpu_end() doesn't hurt correctness-wise
if !eager.

> It looks like we can get rid of that.

Agreed, but probably this needs a separate change.

> > - What about do_bounds() ? Should not it use save_init_fpu()
> > rather than fpu_save_init() ?
>
> I suppose do_bounds() probably should save the fpu context while
> not preemptible,

plus it also needs the __thread_has_fpu() check. Otherwise fpu_save_init()
can save the wrong FPU state afaics.

> but that may also mean moving conditional_sti()
> until after save_init_fpu() or __save_init_fpu() has been called.

Agreed, this can work too.

> > - Why unlazy_fpu() always does __save_init_fpu() even if
> > use_eager_fpu?
> >
> > and note that in this case __thread_fpu_end() is wrong if
> > use_eager_fpu, but fortunately the only possible caller of
> > unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().
> >
> > - Is unlazy_fpu()->__save_init_fpu() safe wrt __kernel_fpu_begin()
> > from irq?
> >
> > I mean, is it safe if __save_init_fpu() path is interrupted by
> > another __save_init_fpu() + restore_fpu_checking() from
> > __kernel_fpu_begin/end?
>
> I got lost in the core dump code trying to figure out whether this is
> safe or broken. I'll need some more time to look through that code...

It is called indirectly by regset code, see xstateregs_get()->init_fpu().

The coredumping task can't return to user-mode and use FPU in this case,
so this is not that bad. Still unlazy_fpu()->__thread_fpu_end() is wrong
if eager.

And I forgot to mention, the "else" branch in unlazy_fpu() makes no sense.
And note that save_init_fpu() and unlazy_fpu() is the same thing (if we
fix/cleanup them).

Oh. I'll try to finish my cleanups and send them tomorrow. Unless you
do this ;)

Oleg.

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/27/2015 02:40 PM, Oleg Nesterov wrote:
> On 01/26, Rik van Riel wrote:
>> 
>> On 01/24/2015 03:20 PM, Oleg Nesterov wrote:
>> 
>>> Now the questions:
>>> 
>>> - This doesn't hurt, but does it really need __thread_fpu_end?
>>> 
>>> Perhaps this is because we do not check the error code returned
>>> by __save_init_fpu? although I am not sure I understand the
>>> comment above fpu_save_init correctly...
>> 
>> Looking at the code some more, I do not see any call site of 
>> save_init_fpu() that actually needs or wants __thread_fpu_end(), 
>> with or without eager fpu mode.
> 
> Yes. But probably it is needed if __save_init_fpu() returns 0. But
> this is minor, __thread_fpu_end() doesn't hurt correctness-wise if
> !eager.
> 
>> It looks like we can get rid of that.
> 
> Agreed, but probably this needs a separate change.
> 
>>> - What about do_bounds() ? Should not it use save_init_fpu() 
>>> rather than fpu_save_init() ?
>> 
>> I suppose do_bounds() probably should save the fpu context while 
>> not preemptible,
> 
> plus it also needs the __thread_has_fpu() check. Otherwise
> fpu_save_init() can save the wrong FPU state afaics.
> 
>> but that may also mean moving conditional_sti() until after
>> save_init_fpu() or __save_init_fpu() has been called.
> 
> Agreed, this can work too.
> 
>>> - Why unlazy_fpu() always does __save_init_fpu() even if 
>>> use_eager_fpu?
>>> 
>>> and note that in this case __thread_fpu_end() is wrong if 
>>> use_eager_fpu, but fortunately the only possible caller of 
>>> unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().
>>> 
>>> - Is unlazy_fpu()->__save_init_fpu() safe wrt
>>> __kernel_fpu_begin() from irq?

It looks like it should be safe, as long as __save_init_fpu()
knows that the task no longer has the FPU after __kernel_fpu_end(),
so it does not try to save the kernel FPU state to the user's
task->thread.fpu.state->xstate

The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end()
needs to be kept from running during unlazy_fpu().

This means interrupted_kernel_fpu_idle and/or irq_fpu_usable
need to check whether preemption is disabled, and lock out
__kernel_fpu_begin() when preemption is disabled.

It does not look like it currently does that...

>>> I mean, is it safe if __save_init_fpu() path is interrupted by 
>>> another __save_init_fpu() + restore_fpu_checking() from 
>>> __kernel_fpu_begin/end?
>> 
>> I got lost in the core dump code trying to figure out whether
>> this is safe or broken. I'll need some more time to look through
>> that code...
> 
> It is called indirectly by regset code, see
> xstateregs_get()->init_fpu().
> 
> The coredumping task can't return to user-mode and use FPU in this
> case, so this is not that bad. Still
> unlazy_fpu()->__thread_fpu_end() is wrong if eager.
> 
> And I forgot to mention, the "else" branch in unlazy_fpu() makes no
> sense. And note that save_init_fpu() and unlazy_fpu() is the same
> thing (if we fix/cleanup them).

I was wondering why there were several functions doing essentially
the same thing...

> Oh. I'll try to finish my cleanups and send them tomorrow. Unless
> you do this ;)

If you tell me what you would like to see done, I'd be more than
happy to do it :)

I can certainly merge unlazy_fpu() and save_init_fpu() into the
same function, but I am not sure whether or not it should call
__thread_fpu_end() - it looks like that would be desirable in some
cases, but not in others...

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUx/S6AAoJEM553pKExN6DCJwH/0US/6JXKxX0vYOuaw9SKzhX
fgkxWbHFtJ7qn/tXBJqKgQQr5RIJ8dH6opoGNzzeOGeNMISoo9EZrVYxO1mv/Lrk
GoPjFDVyhm/hc74Bvpm7Xtzai7JJanTyLj63pLPu4wm+0+QKPEoRUMvtyLuLe0nM
5GMpnW0wWZn/c0JWLihfKRCK5FecP9Tv9y/1gXGkLWymMw8PDpXxIqo+VJJM86ow
eUrPTFHeAvYh1m0lsxOr4JMUB5+VeZV9zXNPefNHlMcBNchVGvDhxWpdqtGyROd4
A+tMBM4EymrJoTeJrcxuFSdKFcCW0T/9JOHm3tb4B9Qjsk7/ROzp0d/s7bKyyKw=
=9h6C
-----END PGP SIGNATURE-----

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/27/2015 03:27 PM, Rik van Riel wrote:
> On 01/27/2015 02:40 PM, Oleg Nesterov wrote:

>>>> - Why unlazy_fpu() always does __save_init_fpu() even if 
>>>> use_eager_fpu?
>>>> 
>>>> and note that in this case __thread_fpu_end() is wrong if 
>>>> use_eager_fpu, but fortunately the only possible caller of 
>>>> unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().
>>>> 
>>>> - Is unlazy_fpu()->__save_init_fpu() safe wrt 
>>>> __kernel_fpu_begin() from irq?
> 
> It looks like it should be safe, as long as __save_init_fpu() knows
> that the task no longer has the FPU after __kernel_fpu_end(), so it
> does not try to save the kernel FPU state to the user's 
> task->thread.fpu.state->xstate
> 
> The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end() 
> needs to be kept from running during unlazy_fpu().
> 
> This means interrupted_kernel_fpu_idle and/or irq_fpu_usable need
> to check whether preemption is disabled, and lock out 
> __kernel_fpu_begin() when preemption is disabled.
> 
> It does not look like it currently does that...

... and that won't work, because preempt_disable() is a noop
without CONFIG_PREEMPT enabled. Sigh.

Not sure how to work around that, except by having
__Kernel_fpu_end() always restore the task FPU state, if the
task had the FPU when entering.

This suggests it may be advantageous to be liberal with
__thread_fpu_end() when we know user space will not be touching
the FPU again for a while, since that would reduce the work
done by __kernel_fpu_begin()/__kernel_fpu_end().

Does that make sense?

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUx/oJAAoJEM553pKExN6Dh4YH/AtVOza5VeZuz55s59LifpLt
5eXMQBxQt0JiX9NACc+7qXDuEFfkmnzhNpNT98abuic7/1dvj2TfcoXSnkysDaNb
WojFW0gMc973G4UA6ZPsqScxz7jdfaqNrFdFEaTPM6URClIllVXTgnCcDoWCFfR2
627s6vyOW//BgXnedHraGGqnBVpCBLWrsVnhJ0RbPd2cjrrwKPXcbLnk6+uzZqLY
/7Zp2fSDjuAY6PhxRZM529vyJNqIM2mhlKFG8/xnch95mlXwtOQvLuuJmkwbKabb
6Z1gqpyEXbWyB2riD/pQrIlkOg2P4UuLONM7fgFY4Dzc8MEM7UmNR4mWoPAeBuU=
=SSqD
-----END PGP SIGNATURE-----

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Oleg Nesterov]

On 01/27, Rik van Riel wrote:
>
> On 01/27/2015 03:27 PM, Rik van Riel wrote:
> > On 01/27/2015 02:40 PM, Oleg Nesterov wrote:
>
> >>>> - Why unlazy_fpu() always does __save_init_fpu() even if
> >>>> use_eager_fpu?
> >>>>
> >>>> and note that in this case __thread_fpu_end() is wrong if
> >>>> use_eager_fpu, but fortunately the only possible caller of
> >>>> unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().
> >>>>
> >>>> - Is unlazy_fpu()->__save_init_fpu() safe wrt
> >>>> __kernel_fpu_begin() from irq?
> >
> > It looks like it should be safe, as long as __save_init_fpu() knows
> > that the task no longer has the FPU after __kernel_fpu_end(), so it
> > does not try to save the kernel FPU state to the user's
> > task->thread.fpu.state->xstate
> >
> > The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end()
> > needs to be kept from running during unlazy_fpu().
> >
> > This means interrupted_kernel_fpu_idle and/or irq_fpu_usable need
> > to check whether preemption is disabled, and lock out
> > __kernel_fpu_begin() when preemption is disabled.
> >
> > It does not look like it currently does that...
>
> ... and that won't work, because preempt_disable() is a noop
> without CONFIG_PREEMPT enabled. Sigh.
>
> Not sure how to work around that, except by having
> __Kernel_fpu_end() always restore the task FPU state, if the
> task had the FPU when entering.

This is what it does after

	[PATCH 2/3] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_{begin,end}()
	http://marc.info/?l=linux-kernel&m=142134967718861&w=2

(acked by you and already applied).

But probably I misunderstood you, I do not see how this can help...
OK, lets discuss this later.

Oleg.

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Oleg Nesterov]

On 01/27, Rik van Riel wrote:
>
> On 01/27/2015 02:40 PM, Oleg Nesterov wrote:
> >>>
> >>> - Is unlazy_fpu()->__save_init_fpu() safe wrt
> >>> __kernel_fpu_begin() from irq?
>
> It looks like it should be safe, as long as __save_init_fpu()
> knows that the task no longer has the FPU after __kernel_fpu_end(),
> so it does not try to save the kernel FPU state to the user's
> task->thread.fpu.state->xstate

Not sure this is enough, but...

> The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end()
> needs to be kept from running during unlazy_fpu().

Yes,

> This means interrupted_kernel_fpu_idle and/or irq_fpu_usable
> need to check whether preemption is disabled, and lock out
> __kernel_fpu_begin() when preemption is disabled.

But we already have kernel_fpu_disable/enable. unlazy_cpu() can use
it to avoid the race ?

> I can certainly merge unlazy_fpu() and save_init_fpu() into the
> same function, but I am not sure whether or not it should call
> __thread_fpu_end() - it looks like that would be desirable in some
> cases, but not in others...

I _think_ that we never actually want __thread_fpu_end(), although it
doesn't really hurt if !eager. Probably ulazy/save should do

	if (!__save_init_fpu())
		__thread_fpu_end();

But again, this is minor.

Oleg.

Re: question about save_xstate_sig() - WHY DOES THIS WORK?

[Rik van Riel]

On 01/29/2015 03:45 PM, Oleg Nesterov wrote:
> On 01/27, Rik van Riel wrote:
>>
>> On 01/27/2015 02:40 PM, Oleg Nesterov wrote:
>>>>>
>>>>> - Is unlazy_fpu()->__save_init_fpu() safe wrt
>>>>> __kernel_fpu_begin() from irq?
>>
>> It looks like it should be safe, as long as __save_init_fpu()
>> knows that the task no longer has the FPU after __kernel_fpu_end(),
>> so it does not try to save the kernel FPU state to the user's
>> task->thread.fpu.state->xstate
> 
> Not sure this is enough, but...
> 
>> The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end()
>> needs to be kept from running during unlazy_fpu().
> 
> Yes,
> 
>> This means interrupted_kernel_fpu_idle and/or irq_fpu_usable
>> need to check whether preemption is disabled, and lock out
>> __kernel_fpu_begin() when preemption is disabled.
> 
> But we already have kernel_fpu_disable/enable. unlazy_cpu() can use
> it to avoid the race ?

I suspect this will be fine, if __kernel_fpu_end()
from IRQ context always restores the FPU context,
or calls stts, so things like save_init_fpu() will
either continue where they left off, or trap and
then continue where they left off.

__kernel_fpu_end() from process context can be
lazier, something I can work on in my next version
of the "defer FPU loading to kernel -> user space
boundary" patch series.

>> I can certainly merge unlazy_fpu() and save_init_fpu() into the
>> same function, but I am not sure whether or not it should call
>> __thread_fpu_end() - it looks like that would be desirable in some
>> cases, but not in others...
> 
> I _think_ that we never actually want __thread_fpu_end(), although it
> doesn't really hurt if !eager. Probably ulazy/save should do
> 
> 	if (!__save_init_fpu())
> 		__thread_fpu_end();

There is at least one case where we want __thread_fpu_end(),
and that is xstateregs_set.  I got this by moving the
__thread_fpu_end() call from save_init_fpu() into init_fpu().

I am not sure about __math_error. I suspect we may need
__thread_fpu_end() in there so the math state can be
re-initialized if the task catches SIGFPE and continues.

On the other hand, I do not see code in there that
actually does that at the moment...

Let me send my RFC patch to clean up & merge
unlazy_fpu and save_init_fpu() in the next email.

[PATCH RFC] x86,fpu: merge save_init_fpu & unlazy_fpu

[Rik van Riel]

The functions save_init_fpu and unlazy_fpu do essentially the
same thing: save the fpu context to memory, and call __thread_fpu_end,
which most of the callers do not need or want.

Get rid of the function unlazy_fpu and make sure save_init_fpu does
what unlazy_fpu does today, including preemption safe state saving
in potentially unusual conditions.

Callers of init_fpu do want __thread_fpu_end, so move the call to
__thread_fpu_end into init_fpu.

I am not sure whether math_error requires __thread_fpu_end. One would
think that it would require that in order to sanitize the math state,
before do_device_not_available and math_state_restore restore it,
but those do not currently seem to do that. I am not sure how a task
that catches SIGFPE is supposed to recover and continue...

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 18 ++++++++----------
 arch/x86/include/asm/i387.h         |  2 --
 arch/x86/kernel/i387.c              | 18 ++++--------------
 arch/x86/kernel/traps.c             |  1 +
 4 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 0dbc08282291..c7e440ddd269 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -524,16 +524,14 @@ static inline void __save_fpu(struct task_struct *tsk)
  */
 static inline void save_init_fpu(struct task_struct *tsk)
 {
-	WARN_ON_ONCE(!__thread_has_fpu(tsk));
-
-	if (use_eager_fpu()) {
-		__save_fpu(tsk);
-		return;
-	}
-
 	preempt_disable();
-	__save_init_fpu(tsk);
-	__thread_fpu_end(tsk);
+	if (__thread_has_fpu(tsk)) {
+		if (use_eager_fpu())
+			__save_fpu(tsk);
+		else
+			__save_init_fpu(tsk);
+	} else if (!use_eager_fpu())
+		tsk->thread.fpu_counter = 0;
 	preempt_enable();
 }
 
@@ -600,7 +598,7 @@ static inline void fpu_copy(struct task_struct *dst, struct task_struct *src)
 		struct fpu *dfpu = &dst->thread.fpu;
 		struct fpu *sfpu = &src->thread.fpu;
 
-		unlazy_fpu(src);
+		save_init_fpu(src);
 		memcpy(dfpu->state, sfpu->state, xstate_size);
 	}
 }
diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index 6eb6fcb83f63..07836671acfb 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -101,8 +101,6 @@ static inline int user_has_fpu(void)
 	return current->thread.fpu.has_fpu;
 }
 
-extern void unlazy_fpu(struct task_struct *tsk);
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_I387_H */
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 47348653503a..3afc4e73b07f 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -116,18 +116,6 @@ void __kernel_fpu_end(void)
 }
 EXPORT_SYMBOL(__kernel_fpu_end);
 
-void unlazy_fpu(struct task_struct *tsk)
-{
-	preempt_disable();
-	if (__thread_has_fpu(tsk)) {
-		__save_init_fpu(tsk);
-		__thread_fpu_end(tsk);
-	} else
-		tsk->thread.fpu_counter = 0;
-	preempt_enable();
-}
-EXPORT_SYMBOL(unlazy_fpu);
-
 unsigned int mxcsr_feature_mask __read_mostly = 0xffffffffu;
 unsigned int xstate_size;
 EXPORT_SYMBOL_GPL(xstate_size);
@@ -245,8 +233,10 @@ int init_fpu(struct task_struct *tsk)
 	int ret;
 
 	if (tsk_used_math(tsk)) {
-		if (cpu_has_fpu && tsk == current)
-			unlazy_fpu(tsk);
+		if (cpu_has_fpu && tsk == current) {
+			save_init_fpu(tsk);
+			__thread_fpu_end(tsk);
+		}
 		tsk->thread.fpu.last_cpu = ~0;
 		return 0;
 	}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index fb4cb6adf225..201522fd2c96 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -664,6 +664,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	 * Save the info for the exception handler and clear the error.
 	 */
 	save_init_fpu(task);
+	__thread_fpu_end(task);
 	task->thread.trap_nr = trapnr;
 	task->thread.error_code = error_code;
 	info.si_signo = SIGFPE;

Re: [PATCH RFC] x86,fpu: merge save_init_fpu & unlazy_fpu

[Oleg Nesterov]

On 01/29, Rik van Riel wrote:
>
> The functions save_init_fpu and unlazy_fpu do essentially the
> same thing:

Yes ;) Could you look at 1-3 I sent ?

> Callers of init_fpu do want __thread_fpu_end, so move the call to
> __thread_fpu_end into init_fpu.

I don't think so... Contrary, I think this __thread_fpu_end() is simply
wrong.

>  static inline void save_init_fpu(struct task_struct *tsk)
>  {
> -	WARN_ON_ONCE(!__thread_has_fpu(tsk));
> -
> -	if (use_eager_fpu()) {
> -		__save_fpu(tsk);
> -		return;
> -	}
> -
>  	preempt_disable();
> -	__save_init_fpu(tsk);
> -	__thread_fpu_end(tsk);
> +	if (__thread_has_fpu(tsk)) {
> +		if (use_eager_fpu())
> +			__save_fpu(tsk);
> +		else
> +			__save_init_fpu(tsk);

See the changelog in 2/3. I think we still need __thread_fpu_end() if
__save_init_fpu() returns 0. In this case (_I think_) the state of FPU
doesn't match the saved state. IOW, "save_init" == "save" + "init" (I guess),
and that "init" can (say) reset some control register to default value.


> +	} else if (!use_eager_fpu())
> +		tsk->thread.fpu_counter = 0;

See 1/3, I think this should be simply removed.

> @@ -245,8 +233,10 @@ int init_fpu(struct task_struct *tsk)
>  	int ret;
>  
>  	if (tsk_used_math(tsk)) {
> -		if (cpu_has_fpu && tsk == current)
> -			unlazy_fpu(tsk);
> +		if (cpu_has_fpu && tsk == current) {
> +			save_init_fpu(tsk);
> +			__thread_fpu_end(tsk);

See above.

Oleg.

[PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Oleg Nesterov]

On 01/23, Rik van Riel wrote:
>
> Not only is this broken with my new code, but it looks like it may
> be broken with the current code, too...

Lets (try to) fix unlazy_fpu/save_init_fpu at least.

Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks
wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.

Oleg.

[PATCH 1/3] x86, fpu: unlazy_fpu: don't reset thread.fpu_counter

[Oleg Nesterov]

It is not clear why the "else" branch clears ->fpu_counter, this makes
no sense.

If use_eager_fpu() then this has no effect. Otherwise, if we actually
wanted to prevent fpu preload after the context switch we would need to
reset it unconditionally, even if __thread_has_fpu().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/i387.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 4734865..c3b92c0 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -122,8 +122,7 @@ void unlazy_fpu(struct task_struct *tsk)
 	if (__thread_has_fpu(tsk)) {
 		__save_init_fpu(tsk);
 		__thread_fpu_end(tsk);
-	} else
-		tsk->thread.fpu_counter = 0;
+	}
 	preempt_enable();
 }
 EXPORT_SYMBOL(unlazy_fpu);
-- 
1.5.5.1

Re: [PATCH 1/3] x86, fpu: unlazy_fpu: don't reset thread.fpu_counter

[Rik van Riel]

On 01/29/2015 04:07 PM, Oleg Nesterov wrote:
> It is not clear why the "else" branch clears ->fpu_counter, this makes
> no sense.
> 
> If use_eager_fpu() then this has no effect. Otherwise, if we actually
> wanted to prevent fpu preload after the context switch we would need to
> reset it unconditionally, even if __thread_has_fpu().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

[PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()

[Oleg Nesterov]

unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu().
Unconditional __thread_fpu_end() is only correct if we know that this
thread can't return to user-mode and use FPU.

Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(),
and init_fpu(current) can be only called by the coredumping thread via
regset->get(). But it is exported to modules, and imo this should be
fixed anyway.

And if we check use_eager_fpu() we can use __save_fpu() like fpu_copy()
and save_init_fpu() do.

- It seems that even !use_eager_fpu() case doesn't need the unconditional
  __thread_fpu_end(), we only need it if __save_init_fpu() returns 0.

- It is still not clear to me if __save_init_fpu() can safely nest with
  another save + restore from __kernel_fpu_begin(). If not, we can use
  kernel_fpu_disable() to fix the race.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/i387.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index c3b92c0..8e070a6 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -120,8 +120,12 @@ void unlazy_fpu(struct task_struct *tsk)
 {
 	preempt_disable();
 	if (__thread_has_fpu(tsk)) {
-		__save_init_fpu(tsk);
-		__thread_fpu_end(tsk);
+		if (use_eager_fpu()) {
+			__save_fpu(tsk);
+		} else {
+			__save_init_fpu(tsk);
+			__thread_fpu_end(tsk);
+		}
 	}
 	preempt_enable();
 }
-- 
1.5.5.1

Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()

[Rik van Riel]

On 01/29/2015 04:08 PM, Oleg Nesterov wrote:
> unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu().
> Unconditional __thread_fpu_end() is only correct if we know that this
> thread can't return to user-mode and use FPU.
> 
> Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(),
> and init_fpu(current) can be only called by the coredumping thread via
> regset->get(). But it is exported to modules, and imo this should be
> fixed anyway.

What about xfpregs_set?

It looks like that code copies an entire FPU state in
from userspace, and expects the kernel to start using
that new FPU state.

This is called from the ptrace code.

When we switch to the traced task, the __thread_fpu_end()
that was called from init_fpu() ensures that
switch_fpu_begin() will actually load the new FPU state
from memory into the registers, and we will not take
the fpu_lazy_restore() branch.

I believe that xstateregs_set and xfpregs_set still
need __thread_fpu_end().

What am I missing?

> And if we check use_eager_fpu() we can use __save_fpu() like fpu_copy()
> and save_init_fpu() do.
> 
> - It seems that even !use_eager_fpu() case doesn't need the unconditional
>   __thread_fpu_end(), we only need it if __save_init_fpu() returns 0.
> 
> - It is still not clear to me if __save_init_fpu() can safely nest with
>   another save + restore from __kernel_fpu_begin(). If not, we can use
>   kernel_fpu_disable() to fix the race.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/i387.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index c3b92c0..8e070a6 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -120,8 +120,12 @@ void unlazy_fpu(struct task_struct *tsk)
>  {
>  	preempt_disable();
>  	if (__thread_has_fpu(tsk)) {
> -		__save_init_fpu(tsk);
> -		__thread_fpu_end(tsk);
> +		if (use_eager_fpu()) {
> +			__save_fpu(tsk);
> +		} else {
> +			__save_init_fpu(tsk);
> +			__thread_fpu_end(tsk);
> +		}
>  	}
>  	preempt_enable();
>  }
> 

Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()

[Oleg Nesterov]

On 01/29, Rik van Riel wrote:
>
> On 01/29/2015 04:08 PM, Oleg Nesterov wrote:
> > unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu().
> > Unconditional __thread_fpu_end() is only correct if we know that this
> > thread can't return to user-mode and use FPU.
> >
> > Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(),
> > and init_fpu(current) can be only called by the coredumping thread via
> > regset->get(). But it is exported to modules, and imo this should be
> > fixed anyway.
>
> What about xfpregs_set?
>
> It looks like that code copies an entire FPU state in
> from userspace, and expects the kernel to start using
> that new FPU state.
>
> This is called from the ptrace code.

Yes. But in this case tsk != current, and we ensure that __switch_to()
was finished. wait_task_inactive().

> When we switch to the traced task, the __thread_fpu_end()
> that was called from init_fpu() ensures that
> switch_fpu_begin() will actually load the new FPU state
> from memory into the registers, and we will not take
> the fpu_lazy_restore() branch.

No. in this case we rely on "tsk->thread.fpu.last_cpu = ~0" which disables
fpu_lazy_restore().

> What am I missing?

Or me ;)

Oleg.

Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()

[Rik van Riel]

On 01/29/2015 04:49 PM, Oleg Nesterov wrote:

> Yes. But in this case tsk != current, and we ensure that __switch_to()
> was finished. wait_task_inactive().
> 
>> When we switch to the traced task, the __thread_fpu_end()
>> that was called from init_fpu() ensures that
>> switch_fpu_begin() will actually load the new FPU state
>> from memory into the registers, and we will not take
>> the fpu_lazy_restore() branch.
> 
> No. in this case we rely on "tsk->thread.fpu.last_cpu = ~0" which disables
> fpu_lazy_restore().

I should have known that.

Especially considering I have a patch here that
converts "tsk->thread.fpu.last_cpu = ~0" into
a disable_lazy_restore() call...

Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()

[Rik van Riel]

On 01/29/2015 04:08 PM, Oleg Nesterov wrote:
> unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu().
> Unconditional __thread_fpu_end() is only correct if we know that this
> thread can't return to user-mode and use FPU.
> 
> Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(),
> and init_fpu(current) can be only called by the coredumping thread via
> regset->get(). But it is exported to modules, and imo this should be
> fixed anyway.
> 
> And if we check use_eager_fpu() we can use __save_fpu() like fpu_copy()
> and save_init_fpu() do.
> 
> - It seems that even !use_eager_fpu() case doesn't need the unconditional
>   __thread_fpu_end(), we only need it if __save_init_fpu() returns 0.
> 
> - It is still not clear to me if __save_init_fpu() can safely nest with
>   another save + restore from __kernel_fpu_begin(). If not, we can use
>   kernel_fpu_disable() to fix the race.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

[PATCH 3/3] x86, fpu: kill save_init_fpu(), change math_error() to use unlazy_fpu()

[Oleg Nesterov]

math_error() calls save_init_fpu() after conditional_sti(), this means
that the caller can be preempted. If !use_eager_fpu() we can hit the
WARN_ON_ONCE(!__thread_has_fpu(tsk)) and/or save the wrong FPU state.

Change math_error() to use unlazy_fpu() and kill save_init_fpu().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h |   18 ------------------
 arch/x86/kernel/traps.c             |    2 +-
 2 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index e97622f..02f2e08 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -518,24 +518,6 @@ static inline void __save_fpu(struct task_struct *tsk)
 }
 
 /*
- * These disable preemption on their own and are safe
- */
-static inline void save_init_fpu(struct task_struct *tsk)
-{
-	WARN_ON_ONCE(!__thread_has_fpu(tsk));
-
-	if (use_eager_fpu()) {
-		__save_fpu(tsk);
-		return;
-	}
-
-	preempt_disable();
-	__save_init_fpu(tsk);
-	__thread_fpu_end(tsk);
-	preempt_enable();
-}
-
-/*
  * i387 state interaction
  */
 static inline unsigned short get_fpu_cwd(struct task_struct *tsk)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index fb4cb6a..51c4658 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -663,7 +663,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	/*
 	 * Save the info for the exception handler and clear the error.
 	 */
-	save_init_fpu(task);
+	unlazy_fpu(task);
 	task->thread.trap_nr = trapnr;
 	task->thread.error_code = error_code;
 	info.si_signo = SIGFPE;
-- 
1.5.5.1

Re: [PATCH 3/3] x86, fpu: kill save_init_fpu(), change math_error() to use unlazy_fpu()

[Rik van Riel]

On 01/29/2015 04:08 PM, Oleg Nesterov wrote:
> math_error() calls save_init_fpu() after conditional_sti(), this means
> that the caller can be preempted. If !use_eager_fpu() we can hit the
> WARN_ON_ONCE(!__thread_has_fpu(tsk)) and/or save the wrong FPU state.
> 
> Change math_error() to use unlazy_fpu() and kill save_init_fpu().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Dave Hansen]

On 01/29/2015 01:07 PM, Oleg Nesterov wrote:
> On 01/23, Rik van Riel wrote:
>> > Not only is this broken with my new code, but it looks like it may
>> > be broken with the current code, too...
> Lets (try to) fix unlazy_fpu/save_init_fpu at least.
> 
> Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks
> wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.

IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will
enable eagerfpu.  It's a bit of an indirect way to avoid the WARN_ON()
and probably needs a better comment.

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Oleg Nesterov]

On 01/29, Dave Hansen wrote:
>
> On 01/29/2015 01:07 PM, Oleg Nesterov wrote:
> > On 01/23, Rik van Riel wrote:
> >> > Not only is this broken with my new code, but it looks like it may
> >> > be broken with the current code, too...
> > Lets (try to) fix unlazy_fpu/save_init_fpu at least.
> >
> > Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks
> > wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.
>
> IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will
> enable eagerfpu.

unless eagerfpu=off? but this doesn't matter.

> It's a bit of an indirect way to avoid the WARN_ON()
> and probably needs a better comment.

I don't think it is safe to assume that current has FPU even if eagerfpu.
And note that Rik is working on patches which can change this rule.

Or I misunderstood?

Oleg.

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Dave Hansen]

On 01/29/2015 01:33 PM, Oleg Nesterov wrote:
> On 01/29, Dave Hansen wrote:
>>
>> On 01/29/2015 01:07 PM, Oleg Nesterov wrote:
>>> On 01/23, Rik van Riel wrote:
>>>>> Not only is this broken with my new code, but it looks like it may
>>>>> be broken with the current code, too...
>>> Lets (try to) fix unlazy_fpu/save_init_fpu at least.
>>>
>>> Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks
>>> wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.
>>
>> IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will
>> enable eagerfpu.
> 
> unless eagerfpu=off? but this doesn't matter.

Yeah, that's true.  That would also explain why I haven't run in to this
at all in testing.

Ugh, fpu_save_init() says it isn't preempt safe anyway, so we shouldn't
be using it.

I'll send a fix.

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Oleg Nesterov]

On 01/29, Dave Hansen wrote:
>
> On 01/29/2015 01:33 PM, Oleg Nesterov wrote:
> > On 01/29, Dave Hansen wrote:
> >>
> >> On 01/29/2015 01:07 PM, Oleg Nesterov wrote:
> >>> On 01/23, Rik van Riel wrote:
> >>>>> Not only is this broken with my new code, but it looks like it may
> >>>>> be broken with the current code, too...
> >>> Lets (try to) fix unlazy_fpu/save_init_fpu at least.
> >>>
> >>> Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks
> >>> wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.
> >>
> >> IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will
> >> enable eagerfpu.
> >
> > unless eagerfpu=off? but this doesn't matter.
>
> Yeah, that's true.  That would also explain why I haven't run in to this
> at all in testing.
>
> Ugh, fpu_save_init() says it isn't preempt safe anyway, so we shouldn't
> be using it.

Yes, plus (I _think_) _init can add more problems.

> I'll send a fix.

How about the trivial patch below (on top of this series) ?

Oleg.

--- x/arch/x86/kernel/traps.c
+++ x/arch/x86/kernel/traps.c
@@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	xsave_buf = &(tsk->thread.fpu.state->xsave);
 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
 	if (!bndcsr)
--- x/arch/x86/mm/mpx.c
+++ x/arch/x86/mm/mpx.c
@@ -352,7 +352,7 @@ static __user void *task_get_bounds_dir(
 	 * The bounds directory pointer is stored in a register
 	 * only accessible if we first do an xsave.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	bndcsr = get_xsave_addr(&tsk->thread.fpu.state->xsave, XSTATE_BNDCSR);
 	if (!bndcsr)
 		return MPX_INVALID_BOUNDS_DIR;

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Rik van Riel]

On 01/29/2015 04:56 PM, Oleg Nesterov wrote:

> How about the trivial patch below (on top of this series) ?

Hey, I just typed that, too! :)

Reviewed-by: Rik van Riel <riel@redhat.com>

> --- x/arch/x86/kernel/traps.c
> +++ x/arch/x86/kernel/traps.c
> @@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
>  	 * It is not directly accessible, though, so we need to
>  	 * do an xsave and then pull it out of the xsave buffer.
>  	 */
> -	fpu_save_init(&tsk->thread.fpu);
> +	unlazy_fpu(tsk);
>  	xsave_buf = &(tsk->thread.fpu.state->xsave);
>  	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
>  	if (!bndcsr)
> --- x/arch/x86/mm/mpx.c
> +++ x/arch/x86/mm/mpx.c
> @@ -352,7 +352,7 @@ static __user void *task_get_bounds_dir(
>  	 * The bounds directory pointer is stored in a register
>  	 * only accessible if we first do an xsave.
>  	 */
> -	fpu_save_init(&tsk->thread.fpu);
> +	unlazy_fpu(tsk);
>  	bndcsr = get_xsave_addr(&tsk->thread.fpu.state->xsave, XSTATE_BNDCSR);
>  	if (!bndcsr)
>  		return MPX_INVALID_BOUNDS_DIR;
> 

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Dave Hansen]

On 01/29/2015 01:56 PM, Oleg Nesterov wrote:
> --- x/arch/x86/kernel/traps.c
> +++ x/arch/x86/kernel/traps.c
> @@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
>  	 * It is not directly accessible, though, so we need to
>  	 * do an xsave and then pull it out of the xsave buffer.
>  	 */
> -	fpu_save_init(&tsk->thread.fpu);
> +	unlazy_fpu(tsk);
>  	xsave_buf = &(tsk->thread.fpu.state->xsave);
...
> 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

Hmm, if the the thread was not using the FPU, and this fails to save
anything in to the xsave_buf, what will bndcsr point to?  It _looks_ to
me like it will just point to uninitialized data since the xsave never
happened.

Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against
the xsave->xstate_bv?

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/29/2015 06:26 PM, Dave Hansen wrote:
> On 01/29/2015 01:56 PM, Oleg Nesterov wrote:
>> --- x/arch/x86/kernel/traps.c +++ x/arch/x86/kernel/traps.c @@
>> -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r * It is
>> not directly accessible, though, so we need to * do an xsave and
>> then pull it out of the xsave buffer. */ -
>> fpu_save_init(&tsk->thread.fpu); +	unlazy_fpu(tsk); xsave_buf =
>> &(tsk->thread.fpu.state->xsave);
> ...
>> bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
> 
> Hmm, if the the thread was not using the FPU, and this fails to
> save anything in to the xsave_buf, what will bndcsr point to?

If the thread was not using the FPU, can we reach the
bound range exception?

I believe the MPX feature uses information in the xstate.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUyt9yAAoJEM553pKExN6D6U4H+QENaWW78N/A9MfdluF9j1c4
mLVjsna+PrdglRNMgZPIozD/V+aONiVbEvUYt2bGuBP7PaHdvasm+05/U+SJF37z
SxdjH0+1U+IZPycf0eGRkrFpZvUuegNzJyQFcvPltjRNHc7faDm3nJv+xdjDd9DX
NxcvexnjhkLpXvbEIOksv/41EMjyYtDBEWwenCANCLQaGyk4VJUxXqiZ0ivqtNJX
WuzVydvtDebWKPHF61qqtDCGkuuJvypWR6Wbgqe1McKqNgElT4c3f5QDLG45fKrw
YaiFzBGpeFEJtDe7XLCmdlp6xUGLL2SxFn5PVdyB1toHd+V6eN//QlwAOhDJPeA=
=HPaU
-----END PGP SIGNATURE-----

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Dave Hansen]

On 01/29/2015 05:33 PM, Rik van Riel wrote:
>> > Hmm, if the the thread was not using the FPU, and this fails to
>> > save anything in to the xsave_buf, what will bndcsr point to?
> If the thread was not using the FPU, can we reach the
> bound range exception?
> 
> I believe the MPX feature uses information in the xstate.

There are really two kinds of bounds exceptions.  There is an "old"
'BOUND' instruction that will generate #BR's as well as the new MPX
instructions.  The new ones require xstate and poking the FPU, etc...
The old one did not.

So, we _can_ as far as I know get in to do_bounds() without using the FPU.

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Oleg Nesterov]

On 01/29, Dave Hansen wrote:
>
> On 01/29/2015 01:56 PM, Oleg Nesterov wrote:
> > --- x/arch/x86/kernel/traps.c
> > +++ x/arch/x86/kernel/traps.c
> > @@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
> >  	 * It is not directly accessible, though, so we need to
> >  	 * do an xsave and then pull it out of the xsave buffer.
> >  	 */
> > -	fpu_save_init(&tsk->thread.fpu);
> > +	unlazy_fpu(tsk);
> >  	xsave_buf = &(tsk->thread.fpu.state->xsave);
> ...
> > 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
>
> Hmm, if the the thread was not using the FPU, and this fails to save
> anything in to the xsave_buf, what will bndcsr point to?  It _looks_ to
> me like it will just point to uninitialized data since the xsave never
> happened.
>
> Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against
> the xsave->xstate_bv?

Can't really comment, but let me clarify what I meant just in case...

If it was not using FPU then I guess do_bounds() can't happen. However,
it can be preempted after conditional_sti(). fpu_save_init() is obviously
wrong unless __thread_has_fpu() == T, and this can be false if !eagerfpu
or if we add TIF_LOAD_FPU (defer FPU restore until return to userspace).

Oleg.

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Oleg Nesterov]

On 01/30, Oleg Nesterov wrote:
>
> On 01/29, Dave Hansen wrote:
> >
> > On 01/29/2015 01:56 PM, Oleg Nesterov wrote:
> > > --- x/arch/x86/kernel/traps.c
> > > +++ x/arch/x86/kernel/traps.c
> > > @@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
> > >  	 * It is not directly accessible, though, so we need to
> > >  	 * do an xsave and then pull it out of the xsave buffer.
> > >  	 */
> > > -	fpu_save_init(&tsk->thread.fpu);
> > > +	unlazy_fpu(tsk);
> > >  	xsave_buf = &(tsk->thread.fpu.state->xsave);
> > ...
> > > 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
> >
> > Hmm, if the the thread was not using the FPU, and this fails to save
> > anything in to the xsave_buf, what will bndcsr point to?  It _looks_ to
> > me like it will just point to uninitialized data since the xsave never
> > happened.
> >
> > Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against
> > the xsave->xstate_bv?
>
> Can't really comment, but let me clarify what I meant just in case...
>
> If it was not using FPU then I guess do_bounds() can't happen. However,
> it can be preempted after conditional_sti(). fpu_save_init() is obviously
> wrong unless __thread_has_fpu() == T, and this can be false if !eagerfpu
> or if we add TIF_LOAD_FPU (defer FPU restore until return to userspace).

Forgot to mention... and if we use unlazy_fpu() we should not worry about
preemption/__thread_has_fpu, we can rely on __save_init_fpu() in __switch_to()
and/or __thread_has_fpu() check in unlazy_fpu().

But I am afraid I misunderstood your concerns, sorry for noise in this case.

Oleg.

Re: [PATCH 0/3]: x86, fpu: unlazy_fpu fixes/cleanups

[Oleg Nesterov]

On 01/30, Oleg Nesterov wrote:
>
> On 01/30, Oleg Nesterov wrote:
> >
> > On 01/29, Dave Hansen wrote:
> > >
> > > On 01/29/2015 01:56 PM, Oleg Nesterov wrote:
> > > > --- x/arch/x86/kernel/traps.c
> > > > +++ x/arch/x86/kernel/traps.c
> > > > @@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
> > > >  	 * It is not directly accessible, though, so we need to
> > > >  	 * do an xsave and then pull it out of the xsave buffer.
> > > >  	 */
> > > > -	fpu_save_init(&tsk->thread.fpu);
> > > > +	unlazy_fpu(tsk);
> > > >  	xsave_buf = &(tsk->thread.fpu.state->xsave);
> > > ...
> > > > 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
> > >
> > > Hmm, if the the thread was not using the FPU, and this fails to save
> > > anything in to the xsave_buf, what will bndcsr point to?  It _looks_ to
> > > me like it will just point to uninitialized data since the xsave never
> > > happened.
> > >
> > > Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against
> > > the xsave->xstate_bv?
> >
> > Can't really comment, but let me clarify what I meant just in case...
> >
> > If it was not using FPU then I guess do_bounds() can't happen. However,
> > it can be preempted after conditional_sti(). fpu_save_init() is obviously
> > wrong unless __thread_has_fpu() == T, and this can be false if !eagerfpu
> > or if we add TIF_LOAD_FPU (defer FPU restore until return to userspace).
>
> Forgot to mention... and if we use unlazy_fpu() we should not worry about
> preemption/__thread_has_fpu, we can rely on __save_init_fpu() in __switch_to()
> and/or __thread_has_fpu() check in unlazy_fpu().
>
> But I am afraid I misunderstood your concerns, sorry for noise in this case.

Yes ;)

Perhaps do_bounds() needs the additional

	if (!used_math())
		goto exit_trap;

check?

Oleg.

[PATCH 0/3] cleanups to the disable lazy fpu restore code

[riel]

These go on top of Oleg's patches from yesterday.

The mechanism to disable lazy FPU restore is inscrutible
in several places, and dubious at best in one.

These patches make things explicit.

[PATCH 1/3] x86,fpu: move lazy restore functions up a few lines

[riel]

From: Rik van Riel <riel@redhat.com>

We need another lazy restore related function, that will be called
from a function that is above where the lazy restore functions are
now. It would be nice to keep all three functions grouped together.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 27d00e04f911..439ac3921a1e 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -67,6 +67,24 @@ extern void finit_soft_fpu(struct i387_soft_struct *soft);
 static inline void finit_soft_fpu(struct i387_soft_struct *soft) {}
 #endif
 
+/*
+ * Must be run with preemption disabled: this clears the fpu_owner_task,
+ * on this CPU.
+ *
+ * This will disable any lazy FPU state restore of the current FPU state,
+ * but if the current thread owns the FPU, it will still be saved by.
+ */
+static inline void __cpu_disable_lazy_restore(unsigned int cpu)
+{
+	per_cpu(fpu_owner_task, cpu) = NULL;
+}
+
+static inline int fpu_lazy_restore(struct task_struct *new, unsigned int cpu)
+{
+	return new == this_cpu_read_stable(fpu_owner_task) &&
+		cpu == new->thread.fpu.last_cpu;
+}
+
 static inline int is_ia32_compat_frame(void)
 {
 	return config_enabled(CONFIG_IA32_EMULATION) &&
@@ -400,24 +418,6 @@ static inline void drop_init_fpu(struct task_struct *tsk)
  */
 typedef struct { int preload; } fpu_switch_t;
 
-/*
- * Must be run with preemption disabled: this clears the fpu_owner_task,
- * on this CPU.
- *
- * This will disable any lazy FPU state restore of the current FPU state,
- * but if the current thread owns the FPU, it will still be saved by.
- */
-static inline void __cpu_disable_lazy_restore(unsigned int cpu)
-{
-	per_cpu(fpu_owner_task, cpu) = NULL;
-}
-
-static inline int fpu_lazy_restore(struct task_struct *new, unsigned int cpu)
-{
-	return new == this_cpu_read_stable(fpu_owner_task) &&
-		cpu == new->thread.fpu.last_cpu;
-}
-
 static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct task_struct *new, int cpu)
 {
 	fpu_switch_t fpu;
-- 
1.9.3

[PATCH 2/3] x86,fpu: introduce task_disable_lazy_fpu_restore helper

[riel]

From: Rik van Riel <riel@redhat.com>

Currently there are a few magic assignments sprinkled through the
code that disable lazy FPU state restoring, some more effective than
others, and all equally mystifying.

It would be easier to have a helper to explicitly disable lazy
FPU state restoring for a task.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 439ac3921a1e..c1f66261ad12 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -79,6 +79,16 @@ static inline void __cpu_disable_lazy_restore(unsigned int cpu)
 	per_cpu(fpu_owner_task, cpu) = NULL;
 }
 
+/*
+ * Used to indicate that the FPU state in memory is newer than the FPU
+ * state in registers, and the FPU state should be reloaded next time the
+ * task is run. Only safe on the current task, or non-running tasks.
+ */
+static inline void task_disable_lazy_fpu_restore(struct task_struct *tsk)
+{
+	tsk->thread.fpu.last_cpu = ~0;
+}
+
 static inline int fpu_lazy_restore(struct task_struct *new, unsigned int cpu)
 {
 	return new == this_cpu_read_stable(fpu_owner_task) &&
-- 
1.9.3

[PATCH 3/3] x86,fpu: use disable_task_lazy_fpu_restore helper

[riel]

From: Rik van Riel <riel@redhat.com>

Replace magic assignments of fpu.last_cpu = ~0 with more explicit
disable_task_lazy_fpu_restore calls.

This also fixes the lazy FPU restore disabling in drop_fpu, which
only really works when !use_eager_fpu().  This is fine for now,
because fpu_lazy_restore() is only used when !use_eager_fpu()
currently, but we may want to expand that.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 9 +++++----
 arch/x86/kernel/i387.c              | 2 +-
 arch/x86/kernel/process.c           | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index c1f66261ad12..e2832f9dfed5 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk)
 	 * Forget coprocessor state..
 	 */
 	preempt_disable();
-	tsk->thread.fpu_counter = 0;
+	task_disable_lazy_fpu_restore(tsk);
 	__drop_fpu(tsk);
 	clear_used_math();
 	preempt_enable();
@@ -440,8 +440,9 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 					     new->thread.fpu_counter > 5);
 	if (__thread_has_fpu(old)) {
 		if (!__save_init_fpu(old))
-			cpu = ~0;
-		old->thread.fpu.last_cpu = cpu;
+			task_disable_lazy_fpu_restore(old);
+		else
+			old->thread.fpu.last_cpu = cpu;
 		old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task! */
 
 		/* Don't change CR0.TS if we just switch! */
@@ -453,7 +454,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 			stts();
 	} else {
 		old->thread.fpu_counter = 0;
-		old->thread.fpu.last_cpu = ~0;
+		task_disable_lazy_fpu_restore(old);
 		if (fpu.preload) {
 			new->thread.fpu_counter++;
 			if (!use_eager_fpu() && fpu_lazy_restore(new, cpu))
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 8e070a6c30e5..8416b5f85806 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -250,7 +250,7 @@ int init_fpu(struct task_struct *tsk)
 	if (tsk_used_math(tsk)) {
 		if (cpu_has_fpu && tsk == current)
 			unlazy_fpu(tsk);
-		tsk->thread.fpu.last_cpu = ~0;
+		task_disable_lazy_fpu_restore(tsk);
 		return 0;
 	}
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index dd9a069a5ec5..83480373a642 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -68,8 +68,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 
 	dst->thread.fpu_counter = 0;
 	dst->thread.fpu.has_fpu = 0;
-	dst->thread.fpu.last_cpu = ~0;
 	dst->thread.fpu.state = NULL;
+	task_disable_lazy_fpu_restore(dst);
 	if (tsk_used_math(src)) {
 		int err = fpu_alloc(&dst->thread.fpu);
 		if (err)
-- 
1.9.3

Re: [PATCH 3/3] x86,fpu: use disable_task_lazy_fpu_restore helper

[Dave Hansen]

On 01/30/2015 09:49 AM, riel@redhat.com wrote:
> @@ -440,8 +440,9 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
>  					     new->thread.fpu_counter > 5);
>  	if (__thread_has_fpu(old)) {
>  		if (!__save_init_fpu(old))
> -			cpu = ~0;
> -		old->thread.fpu.last_cpu = cpu;
> +			task_disable_lazy_fpu_restore(old);
> +		else
> +			old->thread.fpu.last_cpu = cpu;

What is the 'else' doing here?  It seems a bit disconnected from the
other parts of the patch.

Re: [PATCH 3/3] x86,fpu: use disable_task_lazy_fpu_restore helper

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/30/2015 04:46 PM, Dave Hansen wrote:
> On 01/30/2015 09:49 AM, riel@redhat.com wrote:
>> @@ -440,8 +440,9 @@ static inline fpu_switch_t
>> switch_fpu_prepare(struct task_struct *old, struct ta 
>> new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if
>> (!__save_init_fpu(old)) -			cpu = ~0; -		old->thread.fpu.last_cpu
>> = cpu; +			task_disable_lazy_fpu_restore(old); +		else +
>> old->thread.fpu.last_cpu = cpu;
> 
> What is the 'else' doing here?  It seems a bit disconnected from
> the other parts of the patch.

The assignment was already there before, but we would set
cpu to ~0 if __save_init_fpu failed.

Now there are different branches for failure and success,
with the success branch having the same assignment that
was there before.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUy/w1AAoJEM553pKExN6D/fsH/RjpQE67P368cvHl6QKa0RUo
TDF0iEl4XqLT9wlYPsr+NgVJG0rFK36iVrGtKVV/uziNmk+1H43WZUt/u8tsTC1k
gYptwOeBpGVyktmwdperBejTm3q4k9WLYASOvU90S2fMO5IavCpafkM5CJwojFie
0fquULSkgmyxXIJrNTNYznA2YYZaiHcKY8i05H9W0iLYbYQseQhDKl7sZ5uWcfd2
ziWmWpMKSgD0AaC1PbMm8k4kKHkbaKlabR9AyOd+9Nk5voU/pPcJOm4QgQru2ZhR
2wRdVA+MMwoglsPygV3I2OPqJN95o0JAgSTJk5DXfPuiMYoAe2+KWy5pa0ERvMo=
=Fmf0
-----END PGP SIGNATURE-----

Re: [PATCH 3/3] x86,fpu: use disable_task_lazy_fpu_restore helper

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/30/2015 04:46 PM, Dave Hansen wrote:
> On 01/30/2015 09:49 AM, riel@redhat.com wrote:
>> @@ -440,8 +440,9 @@ static inline fpu_switch_t
>> switch_fpu_prepare(struct task_struct *old, struct ta 
>> new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if
>> (!__save_init_fpu(old)) -			cpu = ~0; -		old->thread.fpu.last_cpu
>> = cpu; +			task_disable_lazy_fpu_restore(old); +		else +
>> old->thread.fpu.last_cpu = cpu;
> 
> What is the 'else' doing here?  It seems a bit disconnected from
> the other parts of the patch.

Let me break that out into a separate patch, for reviewing
clarity.

INCOMING!

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUz7o7AAoJEM553pKExN6DekMIAIuAFk437HUVDviJX9tx8his
2Jec8932I2+nd8OBzqBdCjiSUimLCNdT7eygf2UUdMciaNuHQ1CU1GAkM2MOglvo
/ZYAGoZPbqdJZOQZp0HIzpqBGaqTjNoNfY46EVZyQgc/e3LVAfMZEF/SHhjLESEt
gaJ0/ucRxl8a5C4qd29Pdgbn6jaionmEA1xrdYqfvQM32JVt4B7Qu/v2ZVCWDsCt
W+TIEWFJga7u6vtKkfC4eHB0PPliMTn0N5bzCSk4zGZRtj+T8rVJ1eFcxhmEsWFs
N6EF5CwGMVU0QvsWHK6Js2ELHAo+I1KJ7e/m0dHeBj/P4To835tnMh3a+IK0XWo=
=rIwr
-----END PGP SIGNATURE-----

[PATCH 0/6] cleanups to lazy FPU restore code

[riel]

These go on top of Oleg's patches.

The mechanism to disable lazy FPU restore is inscrutible
in several places, and dubious at best in one.

These patches make things explicit.

They also get rid of some apparently useless or redundant
code, and make fpu_lazy_restore work when in eager FPU mode.

[PATCH 1/6] x86,fpu: move lazy restore functions up a few lines

[riel]

From: Rik van Riel <riel@redhat.com>

We need another lazy restore related function, that will be called
from a function that is above where the lazy restore functions are
now. It would be nice to keep all three functions grouped together.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 27d00e04f911..439ac3921a1e 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -67,6 +67,24 @@ extern void finit_soft_fpu(struct i387_soft_struct *soft);
 static inline void finit_soft_fpu(struct i387_soft_struct *soft) {}
 #endif
 
+/*
+ * Must be run with preemption disabled: this clears the fpu_owner_task,
+ * on this CPU.
+ *
+ * This will disable any lazy FPU state restore of the current FPU state,
+ * but if the current thread owns the FPU, it will still be saved by.
+ */
+static inline void __cpu_disable_lazy_restore(unsigned int cpu)
+{
+	per_cpu(fpu_owner_task, cpu) = NULL;
+}
+
+static inline int fpu_lazy_restore(struct task_struct *new, unsigned int cpu)
+{
+	return new == this_cpu_read_stable(fpu_owner_task) &&
+		cpu == new->thread.fpu.last_cpu;
+}
+
 static inline int is_ia32_compat_frame(void)
 {
 	return config_enabled(CONFIG_IA32_EMULATION) &&
@@ -400,24 +418,6 @@ static inline void drop_init_fpu(struct task_struct *tsk)
  */
 typedef struct { int preload; } fpu_switch_t;
 
-/*
- * Must be run with preemption disabled: this clears the fpu_owner_task,
- * on this CPU.
- *
- * This will disable any lazy FPU state restore of the current FPU state,
- * but if the current thread owns the FPU, it will still be saved by.
- */
-static inline void __cpu_disable_lazy_restore(unsigned int cpu)
-{
-	per_cpu(fpu_owner_task, cpu) = NULL;
-}
-
-static inline int fpu_lazy_restore(struct task_struct *new, unsigned int cpu)
-{
-	return new == this_cpu_read_stable(fpu_owner_task) &&
-		cpu == new->thread.fpu.last_cpu;
-}
-
 static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct task_struct *new, int cpu)
 {
 	fpu_switch_t fpu;
-- 
1.9.3

[PATCH 2/6] x86,fpu: introduce task_disable_lazy_fpu_restore helper

[riel]

From: Rik van Riel <riel@redhat.com>

Currently there are a few magic assignments sprinkled through the
code that disable lazy FPU state restoring, some more effective than
others, and all equally mystifying.

It would be easier to have a helper to explicitly disable lazy
FPU state restoring for a task.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 439ac3921a1e..c1f66261ad12 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -79,6 +79,16 @@ static inline void __cpu_disable_lazy_restore(unsigned int cpu)
 	per_cpu(fpu_owner_task, cpu) = NULL;
 }
 
+/*
+ * Used to indicate that the FPU state in memory is newer than the FPU
+ * state in registers, and the FPU state should be reloaded next time the
+ * task is run. Only safe on the current task, or non-running tasks.
+ */
+static inline void task_disable_lazy_fpu_restore(struct task_struct *tsk)
+{
+	tsk->thread.fpu.last_cpu = ~0;
+}
+
 static inline int fpu_lazy_restore(struct task_struct *new, unsigned int cpu)
 {
 	return new == this_cpu_read_stable(fpu_owner_task) &&
-- 
1.9.3

[PATCH 3/6] x86,fpu: use an explicit if/else in switch_fpu_prepare

[riel]

From: Rik van Riel <riel@redhat.com>

Use an explicit if/else branch after __save_init_fpu(old) in
switch_fpu_prepare.  This makes substituting the assignment
with a call to task_disable_lazy_fpu() in the next patch easier
to review.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index c1f66261ad12..04063751ac80 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -440,8 +440,9 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 					     new->thread.fpu_counter > 5);
 	if (__thread_has_fpu(old)) {
 		if (!__save_init_fpu(old))
-			cpu = ~0;
-		old->thread.fpu.last_cpu = cpu;
+			old->thread.fpu.last_cpu = ~0;
+		else
+			old->thread.fpu.last_cpu = cpu;
 		old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task! */
 
 		/* Don't change CR0.TS if we just switch! */
-- 
1.9.3

[PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

[riel]

From: Rik van Riel <riel@redhat.com>

Replace magic assignments of fpu.last_cpu = ~0 with more explicit
disable_task_lazy_fpu_restore calls.

This also fixes the lazy FPU restore disabling in drop_fpu, which
only really works when !use_eager_fpu().  This is fine for now,
because fpu_lazy_restore() is only used when !use_eager_fpu()
currently, but we may want to expand that.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 6 +++---
 arch/x86/kernel/i387.c              | 2 +-
 arch/x86/kernel/process.c           | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 04063751ac80..e2832f9dfed5 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk)
 	 * Forget coprocessor state..
 	 */
 	preempt_disable();
-	tsk->thread.fpu_counter = 0;
+	task_disable_lazy_fpu_restore(tsk);
 	__drop_fpu(tsk);
 	clear_used_math();
 	preempt_enable();
@@ -440,7 +440,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 					     new->thread.fpu_counter > 5);
 	if (__thread_has_fpu(old)) {
 		if (!__save_init_fpu(old))
-			old->thread.fpu.last_cpu = ~0;
+			task_disable_lazy_fpu_restore(old);
 		else
 			old->thread.fpu.last_cpu = cpu;
 		old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task! */
@@ -454,7 +454,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 			stts();
 	} else {
 		old->thread.fpu_counter = 0;
-		old->thread.fpu.last_cpu = ~0;
+		task_disable_lazy_fpu_restore(old);
 		if (fpu.preload) {
 			new->thread.fpu_counter++;
 			if (!use_eager_fpu() && fpu_lazy_restore(new, cpu))
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 8e070a6c30e5..8416b5f85806 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -250,7 +250,7 @@ int init_fpu(struct task_struct *tsk)
 	if (tsk_used_math(tsk)) {
 		if (cpu_has_fpu && tsk == current)
 			unlazy_fpu(tsk);
-		tsk->thread.fpu.last_cpu = ~0;
+		task_disable_lazy_fpu_restore(tsk);
 		return 0;
 	}
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index dd9a069a5ec5..83480373a642 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -68,8 +68,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 
 	dst->thread.fpu_counter = 0;
 	dst->thread.fpu.has_fpu = 0;
-	dst->thread.fpu.last_cpu = ~0;
 	dst->thread.fpu.state = NULL;
+	task_disable_lazy_fpu_restore(dst);
 	if (tsk_used_math(src)) {
 		int err = fpu_alloc(&dst->thread.fpu);
 		if (err)
-- 
1.9.3

Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

[Oleg Nesterov]

I'll try to read this patch tomorrow. Too late for me.

I think it is fine, but

On 02/02, riel@redhat.com wrote:
>
> This also fixes the lazy FPU restore disabling in drop_fpu, which
> only really works when !use_eager_fpu().
> ...
>
> --- a/arch/x86/include/asm/fpu-internal.h
> +++ b/arch/x86/include/asm/fpu-internal.h
> @@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk)
>  	 * Forget coprocessor state..
>  	 */
>  	preempt_disable();
> -	tsk->thread.fpu_counter = 0;
> +	task_disable_lazy_fpu_restore(tsk);
>  	__drop_fpu(tsk);
>  	clear_used_math();

perhaps this makes sense anyway, but I am not sure if the changelog is right.

Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect after this.

>  	preempt_enable();
> @@ -440,7 +440,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
>  					     new->thread.fpu_counter > 5);
>  	if (__thread_has_fpu(old)) {
>  		if (!__save_init_fpu(old))
> -			old->thread.fpu.last_cpu = ~0;
> +			task_disable_lazy_fpu_restore(old);
>  		else
>  			old->thread.fpu.last_cpu = cpu;
>  		old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task! */
> @@ -454,7 +454,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
>  			stts();
>  	} else {
>  		old->thread.fpu_counter = 0;
> -		old->thread.fpu.last_cpu = ~0;
> +		task_disable_lazy_fpu_restore(old);

I am also wondering if we can remove this task_disable_lazy_fpu_restore...
I mean, perhaps we should shift this into __thread_fpu_end() path. But this
is almost off-topic and in any case this patch should not do this.

Oleg.

Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/02/2015 02:21 PM, Oleg Nesterov wrote:
> I'll try to read this patch tomorrow. Too late for me.
> 
> I think it is fine, but
> 
> On 02/02, riel@redhat.com wrote:
>> 
>> This also fixes the lazy FPU restore disabling in drop_fpu,
>> which only really works when !use_eager_fpu(). ...
>> 
>> --- a/arch/x86/include/asm/fpu-internal.h +++
>> b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static
>> inline void drop_fpu(struct task_struct *tsk) * Forget
>> coprocessor state.. */ preempt_disable(); -
>> tsk->thread.fpu_counter = 0; +
>> task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); 
>> clear_used_math();
> 
> perhaps this makes sense anyway, but I am not sure if the changelog
> is right.
> 
> Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect
> after this.

There are several code paths, including signal handler stack setup and
teardown, where we first clear PF_USED_MATH, but later on set it again,
after setting up a new math state for the task.

We need to ensure we always use that new math state, and never lazily
re-use what is still in the FPU registers.

>> preempt_enable(); @@ -440,7 +440,7 @@ static inline fpu_switch_t
>> switch_fpu_prepare(struct task_struct *old, struct ta 
>> new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if
>> (!__save_init_fpu(old)) -			old->thread.fpu.last_cpu = ~0; +
>> task_disable_lazy_fpu_restore(old); else old->thread.fpu.last_cpu
>> = cpu; old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task!
>> */ @@ -454,7 +454,7 @@ static inline fpu_switch_t
>> switch_fpu_prepare(struct task_struct *old, struct ta stts(); }
>> else { old->thread.fpu_counter = 0; -		old->thread.fpu.last_cpu =
>> ~0; +		task_disable_lazy_fpu_restore(old);
> 
> I am also wondering if we can remove this
> task_disable_lazy_fpu_restore... I mean, perhaps we should shift
> this into __thread_fpu_end() path. But this is almost off-topic and
> in any case this patch should not do this.

Good question. I also wonder why this last_cpu = ~0 is there
anyway. If !__thread_has_fpu(old), then I don't see how the
lazy restore code would ever give a false positive on that CPU.

Either nobody else used the FPU and the task's state is still
there, or somebody else did, and the fpu_owner_task will be
pointing elsewhere.

It may be possible to simply remove this one completely, but
like you said, that should probably be a different patch.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUz9NaAAoJEM553pKExN6DuLwH/2tUl/BZ//yjXDuv9U8PeSP9
tgqyTIuM9j36qtT9jG+iX84IDGxm1AATLXI9HQbl3KkPuAYSKo9ECnxJZO0IaDeu
vCuhtNlSnP/Fr/xe8CZ1LcWNgQBEJLINkYZn5paA5qQybsr+Z/Ll/c0/0DuGooRt
8iIGYAFHyaUJx8dkinbaaLCwP9Fg5oeXx7PAi7kpRsRtMOo1LSTZEbCTx/zIlc7L
vjz+vipQuRJxzxzwqHhE6TFpCG7c/0QiUcECoVQ13zTPAumFX23w6BSr/4llWrr9
XV0Q9vExthrcIPZynchUnsTTBGT5gVYwXL1FI3b907gFqXYDXMWPRYlXm6pneY4=
=2c6g
-----END PGP SIGNATURE-----

Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

[Oleg Nesterov]

On 02/02, Rik van Riel wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/02/2015 02:21 PM, Oleg Nesterov wrote:
> > I'll try to read this patch tomorrow. Too late for me.
> >
> > I think it is fine, but
> >
> > On 02/02, riel@redhat.com wrote:
> >>
> >> This also fixes the lazy FPU restore disabling in drop_fpu,
> >> which only really works when !use_eager_fpu(). ...
> >>
> >> --- a/arch/x86/include/asm/fpu-internal.h +++
> >> b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static
> >> inline void drop_fpu(struct task_struct *tsk) * Forget
> >> coprocessor state.. */ preempt_disable(); -
> >> tsk->thread.fpu_counter = 0; +
> >> task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk);
> >> clear_used_math();
> >
> > perhaps this makes sense anyway, but I am not sure if the changelog
> > is right.
> >
> > Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect
> > after this.
>
> There are several code paths, including signal handler stack setup and
> teardown, where we first clear PF_USED_MATH, but later on set it again,
> after setting up a new math state for the task.
>
> We need to ensure we always use that new math state, and never lazily
> re-use what is still in the FPU registers.

Still can't understand....

I can only see only on example of re-setting PF_USED_MATH if eager_fpu,
__restore_xstate_sig() does drop_fpu() + math_state_restore(). And this
case looks fine in this sense...



Although let me repeat that imho math_state_restore() and all current users
(including flush_thread() added by "don't abuse FPU in kernel threads") need
cleanups in any case. I was going to (try to) do this if/when that series is
applied. In particular, in this case math_state_restore() will wrongly return
with irqs disabled or I am totally confused.

And set_used_math() if copy_from_user() fails doesn't look right, but this
is another story.


If I missed something, perhaps you can update the changelog to explain how
exactly it fixes drop_fpu? Otherwise, imo this patch should not change it.
As we already discussed, we can probably cleanup this disable_lazy_fpu
logic, but this needs another change/discussion.

Oleg.

Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/03/2015 02:08 PM, Oleg Nesterov wrote:
> On 02/02, Rik van Riel wrote:
>> 
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 02/02/2015 02:21 PM, Oleg Nesterov wrote:
>>> I'll try to read this patch tomorrow. Too late for me.
>>> 
>>> I think it is fine, but
>>> 
>>> On 02/02, riel@redhat.com wrote:
>>>> 
>>>> This also fixes the lazy FPU restore disabling in drop_fpu, 
>>>> which only really works when !use_eager_fpu(). ...
>>>> 
>>>> --- a/arch/x86/include/asm/fpu-internal.h +++ 
>>>> b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@
>>>> static inline void drop_fpu(struct task_struct *tsk) *
>>>> Forget coprocessor state.. */ preempt_disable(); - 
>>>> tsk->thread.fpu_counter = 0; + 
>>>> task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); 
>>>> clear_used_math();
>>> 
>>> perhaps this makes sense anyway, but I am not sure if the
>>> changelog is right.
>>> 
>>> Note that we clear PF_USED_MATH, last_fpu/has_fpu have no
>>> effect after this.
>> 
>> There are several code paths, including signal handler stack
>> setup and teardown, where we first clear PF_USED_MATH, but later
>> on set it again, after setting up a new math state for the task.
>> 
>> We need to ensure we always use that new math state, and never
>> lazily re-use what is still in the FPU registers.
> 
> Still can't understand....
> 
> I can only see only on example of re-setting PF_USED_MATH if
> eager_fpu, __restore_xstate_sig() does drop_fpu() +
> math_state_restore(). And this case looks fine in this sense...
> 
> 
> 
> Although let me repeat that imho math_state_restore() and all
> current users (including flush_thread() added by "don't abuse FPU
> in kernel threads") need cleanups in any case. I was going to (try
> to) do this if/when that series is applied. In particular, in this
> case math_state_restore() will wrongly return with irqs disabled or
> I am totally confused.
> 
> And set_used_math() if copy_from_user() fails doesn't look right,
> but this is another story.
> 
> 
> If I missed something, perhaps you can update the changelog to
> explain how exactly it fixes drop_fpu? Otherwise, imo this patch
> should not change it. As we already discussed, we can probably
> cleanup this disable_lazy_fpu logic, but this needs another
> change/discussion.

I don't think this changes the behaviour of any code path as the
kernel currently stands.

However, having drop_fpu() disable lazy restore in a more explicit
way may help us going forward, with the "delay FPU state restore to
kernel -> user space switch" series.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU0UUxAAoJEM553pKExN6DzSMIAIuPNXZ8P5zHAPMLePQhfaZK
QaatOplJ5F8GNN3bT2sJHLIPfDJVokMbGFv7ff8/jPsZ03uBm6onFPp/4Qn7dGZ4
FQyFX7OkmR7D4NJ0KZ9J1ghWhfRhmxfAqr/qwrdovUJ/Hfz73FdqGPYpP90qvY/2
0psSaW6ubJen6l9QYBear5juhQE3+akfwc/D1ItpZtZRUHOJTewBiww/9I/kUrDZ
mp1j4szVEjaQ9jB5Et4KO4EFaaEFzdj2JXwSV10neBgLrZ5dixYII1kUFQQFxvTd
bse2Rjsh0+6ESkUOoXLy1y0IYbjrQqEc+YdrGbnf64XJLIjj1a8U8F3dU5xPdCM=
=0+mw
-----END PGP SIGNATURE-----

Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/02/2015 02:21 PM, Oleg Nesterov wrote:
> I'll try to read this patch tomorrow. Too late for me.
> 
> I think it is fine, but
> 
> On 02/02, riel@redhat.com wrote:
>> 
>> This also fixes the lazy FPU restore disabling in drop_fpu,
>> which only really works when !use_eager_fpu(). ...
>> 
>> --- a/arch/x86/include/asm/fpu-internal.h +++
>> b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static
>> inline void drop_fpu(struct task_struct *tsk) * Forget
>> coprocessor state.. */ preempt_disable(); -
>> tsk->thread.fpu_counter = 0; +
>> task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); 
>> clear_used_math();
> 
> perhaps this makes sense anyway, but I am not sure if the changelog
> is right.
> 
> Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect
> after this.

After a few days on the RCU code, I'm back to this :)

You are right, I should drop the change above from the patch,
and disable lazy fpu restore explicitly in places where we
need it.

I will send a v2 of these patches, without patch 6/6.

Ingo, would you like me to also re-send Oleg's FPU patches that are
not in -tip yet?

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU1O71AAoJEM553pKExN6DUtEH/R8X02MHD7GUxO4hyiLECL9f
SQDdF1l+1rqcQo7e+5nRJ5/rFvAIaNA5oBMOZJ9MlJRd6HOKS+VqLLpb8idvApeK
zRiYUdeyoui3e0po2nNCx/GBfXqMYca+qH4JMMq6gzv0c9Nm9Zq9pbKDbO2znns8
10YQexvZsHUiJgt63YoZSwxFKD1iAhnTOGBhZOJu/s27Qtu+7gyuyIcuMMDFj2eu
ACGqZPcjG2u7TlC+D4uivSferMrduNnWsxGq3AY32/nNhjKGmXaFJxRHDdCsYAGJ
+S+SFuUWq6JoD7MuC02NQyAs/bQzqhVfn8v7gm2gromqIIQGzBABtI+Em0LGFP4=
=3R+T
-----END PGP SIGNATURE-----

[PATCH 5/6] x86,fpu: also check fpu_lazy_restore when use_eager_fpu

[riel]

From: Rik van Riel <riel@redhat.com>

With Oleg's patch "x86, fpu: don't abuse FPU in kernel threads if
use_eager_fpu()", kernel threads no longer have an FPU state even
on systems with use_eager_fpu()

That in turn means that a task may still have its FPU state
loaded in the FPU registers, if the task only got interrupted by
kernel threads from when it went to sleep, to when it woke up
again.

In that case, there is no need to restore the FPU state for
this task, since it is still in the registers.

The kernel can simply use the same logic to determine this as
is used for !use_eager_fpu() systems.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index e2832f9dfed5..085903415cbd 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -457,7 +457,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 		task_disable_lazy_fpu_restore(old);
 		if (fpu.preload) {
 			new->thread.fpu_counter++;
-			if (!use_eager_fpu() && fpu_lazy_restore(new, cpu))
+			if (fpu_lazy_restore(new, cpu))
 				fpu.preload = 0;
 			else
 				prefetch(new->thread.fpu.state);
-- 
1.9.3

Re: [PATCH 5/6] x86,fpu: also check fpu_lazy_restore when use_eager_fpu

[Oleg Nesterov]

On 02/02, riel@redhat.com wrote:
>
> From: Rik van Riel <riel@redhat.com>
>
> With Oleg's patch "x86, fpu: don't abuse FPU in kernel threads if
> use_eager_fpu()", kernel threads no longer have an FPU state even
> on systems with use_eager_fpu()
>
> That in turn means that a task may still have its FPU state
> loaded in the FPU registers, if the task only got interrupted by
> kernel threads from when it went to sleep, to when it woke up
> again.
>
> In that case, there is no need to restore the FPU state for
> this task, since it is still in the registers.
>
> The kernel can simply use the same logic to determine this as
> is used for !use_eager_fpu() systems.

Yes, agreed, I was going to do this too.

And in fact this change make sense even without "don't abuse FPU in kernel
threads", I think.

But in theory it depends on another change, "__kernel_fpu_begin() should
clear fpu_owner_task even if use_eager_fpu()".

And that series was ignored ;)

I think this patch is fine.

> --- a/arch/x86/include/asm/fpu-internal.h
> +++ b/arch/x86/include/asm/fpu-internal.h
> @@ -457,7 +457,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
>  		task_disable_lazy_fpu_restore(old);
>  		if (fpu.preload) {
>  			new->thread.fpu_counter++;
> -			if (!use_eager_fpu() && fpu_lazy_restore(new, cpu))
> +			if (fpu_lazy_restore(new, cpu))
>  				fpu.preload = 0;
>  			else
>  				prefetch(new->thread.fpu.state);
> --
> 1.9.3
>

Re: [PATCH 5/6] x86,fpu: also check fpu_lazy_restore when use_eager_fpu

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/02/2015 01:55 PM, Oleg Nesterov wrote:
> On 02/02, riel@redhat.com wrote:
>> 
>> From: Rik van Riel <riel@redhat.com>
>> 
>> With Oleg's patch "x86, fpu: don't abuse FPU in kernel threads
>> if use_eager_fpu()", kernel threads no longer have an FPU state
>> even on systems with use_eager_fpu()
>> 
>> That in turn means that a task may still have its FPU state 
>> loaded in the FPU registers, if the task only got interrupted by 
>> kernel threads from when it went to sleep, to when it woke up 
>> again.
>> 
>> In that case, there is no need to restore the FPU state for this
>> task, since it is still in the registers.
>> 
>> The kernel can simply use the same logic to determine this as is
>> used for !use_eager_fpu() systems.
> 
> Yes, agreed, I was going to do this too.
> 
> And in fact this change make sense even without "don't abuse FPU in
> kernel threads", I think.
> 
> But in theory it depends on another change, "__kernel_fpu_begin()
> should clear fpu_owner_task even if use_eager_fpu()".
> 
> And that series was ignored ;)
> 
> I think this patch is fine.

Ingo,

does the FPU code have a sub-maintainer, or should all the
FPU patches go straight through you?

Would it be better for you if FPU patches came through a git
tree you could just pull?

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUz83HAAoJEM553pKExN6DqrQH/iP5cfUUtIqguM4oS19WO33Y
5H/wdjVIGOJw4Rt7U+550Y9m5VMXsuQrO17PagjZGsxbSm9QQhA6esVMDvXfOFH1
taWW1tFog4VTMueNYbOC5asqsicTrhNqfiLQFM9CmJFGOPO4lDQ9n+OPS64CkQ/d
onylevtR4UWjggpEdkoOlmvbQH8RhnaC4JWKSXxP06YBakfP41gIMMfAjFNMP9O4
b/r1nU/WBQsTSX7pzQMnEx/Igp9LkT+X0Y93NerF/0O7gic9Wv7tKTEaUa3DzS7p
U7xgT8xX88AJ6QQGQfB3IQSkjcb3N9UCY1CYkkWqigm9MhhlLxPyRDAy7vkrUlk=
=j/eO
-----END PGP SIGNATURE-----

[PATCH 6/6] x86,fpu: remove redundant increments of fpu_counter

[riel]

From: Rik van Riel <riel@redhat.com>

fpu.preload only gets set if new->thread.fpu_counter is already
larger than 5. Incrementing it further does absolutely nothing.
Remove those lines.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 085903415cbd..88df5808a2db 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -447,7 +447,6 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 
 		/* Don't change CR0.TS if we just switch! */
 		if (fpu.preload) {
-			new->thread.fpu_counter++;
 			__thread_set_has_fpu(new);
 			prefetch(new->thread.fpu.state);
 		} else if (!use_eager_fpu())
@@ -456,7 +455,6 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 		old->thread.fpu_counter = 0;
 		task_disable_lazy_fpu_restore(old);
 		if (fpu.preload) {
-			new->thread.fpu_counter++;
 			if (fpu_lazy_restore(new, cpu))
 				fpu.preload = 0;
 			else
-- 
1.9.3

Re: [PATCH 6/6] x86,fpu: remove redundant increments of fpu_counter

[Oleg Nesterov]

On 02/02, riel@redhat.com wrote:
>
> From: Rik van Riel <riel@redhat.com>
>
> fpu.preload only gets set if new->thread.fpu_counter is already
> larger than 5. Incrementing it further does absolutely nothing.
> Remove those lines.

I _think_ that we increment it further on purpose. Note that fpu_counter
is "char", so it seems that we want no more than 256 automatic preloads.

So I am not sure about this change. At least the changelog doesn't look
right.

Oleg.

> --- a/arch/x86/include/asm/fpu-internal.h
> +++ b/arch/x86/include/asm/fpu-internal.h
> @@ -447,7 +447,6 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
>  
>  		/* Don't change CR0.TS if we just switch! */
>  		if (fpu.preload) {
> -			new->thread.fpu_counter++;
>  			__thread_set_has_fpu(new);
>  			prefetch(new->thread.fpu.state);
>  		} else if (!use_eager_fpu())
> @@ -456,7 +455,6 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
>  		old->thread.fpu_counter = 0;
>  		task_disable_lazy_fpu_restore(old);
>  		if (fpu.preload) {
> -			new->thread.fpu_counter++;
>  			if (fpu_lazy_restore(new, cpu))
>  				fpu.preload = 0;
>  			else
> -- 
> 1.9.3
> 

Re: [PATCH 6/6] x86,fpu: remove redundant increments of fpu_counter

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/02/2015 01:34 PM, Oleg Nesterov wrote:
> On 02/02, riel@redhat.com wrote:
>> 
>> From: Rik van Riel <riel@redhat.com>
>> 
>> fpu.preload only gets set if new->thread.fpu_counter is already 
>> larger than 5. Incrementing it further does absolutely nothing. 
>> Remove those lines.
> 
> I _think_ that we increment it further on purpose. Note that
> fpu_counter is "char", so it seems that we want no more than 256
> automatic preloads.
> 
> So I am not sure about this change. At least the changelog doesn't
> look right.

You are right, lets drop this patch 6/6.

Do the other five look right?

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUz8SFAAoJEM553pKExN6DQOcH/1+BrG+jIItcfZG3KwnN3Dfv
nXtmlNRiopMTJQ3HxzYEBT40zuqnKDSsf2cvMkcVJjt3oUukbD8D22iyJQzQxKnZ
g1MSJmZzSFZ1D0hoI/Qv3OLjyYJtcI4BaTHvs8f/YSmovTLY9D3e4zD37RbQp2d4
S0PdX/xxJBHpdJtFNaQni1VE9isIPh5x0YK8T55t5WhwRM+JwH+8C3mlL28ZEyQl
+BclNypHDiFKct+TL9NK+2RBRC2qwjuZzLi0BVrF49yK8G+Zw+8026vGrUOnx6Gg
XICJbyZbkBtn2J7Mc8i2xT2U4fYUy6pjcNuh0OiDSCV1f47iznBN0MjRwzc8ugo=
=mOwp
-----END PGP SIGNATURE-----

Re: [PATCH 6/6] x86,fpu: remove redundant increments of fpu_counter

[Ingo Molnar]

* Rik van Riel <riel@redhat.com> wrote:

> > So I am not sure about this change. At least the 
> > changelog doesn't look right.
> 
> You are right, lets drop this patch 6/6.
> 
> Do the other five look right?

Is there consensus on the first 5 patches?

Thanks,

	Ingo

Re: [PATCH 6/6] x86,fpu: remove redundant increments of fpu_counter

[Borislav Petkov]

On Thu, Feb 19, 2015 at 12:40:03AM +0100, Ingo Molnar wrote:
> Is there consensus on the first 5 patches?

I've picked up that series here:

https://lkml.kernel.org/r/1423252925-14451-1-git-send-email-riel@redhat.com

Maybe I should reply when picking up stuff so that we don't overlap.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 6/6] x86,fpu: remove redundant increments of fpu_counter

[Oleg Nesterov]

Borislav, Ingo,

On 02/19, Ingo Molnar wrote:
>
> Is there consensus on the first 5 patches?

Yes, but perhaps you can also look at another series

	[PATCH 1/3] x86, fpu: __kernel_fpu_begin() should clear fpu_owner_task even if use_eager_fpu()
	http://marc.info/?l=linux-kernel&m=142169357231860&w=2

	[PATCH 2/3] x86, fpu: always allow FPU in interrupt if use_eager_fpu()
	http://marc.info/?l=linux-kernel&m=142169359331866&w=2

	[PATCH 3/3] x86, fpu: don't abuse FPU in kernel threads if use_eager_fpu()
	http://marc.info/?l=linux-kernel&m=142169360931869&w=2

? (off course I will be happy to resend)

It was sent before the recently applied changes. And in fact in theory
728e53fef429a0f "x86/fpu: Also check fpu_lazy_restore() when use_eager_fpu()"
depends on 1/3 above.

Oleg.