move FPU context loading to userspace switch

From: Rik van Riel <[email protected]>

Add some documentation to data structures used for FPU context switching.

Signed-off-by Rik van Riel <[email protected]>

---
 arch/x86/include/asm/processor.h | 9 +++++++--
 arch/x86/kernel/cpu/common.c     | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

arch/x86/include/asm/processor.h +5 -0

index a092a0c..17bd8a0 100644
@@ -400,6 +400,11 @@ struct xsave_struct {
 	/* new processor state extensions will go here */
 } __attribute__ ((packed, aligned (64)));
 
+/*
+ * The FPU state used depends on the capabilities of the hardware; the
+ * registers used for vector instructions on newer hardware are included
+ * in the FPU state.
+ */
 union thread_xstate {
 	struct i387_fsave_struct	fsave;
 	struct i387_fxsave_struct	fxsave;

Quoted 11 lines · On Sun, Jan 11, 2015 at 04:46:23PM -0500, [email protected] wrote:

+++ b/arch/x86/include/asm/processor.h @@ -400,6 +400,11 @@ struct xsave_struct { /* new processor state extensions will go here */ } __attribute__ ((packed, aligned (64)));

+/* + * The FPU state used depends on the capabilities of the hardware; the + * registers used for vector instructions on newer hardware are included + * in the FPU state. + */ union
thread_xstate { struct i387_fsave_struct fsave; struct
i387_fxsave_struct fxsave; @@ -408,8 +413,8 @@ union
thread_xstate { };

Quoted 2 lines

struct fpu { - unsigned int last_cpu; - unsigned int has_fpu; +
unsigned int last_cpu; /* FPU state last loaded on this CPU */

Isn't that the last CPU which had the FPU?

arch/x86/include/asm/processor.h union thread_xstate { · line 417

Quoted 1 line

arch/x86/include/asm/processor.h +1 -0

+	unsigned int has_fpu;		/* FPU state in current use on CPU */

I understand ->has_fpu as this thread has the FPU. See comment over user_has_fpu().

+/*
+ * The FPU state used depends on the capabilities of the hardware; the
+ * registers used for vector instructions on newer hardware are included
+ * in the FPU state.
+ */
union thread_xstate {

Quoted 4 lines · On 01/11/2015 01:46 PM, [email protected] wrote:

+/* + * The FPU state used depends on the capabilities of the hardware; the + * registers used for vector instructions on newer hardware are included + * in the FPU state. + */ union thread_xstate {

Tiny nit, if you happen to revise this set at some point...

While you probably don't need/want to have an exhaustive list of everything that gets saved in there, we should probably still mention that it's a lot more than just vector instructions.

I'd say:

The FPU state used depends on the capabilities of the hardware. This state increasingly contains things not related to the FPU itself, like registers used for vector instructions.

arch/x86/include/asm/fpu-internal.h +4 -4

+static inline void switch_fpu_prepare(struct task_struct *old, struct task_struct *new, int cpu)
 {
-	fpu_switch_t fpu;
-
 	/*
 	 * If the task has used the math, pre-load the FPU on xsave processors
 	 * or if the past 5 consecutive context-switches used math.
 	 */
-	fpu.preload = tsk_used_math(new) && (use_eager_fpu() ||
+	bool preload = tsk_used_math(new) && (use_eager_fpu() ||
 					     new->thread.fpu_counter > 5);
 	if (__thread_has_fpu(old)) {
 		if (!__save_init_fpu(old))

@@ -433,8 +417,9 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 		old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task! */
 
 		/* Don't change CR0.TS if we just switch! */
-		if (fpu.preload) {
+		if (preload) {
 			new->thread.fpu_counter++;
+			set_thread_flag(TIF_LOAD_FPU);
 			__thread_set_has_fpu(new);
 			prefetch(new->thread.fpu.state);
 		} else if (!use_eager_fpu())

arch/x86/include/asm/thread_info.h +1 -0

 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)

@@ -115,6 +116,7 @@ struct thread_info {
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 #define _TIF_ADDR32		(1 << TIF_ADDR32)
 #define _TIF_X32		(1 << TIF_X32)
+#define _TIF_LOAD_FPU		(1 << TIF_LOAD_FPU)
 
 /* work to do in syscall_trace_enter() */
 #define _TIF_WORK_SYSCALL_ENTRY	\

Rik,

I can't review this series, I forgot almost everything I learned about this code. The only thing I can recall is that it needs cleanups and fixes ;) Just a couple of random questions.

Quoted 12 lines · On 01/11, [email protected] wrote:

+static inline void switch_fpu_prepare(struct task_struct *old,
struct task_struct *new, int cpu) { - fpu_switch_t fpu; - /* * If
the task has used the math, pre-load the FPU on xsave processors
* or if the past 5 consecutive context-switches used math. */ -
fpu.preload = tsk_used_math(new) && (use_eager_fpu() || + bool
preload = tsk_used_math(new) && (use_eager_fpu() || new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if (!__save_init_fpu(old)) @@ -433,8 +417,9 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct
ta old->thread.fpu.has_fpu = 0; /* But leave fpu_owner_task! */

Quoted 10 lines

/* Don't change CR0.TS if we just switch! */ - if (fpu.preload)
{ + if (preload) { new->thread.fpu_counter++; +
set_thread_flag(TIF_LOAD_FPU); __thread_set_has_fpu(new); prefetch(new->thread.fpu.state); } else if (!use_eager_fpu()) @@ -442,16 +427,19 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta } else {
old->thread.fpu_counter = 0; old->thread.fpu.last_cpu = ~0; - if
(fpu.preload) { + if (preload) { new->thread.fpu_counter++; if
(!use_eager_fpu() && fpu_lazy_restore(new, cpu)) - fpu.preload
= 0; - else + /* XXX: is this safe against ptrace??? */

Could you explain your concerns?

arch/x86/include/asm/fpu-internal.h static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta · line 434

Quoted 1 line

arch/x86/include/asm/fpu-internal.h +1 -0

+				__thread_fpu_begin(new);

this looks strange/unnecessary, there is another unconditonal __thread_fpu_begin(new) below.

OK, the next patch moves it to switch_fpu_finish(), so perhaps this change should go into 3/11.

And I am not sure I understand set_thread_flag(TIF_LOAD_FPU). This is called before __switch_to() updates kernel_stack, so it seems that the old thread gets this flag set, not new?

Even if this is correct, perhaps set_tsk_thread_flag(new) will look better?

Quoted 7 lines

--- a/arch/x86/include/asm/thread_info.h +++ b/arch/x86/include/asm/thread_info.h @@ -91,6 +91,7 @@ struct
thread_info { #define TIF_SYSCALL_TRACEPOINT 28 /* syscall
tracepoint instrumentation */ #define TIF_ADDR32 29 /* 32-bit
address space on 64 bits */ #define TIF_X32 30 /* 32-bit native
x86-64 binary */ +#define TIF_LOAD_FPU 31 /* load FPU on return
to userspace */

Well, the comment is wrong after this patch, but I see 4/11...

Quoted 6 lines

/* work to do in syscall_trace_enter() */ #define
_TIF_WORK_SYSCALL_ENTRY \ @@ -141,7 +143,7 @@ struct thread_info
{ /* Only used for 64 bit */ #define _TIF_DO_NOTIFY_MASK \
(_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME | \ -
_TIF_USER_RETURN_NOTIFY | _TIF_UPROBE) + _TIF_USER_RETURN_NOTIFY
| _TIF_UPROBE | _TIF_LOAD_FPU)

This too. I mean, this change has no effect until 4/11.

Quoted 30 lines · Oleg Nesterov

Rik,

I can't review this series, I forgot almost everything I learned about this code. The only thing I can recall is that it needs cleanups and fixes ;) Just a couple of random questions.

Quoted 22 lines · On 01/11, [email protected] wrote:

+static inline void switch_fpu_prepare(struct task_struct *old,
struct task_struct *new, int cpu) { - fpu_switch_t fpu; - /* * If
the task has used the math, pre-load the FPU on xsave processors
* or if the past 5 consecutive context-switches used math. */ -
fpu.preload = tsk_used_math(new) && (use_eager_fpu() || + bool
preload = tsk_used_math(new) && (use_eager_fpu() || new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if (!__save_init_fpu(old)) @@ -433,8 +417,9 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct
ta old->thread.fpu.has_fpu = 0; /* But leave fpu_owner_task! */

/* Don't change CR0.TS if we just switch! */ - if (fpu.preload)
{ + if (preload) { new->thread.fpu_counter++; +
set_thread_flag(TIF_LOAD_FPU); __thread_set_has_fpu(new); prefetch(new->thread.fpu.state); } else if (!use_eager_fpu()) @@ -442,16 +427,19 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta } else {
old->thread.fpu_counter = 0; old->thread.fpu.last_cpu = ~0; - if
(fpu.preload) { + if (preload) { new->thread.fpu_counter++; if
(!use_eager_fpu() && fpu_lazy_restore(new, cpu)) - fpu.preload
= 0; - else + /* XXX: is this safe against ptrace??? */

Quoted 1 line

Could you explain your concerns?

Ptrace could modify the in-memory copy of a task's FPU context, while fpu_lazy_restore() could decide that the task's FPU context is still loaded in the registers (nothing else on the CPU has used the FPU since it last ran), and does not need to be re-loaded.

Quoted 1 line

Well, the comment is wrong after this patch, but I see 4/11...

I did not want to change that same line in two different patches, with the idea that that would make things harder to review.

Quoted 7 lines

Quoted 6 lines

/* work to do in syscall_trace_enter() */ #define
_TIF_WORK_SYSCALL_ENTRY \ @@ -141,7 +143,7 @@ struct thread_info
{ /* Only used for 64 bit */ #define _TIF_DO_NOTIFY_MASK \
(_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME | \ -
_TIF_USER_RETURN_NOTIFY | _TIF_UPROBE) + _TIF_USER_RETURN_NOTIFY
| _TIF_UPROBE | _TIF_LOAD_FPU)

Quoted 1 line

This too. I mean, this change has no effect until 4/11.

I can move this line to patch 4/11 if you prefer.

arch/x86/include/asm/fpu-internal.h +0 -2

@@ -420,7 +420,6 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc
 		if (preload) {
 			new->thread.fpu_counter++;
 			set_thread_flag(TIF_LOAD_FPU);
-			__thread_set_has_fpu(new);
 			prefetch(new->thread.fpu.state);
 		} else if (!use_eager_fpu())
 			stts();

@@ -436,7 +435,6 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc
 				prefetch(new->thread.fpu.state);
 				set_thread_flag(TIF_LOAD_FPU);
 			}
-			__thread_fpu_begin(new);
 		}
 		/* else: CR0.TS is still set from a previous FPU switch */
 	}

Quoted 12 lines · On 01/11, [email protected] wrote:

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -420,7 +420,6 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc if (preload) { new->thread.fpu_counter++;
set_thread_flag(TIF_LOAD_FPU); - __thread_set_has_fpu(new);
prefetch(new->thread.fpu.state); } else if (!use_eager_fpu()) stts(); @@ -436,7 +435,6 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc prefetch(new->thread.fpu.state); set_thread_flag(TIF_LOAD_FPU);
} - __thread_fpu_begin(new); } /* else: CR0.TS is still set
from a previous FPU switch */ } @@ -451,6 +449,7 @@ static inline

Quoted 5 lines

void switch_fpu_prepare(struct task_struct *old, struct task_struc static inline void switch_fpu_finish(struct task_struct *new) { if (test_and_clear_thread_flag(TIF_LOAD_FPU))
{ + __thread_fpu_begin(new); if
(unlikely(restore_fpu_checking(new))) drop_init_fpu(new); }

Then perhaps it makes sense to move fpu_lazy_restore() to fpu_finish() too ?

Either way, afaics we do not need use_eager_fpu() before fpu_lazy_restore(), and this reminds me that every use_eager_fpu() check in switch_fpu_prepare() looks confusing.

arch/x86/include/asm/fpu-internal.h +1 -3

-static inline void switch_fpu_finish(struct task_struct *new)
+static inline void switch_fpu_finish(void)
 {
-	if (test_and_clear_thread_flag(TIF_LOAD_FPU)) {
-		__thread_fpu_begin(new);

From: Rik van Riel <[email protected]>

Defer restoring the FPU state, if so desired, until the task returns to userspace.

In case of kernel threads, KVM VCPU threads, and tasks performing longer running operations in kernel space, this could mean skipping the FPU state restore entirely for several context switches.

This also allows the kernel to preserve the FPU state of a userspace task that gets interrupted by a kernel thread in kernel mode.

If the old task left TIF_LOAD_FPU set, and the new task does not want an FPU state restore (or does not have an FPU state), clear the flag to prevent attempted restoring of a non-existent FPU state.

TODO: remove superfluous irq disabling from entry_{32,64}.S, Andy has some conflicting changes in that part of the code, so wait with that.

Signed-off-by Rik van Riel <[email protected]>

---
 arch/x86/include/asm/fpu-internal.h | 33 +++++++++++++++++++++------------
 arch/x86/kernel/process_32.c        |  2 --
 arch/x86/kernel/process_64.c        |  2 --
 arch/x86/kernel/signal.c            |  9 +++++++++
 4 files changed, 30 insertions(+), 16 deletions(-)

arch/x86/include/asm/fpu-internal.h +21 -12

index 27556f4..539b050 100644
@@ -382,6 +382,7 @@ static inline void drop_init_fpu(struct task_struct *tsk)
                else
                        fxrstor_checking(&init_xstate_buf->i387);
        }
+       clear_thread_flag(TIF_LOAD_FPU);
 }
 
 /*

@@ -435,24 +436,32 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc
                                prefetch(new->thread.fpu.state);
                                set_thread_flag(TIF_LOAD_FPU);
                        }
-               }
-               /* else: CR0.TS is still set from a previous FPU switch */
+               } else
+                       /*
+                        * The new task does not want an FPU state restore,
+                        * and may not even have an FPU state. However, the
+                        * old task may have left TIF_LOAD_FPU set.
+                        * Clear it to avoid trouble.
+                        *
+                        * CR0.TS is still set from a previous FPU switch
+                        */
+                       clear_thread_flag(TIF_LOAD_FPU);
        }
 }
 
 /*
- * By the time this gets called, we've already cleared CR0.TS and
- * given the process the FPU if we are going to preload the FPU
- * state - all we need to do is to conditionally restore the register
- * state itself.
+ * This is called if and when the task returns to userspace.
+ * Clear CR0.TS if necessary, so the task can access the FPU register
+ * state this function restores.
  */
-static inline void switch_fpu_finish(struct task_struct *new)
+static inline void switch_fpu_finish(void)
 {
-       if (test_and_clear_thread_flag(TIF_LOAD_FPU)) {
-               __thread_fpu_begin(new);
-               if (unlikely(restore_fpu_checking(new)))
-                       drop_init_fpu(new);
-       }
+       struct task_struct *tsk = current;
+
+       __thread_fpu_begin(tsk);
+
+       if (unlikely(restore_fpu_checking(tsk)))
+               drop_init_fpu(tsk);
 }
 
 /*

arch/x86/kernel/process_32.c +0 -2

index c4b00e6..4da02ae 100644
@@ -319,8 +319,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
        if (prev->gs | next->gs)
                lazy_load_gs(next->gs);
 
-       switch_fpu_finish(next_p);
-
        this_cpu_write(current_task, next_p);
 
        return prev_p;

arch/x86/kernel/process_64.c +0 -2

index ee3824f..20e206e 100644
@@ -350,8 +350,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
                wrmsrl(MSR_KERNEL_GS_BASE, next->gs);
        prev->gsindex = gsindex;
 
-       switch_fpu_finish(next_p);
-
        /*
         * Switch the PDA and FPU contexts.
         */

arch/x86/kernel/signal.c +9 -1

index ed37a76..46e3008 100644
@@ -761,6 +761,15 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
                fire_user_return_notifiers();
 
        user_enter();
+
+       /*
+        * Test thread flag, as the signal code may have cleared TIF_LOAD_FPU.
+        * We cannot reschedule after loading the FPU state back into the CPU.
+        * IRQs will be re-enabled on the switch to userspace.
+        */
+       local_irq_disable();
+       if (test_thread_flag(TIF_LOAD_FPU))
+               switch_fpu_finish();
 }
 
 void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
--

1.9.3

Defer restoring the FPU state, if so desired, until the task returns to userspace.

Quoted 3 lines · On 01/11, [email protected] wrote:

Defer restoring the FPU state, if so desired, until the task returns to userspace.

And I have another concern.

Afaocs with this patch the idle threads will run with TIF_LOAD_FPU set but without fpu.has_fpu.

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · On 01/11, [email protected] wrote:

Defer restoring the FPU state, if so desired, until the task returns to userspace.

And I have another concern.

Afaocs with this patch the idle threads will run with TIF_LOAD_FPU set but without fpu.has_fpu.

Yuck. IMO there are still too many possible states.

AFAICS the sensible states for a task are:

- Task is current on some cpu and FPU is loaded into that cpu. - Task is current on some cpu and FPU is in memory. - Task is current on some cpu and FPU is loaded into a different cpu.

- Task is not current and FPU is in memory. - Task is not current and FPU is loaded into some cpu.

Am I missing anything? (In lazy mode, there are a few more involving CR0.TS.)

That's five states, plus an optional cpu number. But we have tons of state variable that can express all kinds of nonsense things.

If we asserted that we were in a sensible state and fixed the things that exited the sensible states, maybe this would be easier to understand and debug.

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

Quoted 16 lines · Andy Lutomirski

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · On 01/11, [email protected] wrote:

Defer restoring the FPU state, if so desired, until the task returns to userspace.

And I have another concern.

Afaocs with this patch the idle threads will run with TIF_LOAD_FPU set but without fpu.has_fpu.

Yuck. IMO there are still too many possible states.

AFAICS the sensible states for a task are:

Quoted 3 lines

- Task is current on some cpu and FPU is loaded into that cpu. - Task is current on some cpu and FPU is in memory. - Task is current on some cpu and FPU is loaded into a different cpu.

When switching a task out, in prepare_fpu_switch() we will always save the FPU state to memory. The third case can be treated identical to the second case.

Quoted 3 lines

- Task is not current and FPU is in memory. - Task is not current and FPU is loaded into some cpu.

Same for this one. When the task is not current, the FPU state will have been saved to memory. If we try running the task somewhere else, it devolves to "FPU is in memory".

Quoted 9 lines

Am I missing anything? (In lazy mode, there are a few more involving CR0.TS.)

That's five states, plus an optional cpu number. But we have tons of state variable that can express all kinds of nonsense things.

If we asserted that we were in a sensible state and fixed the things that exited the sensible states, maybe this would be easier to understand and debug.

Lets see what things we could test, at different points.

1) At context switch time, we need to make sure that the previous task will no longer have __thread_has_fpu()

2) When loading the FPU state, we need to make sure that the current task does not have __thread_has_fpu()

3) ... what else?

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

iQEcBAEBAgAGBQJUtVmPAAoJEM553pKExN6Dc0gIAI0JZE4VpNlJElnuGbFKzM8t r9dSH3XLeEX5/JARsQMNuwUL7HjjBbYO9PrdpejVAEUq2XltVyq3+RuSYMBprlpV aCWKrw9jn7H43++LSziXh0LP+t+zqL9+/5WUm3oVWkTXJEBu9CdNgYYP7luf87nX +mxqDsp5YjmmYJRW2LncK/SuEpItTAtHI5bORjsbWNQOZ2pl31cr1ZEbdrRaZqje 5wONfs0f2mor0Ms7FSzVJCNmps3TGJnfpkx37vzqRiPkisdWVcM16RwvM6jJTFZ+ VmM2Jj4xWMhPYvqpZ8oZWAb8xNCl+DijjZFEGW1j9Ji5gcPQ2HLvFEWNp3TpIeM= =OU/0
-----END PGP SIGNATURE-----

Quoted 6 lines · Andy Lutomirski

Quoted 2 lines

Quoted 2 lines

- Task is not current and FPU is in memory. - Task is not current and FPU is loaded into some cpu.

Same for this one. When the task is not current, the FPU state will have been saved to memory. If we try running the task somewhere else, it devolves to "FPU is in memory".

Isn't there a case where the FPU is in memory *and* in the cpu regs? Isn't that how you can skip reloading the FPU after going idle and returning? Is this what fpu_lazy_restore is for? Confused.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Am I missing anything? (In lazy mode, there are a few more involving CR0.TS.)

That's five states, plus an optional cpu number. But we have tons of state variable that can express all kinds of nonsense things.

If we asserted that we were in a sensible state and fixed the things that exited the sensible states, maybe this would be easier to understand and debug.

Lets see what things we could test, at different points.

1) At context switch time, we need to make sure that the previous task will no longer have __thread_has_fpu()

2) When loading the FPU state, we need to make sure that the current task does not have __thread_has_fpu()

Quoted 3 lines

Examples, any of which may be wrong:

If !current, then !TIF_LOAD_FPU

Quoted 1 line

If switching out a task with TIF_LOAD_FPU set, then !has_fpu

Quoted 1 line

If last_cpu == smp_processor_id(), then fpu_owner == fpu.

Quoted 2 lines

If has_fpu, then the task must be current somewhere and last_cpu must be the cpu on which it's current.

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

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Andy Lutomirski

Quoted 2 lines

Quoted 2 lines

- Task is not current and FPU is in memory. - Task is not current and FPU is loaded into some cpu.

Same for this one. When the task is not current, the FPU state will have been saved to memory. If we try running the task somewhere else, it devolves to "FPU is in memory".

Quoted 4 lines

Isn't there a case where the FPU is in memory *and* in the cpu regs? Isn't that how you can skip reloading the FPU after going idle and returning? Is this what fpu_lazy_restore is for? Confused.

Indeed, if we end up running the task on the same CPU again, and the FPU still has the state loaded, we may skip restoring the FPU state.

Quoted 11 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Am I missing anything? (In lazy mode, there are a few more involving CR0.TS.)

That's five states, plus an optional cpu number. But we have tons of state variable that can express all kinds of nonsense things.

If we asserted that we were in a sensible state and fixed the things that exited the sensible states, maybe this would be easier to understand and debug.

Quoted 11 lines

Lets see what things we could test, at different points.

1) At context switch time, we need to make sure that the previous task will no longer have __thread_has_fpu()

2) When loading the FPU state, we need to make sure that the current task does not have __thread_has_fpu()

Quoted 3 lines

Examples, any of which may be wrong:

If !current, then !TIF_LOAD_FPU

We set TIF_LOAD_CPU on the next task at context switch time, which is different from the current task. I suspect we can deal with that exception, though :)

What we can test is that "new" does not already have TIF_LOAD_CPU set...

Quoted 1 line

Quoted 1 line

If switching out a task with TIF_LOAD_FPU set, then !has_fpu

... and that old does not have both TIF_LOAD_FPU and has_fpu.

Quoted 1 line

Quoted 1 line

If last_cpu == smp_processor_id(), then fpu_owner == fpu.

Not necessarily, since the task may not have entered userspace in a very long time, so it may not have loaded its FPU context.

Quoted 2 lines

Quoted 2 lines

If has_fpu, then the task must be current somewhere and last_cpu must be the cpu on which it's current.

Indeed, if has_fpu, then last_cpu must match the current cpu.

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

iQEcBAEBAgAGBQJUtWBOAAoJEM553pKExN6DNwIH/2wzfLqqM1V/Asd29nidDUrw zD7HN//LyWTLjNMfAS4M/rOk3LsbphFBOo2L5BE7CYoNAGEWwKcQi7ld3dDAXeZL +AkRtzMNEU1NqzrtnpGhABBrn3wBXwr9ldKSlaVQhYUop3q5Hhg8lyo2v+wWKf7y ULi/RLiERS72tUomFXTE4RT021N2h+tl42jSREEyT0+VqEc7vqTlb5fctsF3VAhS g48fX/VOYit3rXFU9hPz9m9vnodsEGCapdRxsXaE4xA7lg8dZ5WsaAos2TUwPQYt EyCbS9z2Yzy1UpySwZudo6OGbQIaugOtgrcCS/cvdvlRb8K4mLe+807MPGmBOGA= =7wEX
-----END PGP SIGNATURE-----

Quoted 3 lines · On 01/11, [email protected] wrote:

Defer restoring the FPU state, if so desired, until the task returns to userspace.

And I have another concern.

Afaocs with this patch the idle threads will run with TIF_LOAD_FPU set but without fpu.has_fpu.

This is fine by itself, but this (performance-wise) breaks kernel_fpu_begin() if use_eager_fpu() == T. Please see the changelog in 5187b28ff08249ab8a162e8 and note that this patch cc's @stable.

Yes, yes, even if I am right we should blame kernel_fpu_begin() and other eager_fpu oddities. I tried to start the cleanups/fixes in this area some time ago, but they were ignored.

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

Quoted 10 lines · Oleg Nesterov

Quoted 3 lines · On 01/11, [email protected] wrote:

Defer restoring the FPU state, if so desired, until the task returns to userspace.

And I have another concern.

Afaocs with this patch the idle threads will run with TIF_LOAD_FPU set but without fpu.has_fpu.

Quoted 8 lines

This is fine by itself, but this (performance-wise) breaks kernel_fpu_begin() if use_eager_fpu() == T. Please see the changelog in 5187b28ff08249ab8a162e8 and note that this patch cc's @stable.

Yes, yes, even if I am right we should blame kernel_fpu_begin() and other eager_fpu oddities. I tried to start the cleanups/fixes in this area some time ago, but they were ignored.

I suppose we could make kernel_fpu_begin() explicitly point the cpu's fpu pointer at a special value to indicate that we are in the middle of a kernel_fpu_begin() / kernel_fpu_end() session, and should not use the FPU from interrupt context right now.

But yes, I think we need the per-cpu "in_kernel_fpu" and irq_fpu_usable() must die. Please look at http://marc.info/?l=linux-kernel&m=14096628660929

Quoted 4 lines · Oleg Nesterov

But yes, I think we need the per-cpu "in_kernel_fpu" and irq_fpu_usable() must die. Please look at
http://marc.info/?l=linux-kernel&m=14096628660929

Plus I think we need this to fix the race with
math_state_restore()... Please see
http://marc.info/?l=linux-kernel&m=140992479607206

Unfortunately I forgot other problems I found in this area... And I completely forgot the plan I had in mind ;)

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

On 01/13/2015 01:30 PM, Oleg Nesterov wrote:

Quoted 5 lines · Oleg Nesterov

Quoted 4 lines

But yes, I think we need the per-cpu "in_kernel_fpu" and irq_fpu_usable() must die. Please look at
http://marc.info/?l=linux-kernel&m=14096628660929

This link does not work for me. I wonder if that email is not on the marc.info node I managed to send my http request to...

Quoted 6 lines

Plus I think we need this to fix the race with
math_state_restore()... Please see
http://marc.info/?l=linux-kernel&m=140992479607206

Unfortunately I forgot other problems I found in this area... And I completely forgot the plan I had in mind ;)

That I can certainly merge into my series.

Defer restoring the FPU state, if so desired, until the task returns to userspace.

Quoted 3 lines · On 01/11, [email protected] wrote:

Defer restoring the FPU state, if so desired, until the task returns to userspace.

And yet another concern ;) Although I feel that I am totally confused and probably wrong.

Quoted 5 lines

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -382,6 +382,7 @@ static inline void drop_init_fpu(struct task_struct *tsk) else fxrstor_checking(&init_xstate_buf->i387); } + clear_thread_flag(TIF_LOAD_FPU); }

OK, but shouldn't (say) restore_user_xstate() clear TIF_LOAD_FPU too? Otherwise, can't switch_fpu_finish() restore the wrong context later?

Or. Perhaps switch_fpu_finish() should do nothing if fpu.has_fpu == T, I dunno.

Quoted 5 lines · On 01/11, [email protected] wrote:

(quoted diff) +0 -0

@@ -412,8 +412,14 @@ static inline void switch_fpu_prepare(struct

task_struct *old, struct task_struc bool preload = tsk_used_math(new) && (use_eager_fpu() || new->thread.fpu_counter

Quoted 1 line

5); if (__thread_has_fpu(old)) { - if (!__save_init_fpu(old))

Quoted 5 lines

(quoted diff) +1 -0

+		/* +		 * Make sure the FPU state is restored from memory next

time, + * if the task has an FPU exception pending, or the
task's in + * memory FPU state could be changed by a debugger.

(quoted diff) +1 -0

+		 */ +		if (!__save_init_fpu(old) ||

task_is_stopped_or_traced(old)) cpu = ~0;

Well, if debugger wants to change FPU state, it should call init_fpu() which resets .last_cpu ?

Plus "is_stopped" is not needed, ptracer no longer can do STOPPED -> TRACED transition.

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -0

@@ -412,8 +412,14 @@ static inline void switch_fpu_prepare(struct

task_struct *old, struct task_struc bool preload = tsk_used_math(new) && (use_eager_fpu() || new->thread.fpu_counter

Quoted 1 line

5); if (__thread_has_fpu(old)) { - if (!__save_init_fpu(old))

Quoted 8 lines

Quoted 5 lines

(quoted diff) +1 -0

+		/* +		 * Make sure the FPU state is restored from memory next

time, + * if the task has an FPU exception pending, or the
task's in + * memory FPU state could be changed by a debugger.

(quoted diff) +1 -0

+		 */ +		if (!__save_init_fpu(old) ||

task_is_stopped_or_traced(old)) cpu = ~0;

Well, if debugger wants to change FPU state, it should call init_fpu() which resets .last_cpu ?

Does the ptrace (and utrace, and ... ) code actually do that?

Quoted 16 lines · Rik van Riel

Quoted 15 lines

Quoted 8 lines

(quoted diff) +0 -0

@@ -412,8 +412,14 @@ static inline void

switch_fpu_prepare(struct task_struct *old, struct task_struc bool preload = tsk_used_math(new) && (use_eager_fpu() || new->thread.fpu_counter

Quoted 2 lines

5); if (__thread_has_fpu(old)) { - if
(!__save_init_fpu(old))

(quoted diff) +1 -0

+		/* +		 * Make sure the FPU state is restored from memory

Quoted 4 lines

next time, + * if the task has an FPU exception pending, or
the task's in + * memory FPU state could be changed by a
debugger. + */ + if (!__save_init_fpu(old) ||
task_is_stopped_or_traced(old)) cpu = ~0;

Well, if debugger wants to change FPU state, it should call init_fpu() which resets .last_cpu ?

Quoted 1 line

Does the ptrace (and utrace, and ... ) code actually do that?

Yes, see xfpregs_get/set. So I think this change is not needed (but I didn't look at the next patches).

From: Rik van Riel <[email protected]>

If the next task still has its FPU state present in the FPU registers, there is no need to restore it from memory.

This is no big deal on bare metal, where XSAVEOPT / XRSTOR are heavily optimized, but those optimizations do not carry across VMENTER / VMEXIT.

Skipping the call to fpu_restore_checking when the FPU state is already loaded in the CPU could save a little bit of overhead on bare metal too, so this is not just a KVM optimization.

Signed-off-by Rik van Riel <[email protected]>

---
 arch/x86/include/asm/fpu-internal.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/x86/include/asm/fpu-internal.h +0 -0

index 4db8781..a5a40c7 100644

arch/x86/include/asm/fpu-internal.h +4 -3

-                               set_thread_flag(TIF_LOAD_FPU);
-                       }
                } else
                        /*
                         * The new task does not want an FPU state restore,

@@ -466,6 +462,10 @@ static inline void switch_fpu_finish(void)
 
        __thread_fpu_begin(tsk);
 
+       /* The FPU registers already have this task's FPU state. */
+       if (fpu_lazy_restore(tsk, raw_smp_processor_id()))
+               return;
+
        if (unlikely(restore_fpu_checking(tsk)))
                drop_init_fpu(tsk);
 }
--

1.9.3

Quoted 12 lines · On Sun, Jan 11, 2015 at 1:46 PM, <[email protected]> wrote:

(quoted diff) +0 -0

b/arch/x86/include/asm/fpu-internal.h index 4db8781..a5a40c7 100644 --- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -435,13 +435,9 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc old->thread.fpu.last_cpu = ~0; if (preload) {
new->thread.fpu_counter++; - if
(!use_eager_fpu() && fpu_lazy_restore(new, cpu)) -
/* XXX: is this safe against ptrace??? */ -
__thread_fpu_begin(new); - else { +
set_thread_flag(TIF_LOAD_FPU); + if
(!fpu_lazy_restore(new, cpu)) prefetch(new->thread.fpu.state);

Is this prefetch still worth keeping? Wouldn't prefetching the fpu effectively require grabbing more than one cacheline anyway?

If the next task still has its FPU state present in the FPU registers, there is no need to restore it from memory.

arch/x86/include/asm/fpu-internal.h +0 -0

@@ -435,13 +435,9 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc
 		old->thread.fpu.last_cpu = ~0;
 		if (preload) {

Quoted 3 lines · On 01/11, [email protected] wrote:

If the next task still has its FPU state present in the FPU registers, there is no need to restore it from memory.

Another patch I can't understand...

Quoted 10 lines

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -435,13 +435,9 @@ static inline void switch_fpu_prepare(struct task_struct *old, struct task_struc old->thread.fpu.last_cpu = ~0; if (preload) {
new->thread.fpu_counter++; - if (!use_eager_fpu() &&
fpu_lazy_restore(new, cpu)) - /* XXX: is this safe against
ptrace??? */ - __thread_fpu_begin(new); - else { +
set_thread_flag(TIF_LOAD_FPU); + if (!fpu_lazy_restore(new,
cpu)) prefetch(new->thread.fpu.state); -
set_thread_flag(TIF_LOAD_FPU); - }

It is not clear to me why do we set TIF_LOAD_FPU if fpu_lazy_restore() succeeds. __thread_fpu_begin() is cheap.

At the same time, if switch_fpu_finish() does fpu_lazy_restore() anyway, why this patch doesn't remove it from switch_fpu_prepare() ?

Quoted 6 lines

arch/x86/include/asm/fpu-internal.h +1 -0

@@ -466,6 +462,10 @@ static inline void switch_fpu_finish(void)
 
 __thread_fpu_begin(tsk);
 
+	/* The FPU registers already have this task's FPU state. */ +

if (fpu_lazy_restore(tsk, raw_smp_processor_id())) + return; +

Now that this is called before return to user-mode, I am not sure this is correct. Note that __kernel_fpu_begin() doesn't clear fpu_owner_task if use_eager_fpu().

Quoted 10 lines · Oleg Nesterov

Quoted 6 lines

arch/x86/include/asm/fpu-internal.h +1 -0

@@ -466,6 +462,10 @@ static inline void switch_fpu_finish(void)
 
 __thread_fpu_begin(tsk);
 
+	/* The FPU registers already have this task's FPU state. */ +

if (fpu_lazy_restore(tsk, raw_smp_processor_id())) + return; +

Now that this is called before return to user-mode, I am not sure this is correct. Note that __kernel_fpu_begin() doesn't clear fpu_owner_task if use_eager_fpu().

However, __kernel_fpu_begin() does call __thread_clear_has_fpu(), which clears the per-cpu fpu_owner variable, which is also evaluated by fpu_lazy_restore(), so I think this is actually correct.

arch/x86/kernel/i387.c +0 -0

@@ -89,13 +89,11 @@ void __kernel_fpu_end(void)
 	if (use_eager_fpu()) {
 		/*
 		 * For eager fpu, most the time, tsk_used_math() is true.

Quoted 11 lines · On 01/11, [email protected] wrote:

arch/x86/kernel/i387.c +0 -4

@@ -89,13 +89,11 @@ void __kernel_fpu_end(void)
 	if (use_eager_fpu()) {
 		/*
 		 * For eager fpu, most the time, tsk_used_math() is true.
-		 * Restore the user math as we are done with the kernel usage.
-		 * At few instances during thread exit, signal handling etc,
-		 * tsk_used_math() is false. Those few places will take proper
-		 * actions, so we don't need to restore the math here.

arch/x86/kernel/i387.c void __kernel_fpu_end(void) · line 96

Quoted 6 lines

arch/x86/kernel/i387.c +3 -1

+		 * Make sure the user math state is restored on return to
+		 * userspace.
 		 */
 		if (likely(tsk_used_math(current)))
-			math_state_restore();
+			set_thread_flag(TIF_LOAD_FPU);

Hmm. And this looks obviously wrong if interrupted_user_mode().

Currently the kernel will always load the FPU context, even when switching to a kernel thread, or to an idle thread. In the case of a task on a KVM VCPU going idle for a bit, and waking up again later, this creates a vastly inefficient chain of FPU context saves & loads:

interrupted_kernel_fpu_idle() tries to detect if kernel_fpu_begin() is safe or not. In particular it should obviously deny the nested kernel_fpu_begin() and this logic looks very confusing.

If use_eager_fpu() == T we rely on a) __thread_has_fpu() check in interrupted_kernel_fpu_idle(), and b) on the fact that _begin() does __thread_clear_has_fpu().

Otherwise we demand that the interrupted task has no FPU if it is in kernel mode, this works because __kernel_fpu_begin() does clts() and interrupted_kernel_fpu_idle() checks X86_CR0_TS.

Add the per-cpu "bool in_kernel_fpu" variable, and change this code to check/set/clear it. This allows to do more cleanups and fixes, see the next changes.

The patch also moves WARN_ON_ONCE() under preempt_disable() just to make this_cpu_read() look better, this is not really needed. And in fact I think we should move it into __kernel_fpu_begin().

Signed-off-by Oleg Nesterov <[email protected]>

Now that we have in_kernel_fpu we can remove __thread_clear_has_fpu() in __kernel_fpu_begin(). And this allows to replace the asymmetrical and nontrivial use_eager_fpu + tsk_used_math check in kernel_fpu_end() with the same __thread_has_fpu() check.

The logic becomes really simple; if _begin() does save() then _end() needs restore(), this is controlled by __thread_has_fpu(). Otherwise they do clts/stts unless use_eager_fpu().

Not only this makes begin/end symmetrical and imo more understandable, potentially this allows to change irq_fpu_usable() to avoid all other checks except "in_kernel_fpu".

Also, with this patch __kernel_fpu_end() does restore_fpu_checking() and WARNs if it fails instead of math_state_restore(). I think this looks better because we no longer need __thread_fpu_begin(), and it would be better to report the failure in this case.

Signed-off-by Oleg Nesterov <[email protected]> ---

arch/x86/kernel/i387.c | 19 ++++++------------- 1 files changed,
6 insertions(+), 13 deletions(-)

arch/x86/kernel/i387.c index renamed from arch/x86/kernel/i387.c +0 -0

a815723..12088a3 100644 --- a/arch/x86/kernel/i387.c +++ b/arch/x86/kernel/i387.c @@ -81,9 +81,7 @@ void __kernel_fpu_begin(void) this_cpu_write(in_kernel_fpu, true);

if (__thread_has_fpu(me)) { - __thread_clear_has_fpu(me);

__save_init_fpu(me); - /* We do 'stts()' in __kernel_fpu_end() */
} else if (!use_eager_fpu()) { this_cpu_write(fpu_owner_task, NULL); clts(); @@ -93,17 +91,12 @@ EXPORT_SYMBOL(__kernel_fpu_begin);

void __kernel_fpu_end(void) { - if (use_eager_fpu()) { - /* - *
For eager fpu, most the time, tsk_used_math() is true. - *
Restore the user math as we are done with the kernel usage. - *
At few instances during thread exit, signal handling etc, - *
tsk_used_math() is false. Those few places will take proper - *
actions, so we don't need to restore the math here. - */ - if
(likely(tsk_used_math(current))) - math_state_restore(); - } else
{ + struct task_struct *me = current; + + if (__thread_has_fpu(me))
{ + if (WARN_ON(restore_fpu_checking(me))) + drop_init_fpu(me);

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

Quoted 4 lines · Oleg Nesterov

b/arch/x86/kernel/i387.c @@ -81,9 +81,7 @@ void __kernel_fpu_begin(void) this_cpu_write(in_kernel_fpu, true);

if (__thread_has_fpu(me)) { - __thread_clear_has_fpu(me);

I will put that line back in the patch series that defers the loading of FPU state until the switch to user space,

but I
guess we can go either way for now...

Quoted 11 lines · Rik van Riel

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

Quoted 4 lines · Oleg Nesterov

b/arch/x86/kernel/i387.c @@ -81,9 +81,7 @@ void __kernel_fpu_begin(void) this_cpu_write(in_kernel_fpu, true);

if (__thread_has_fpu(me)) { - __thread_clear_has_fpu(me);

I will put that line back in the patch series that defers the loading of FPU state until the switch to user space,

I think this needs more discussion.

Firstly, __thread_clear_has_fpu() should go into __kernel_fpu_end(), it should replace restore_fpu_checking().

But does your series actually need this change? Correctness-wise this is not needed (afaics). Performance-wise I am not sure, kernel_fpu_begin() is unlikely event. Plus I am not sure this is a win (in general), but I can be easily wrong.

Quoted 1 line

but I guess we can go either way for now...

Yes, this should not really conflict with your changes in any case.

Given that you acked 1/3 and 3/3, perhaps you can ack this one as well?

math_state_restore() can race with kernel_fpu_begin() if irq comes right after __thread_fpu_begin(), __save_init_fpu() will overwrite fpu->state we are going to restore.

Add 2 simple helpers, kernel_fpu_disable() and kernel_fpu_enable() which simply set/clear in_kernel_fpu, and change math_state_restore() to exclude kernel_fpu_begin() in between.

Alternatively we could use local_irq_save/restore, but probably these new helpers can have more users.

Perhaps they should disable/enable preemption themselves, in this case we can remove preempt_disable() in __restore_xstate_sig().

Signed-off-by Oleg Nesterov <[email protected]>

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

Quoted 8 lines · Oleg Nesterov

math_state_restore() can race with kernel_fpu_begin() if irq comes right after __thread_fpu_begin(), __save_init_fpu() will overwrite fpu->state we are going to restore.

Add 2 simple helpers, kernel_fpu_disable() and kernel_fpu_enable() which simply set/clear in_kernel_fpu, and change math_state_restore() to exclude kernel_fpu_begin() in between.

Quoted 5 lines

Alternatively we could use local_irq_save/restore, but probably these new helpers can have more users.

Perhaps they should disable/enable preemption themselves, in this case we can remove preempt_disable() in __restore_xstate_sig().

Given that math_state_restore does an implicit preempt_disable through local_irq_disable,

I am not sure whether adding an
explicit preempt_disable would be good or bad.

Reviewed-by Rik van Riel <[email protected]>

Let me send initial kernel_fpu_begin/end cleanups, I believe they make sense anyway and won't conflict with your changes.

This is actually resend, I sent more patches some time ago but they were ignored.

Note that (I hope) we can do more changes on top of this series, in particular:

- remove all checks from irq_fpu_usable() except in_kernel_fpu

- do not abuse FPU in kernel threads, this makes sense even if use_eager_fpu(), and with or without the changes you proposed.

__kernel_fpu_begin() does nothing if !__thread_has_fpu() && use_eager_fpu(), perhaps it assumes that this case is simply impossible. This is certainly not possible if in_interrupt() == T; interrupted_user_mode() should have FPU, and interrupted_kernel_fpu_idle() should fail if !__thread_has_fpu().

However, even if use_eager_fpu() == T a task can do drop_fpu(), then switch to another thread which becomes fpu_owner_task, then resume and call some function which does kernel_fpu_begin(). Say, an exiting task does a lot of things after exit_thread(), it is not safe to assume that it can't use FPU in these paths.

Signed-off-by Oleg Nesterov <[email protected]>

__kernel_fpu_begin() does nothing if !__thread_has_fpu() && use_eager_fpu(), perhaps it assumes that this case is simply impossible. This is certainly not possible if in_interrupt() == T; interrupted_user_mode() should have FPU, and interrupted_kernel_fpu_idle() should fail if !__thread_has_fpu().

However, even if use_eager_fpu() == T a task can do drop_fpu(), then switch to another thread which becomes fpu_owner_task, then resume and call some function which does kernel_fpu_begin(). Say, an exiting task does a lot of things after exit_thread(), it is not safe to assume that it can't use FPU in these paths.

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/i387.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

arch/x86/kernel/i387.c +0 -0

index 81049ff..26f0e80 100644

The __thread_has_fpu() check in interrupted_kernel_fpu_idle() was needed to prevent the nested kernel_fpu_begin(). Now that we have in_kernel_fpu and !__thread_has_fpu() case in __kernel_fpu_begin() does not depend on use_eager_fpu() (except clts) we can remove it.

__thread_has_fpu() can be false even if use_eager_fpu(), but this case does not differ from !use_eager_fpu() case except we should not worry about X86_CR0_TS, __kernel_fpu_begin/end will not touch this bit.

Note: I think we can kill all irq_fpu_usable() checks except in_kernel_fpu, just we need to record the state of X86_CR0_TS in __kernel_fpu_begin() and conditionalize stts() in __kernel_fpu_end(), but this needs another patch.

Signed-off-by Oleg Nesterov <[email protected]>

The __thread_has_fpu() check in interrupted_kernel_fpu_idle() was needed to prevent the nested kernel_fpu_begin(). Now that we have in_kernel_fpu and !__thread_has_fpu() case in __kernel_fpu_begin() does not depend on use_eager_fpu() (except clts) we can remove it.

__thread_has_fpu() can be false even if use_eager_fpu(), but this case does not differ from !use_eager_fpu() case except we should not worry about X86_CR0_TS, __kernel_fpu_begin/end will not touch this bit.

Note: I think we can kill all irq_fpu_usable() checks except in_kernel_fpu, just we need to record the state of X86_CR0_TS in __kernel_fpu_begin() and conditionalize stts() in __kernel_fpu_end(), but this needs another patch.

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/i387.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

arch/x86/kernel/i387.c +3 -4

index 26f0e80..4734865 100644
@@ -41,8 +41,8 @@ void kernel_fpu_enable(void)
  * be set (so that the clts/stts pair does nothing that is
  * visible in the interrupted kernel thread).
  *
- * Except for the eagerfpu case when we return 1 unless we've already
- * been eager and saved the state in kernel_fpu_begin().
+ * Except for the eagerfpu case when we return true; in the likely case
+ * the thread has FPU but we are not going to set/clear TS.
  */
 static inline bool interrupted_kernel_fpu_idle(void)
 {

@@ -50,7 +50,7 @@ static inline bool interrupted_kernel_fpu_idle(void)
                return false;
 
        if (use_eager_fpu())
-               return __thread_has_fpu(current);
+               return true;
 
        return !__thread_has_fpu(current) &&
                (read_cr0() & X86_CR0_TS);
--

1.5.5.1

The __thread_has_fpu() check in interrupted_kernel_fpu_idle() was needed to prevent the nested kernel_fpu_begin(). Now that we have in_kernel_fpu and !__thread_has_fpu() case in __kernel_fpu_begin() does not depend on use_eager_fpu() (except clts) we can remove it.

__thread_has_fpu() can be false even if use_eager_fpu(), but this case does not differ from !use_eager_fpu() case except we should not worry about X86_CR0_TS, __kernel_fpu_begin/end will not touch this bit.

Note: I think we can kill all irq_fpu_usable() checks except in_kernel_fpu, just we need to record the state of X86_CR0_TS in __kernel_fpu_begin() and conditionalize stts() in __kernel_fpu_end(), but this needs another patch.

Signed-off-by Oleg Nesterov <[email protected]>

Afaics there is no reason why kernel threads should have FPU context even if use_eager_fpu() == T. Now that interrupted_kernel_fpu_idle() does not check __thread_has_fpu() we can remove the init_fpu() code from eager_fpu_init() and change flush_thread() called by do_execve() to initialize FPU.

Note: of course, the change in flush_thread() is horrible and must be cleanuped. We need the new helper, and flush_thread() should return the error if init_fpu() fails.

Signed-off-by Oleg Nesterov <[email protected]>

Afaics there is no reason why kernel threads should have FPU context even if use_eager_fpu() == T. Now that interrupted_kernel_fpu_idle() does not check __thread_has_fpu() we can remove the init_fpu() code

from eager_fpu_init() and change flush_thread() called by do_execve() to initialize FPU.

Note: of course, the change in flush_thread() is horrible and must be cleanuped. We need the new helper, and flush_thread() should return the error if init_fpu() fails.

Signed-off-by Oleg Nesterov <[email protected]>