x86,fpu: make FPU context switching much lazier

From: Rik van Riel <[email protected]>

Move all handling of the next state FPU state handling into switch_fpu_finish, in preparation for more lazily switching FPU states.

CR0.TS state is mirrored in a per-cpu variable, instead of being passed around in a local variable, because that will not be possible later in the series.

Quoted 11 lines · On Oct 1, 2016 1:49 PM, <[email protected]> wrote:

From: Rik van Riel <[email protected]>

Move all handling of the next state FPU state handling into switch_fpu_finish, in preparation for more lazily switching FPU states.

CR0.TS state is mirrored in a per-cpu variable, instead of being passed around in a local variable, because that will not be possible later in the series.

This seems reasonable in principle, but IMO it would be less scary if you rebased onto this:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=x8
6/fpu

From: Rik van Riel <[email protected]>

Delay the loading of FPU registers until a process switches back to userspace. This allows us to skip FPU saving & restoring for kernel threads, the idle task, and tasks that are spinning in kernel space.

It also allows us to not repeatedly save & restore the userspace FPU context on repeated invocations of kernel_fpu_start & kernel_fpu_end.

Not overwriting the FPU state of a task unless we need to also allows us to be be lazier about restoring it, in a future patch.

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

---
 arch/x86/entry/common.c             |  4 ++++
 arch/x86/include/asm/fpu/api.h      |  5 +++++
 arch/x86/include/asm/fpu/internal.h | 44 +++++++++----------------------------
 arch/x86/include/asm/thread_info.h  |  4 +++-
 arch/x86/kernel/fpu/core.c          | 17 ++++++++------
 arch/x86/kernel/process.c           | 35 +++++++++++++++++++++++++++++
 arch/x86/kernel/process_32.c        |  5 ++---
 arch/x86/kernel/process_64.c        |  5 ++---
 8 files changed, 71 insertions(+), 48 deletions(-)

arch/x86/entry/common.c +1 -0

index 1433f6b4607d..a69bbefa3408 100644
@@ -27,6 +27,7 @@
 #include <asm/vdso.h>
 #include <asm/uaccess.h>
 #include <asm/cpufeature.h>
+#include <asm/fpu/api.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>

arch/x86/entry/common.c +1 -18

+static inline void switch_fpu_finish(void)
 {
-       bool preload;
-       /*
-        * If the task has used the math, pre-load the FPU on xsave processors
-        * or if the past 5 consecutive context-switches used math.
-        */
-       preload = static_cpu_has(X86_FEATURE_FPU) &&
-                 new_fpu->fpstate_active &&
-                 (use_eager_fpu() || new_fpu->counter > 5);
-
-       if (preload) {
-               prefetch(&new_fpu->state);
-               new_fpu->counter++;
-               __fpregs_activate(new_fpu);
-               trace_x86_fpu_regs_activated(new_fpu);
-
-               /* Don't change CR0.TS if we just switch! */
-               if (!__this_cpu_read(fpu_active)) {
-                       __fpregs_activate_hw();

arch/x86/entry/common.c +16 -0

+/*
+ * Set up the userspace FPU context before returning to userspace.
+ */
+void switch_fpu_return(void)
+{
+       struct fpu *fpu = &current->thread.fpu;
+       bool preload;
+       /*
+        * If the task has used the math, pre-load the FPU on xsave processors
+        * or if the past 5 consecutive context-switches used math.
+        */
+       preload = static_cpu_has(X86_FEATURE_FPU) &&
+                 fpu->fpstate_active &&
+                 (use_eager_fpu() || fpu->counter > 5);
+
+       if (preload) {

arch/x86/entry/common.c __visible inline void · line 204

Quoted 11 lines · On Sat, Oct 1, 2016 at 1:31 PM,  <[email protected]> wrote:

 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>

arch/x86/entry/common.c +3 -0

@@ -197,6 +198,9 @@ __visible inline void
 prepare_exit_to_usermode(struct pt_regs *regs)
         if (unlikely(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
                 exit_to_usermode_loop(regs, cached_flags);
 
+       if (unlikely(test_and_clear_thread_flag(TIF_LOAD_FPU)))
+               switch_fpu_return();
+

How about:

if (unlikely(...)) {
  exit_to_usermode_loop(regs, cached_flags);
  cached_flags = READ_ONCE(ti->flags);
}

if (ti->flags & _TIF_LOAD_FPU) {
  clear_thread_flag(TIF_LOAD_FPU);
  switch_fpu_return();
}

Quoted 4 lines

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

+static inline void switch_fpu_finish(void)
  {
+       set_thread_flag(TIF_LOAD_FPU);

 }

I can imagine this causing problems with kernel code that accesses current's FPU state, e.g. get_xsave_field_ptr().  I wonder if it would
make sense to make your changes deeper into the FPU core innards so that, for example, we'd have explicit functions that cause the

in-memory state for current to be up-to-date and readable, to cause the in-memory state to be up-to-date and writable (which is the same thing + TIF_FPU_LOAD + whatever other bookkeeping), and causing the in-CPU state to be up-to-date (possibly readable and writable). TIF_LOAD_FPU would trigger the latter.

I've often found it confusing that fpu__save by itself has somewhat ill-defined effects.

Quoted 23 lines

arch/x86/kernel/process.c +9 -0

+/*
+ * Set up the userspace FPU context before returning to userspace.
+ */
+void switch_fpu_return(void)
+{
+       struct fpu *fpu = &current->thread.fpu;
+       bool preload;
+       /*
+        * If the task has used the math, pre-load the FPU on xsave

processors
+        * or if the past 5 consecutive context-switches used math. +        */
+       preload = static_cpu_has(X86_FEATURE_FPU) &&
+                 fpu->fpstate_active &&
+                 (use_eager_fpu() || fpu->counter > 5); +
+       if (preload) {
+               prefetch(&fpu->state);
+               fpu->counter++;
+               __fpregs_activate(fpu);
+               trace_x86_fpu_regs_activated(fpu);

Quoted 6 lines

+
+               /* Don't change CR0.TS if we just switch! */ +               if (!__this_cpu_read(fpu_active)) { +                       __fpregs_activate_hw();
+                       __this_cpu_write(fpu_active, true); +               }

We should just finish getting rid of all TS uses.

Quoted 20 lines · Andy Lutomirski

arch/x86/entry/common.c __visible inline void · line 204

Quoted 11 lines · On Sat, Oct 1, 2016 at 1:31 PM, <[email protected]> wrote:

#define CREATE_TRACE_POINTS
#include <trace/events/syscalls.h>

arch/x86/entry/common.c +3 -0

@@ -197,6 +198,9 @@ __visible inline void
 prepare_exit_to_usermode(struct pt_regs *regs)
        if (unlikely(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
                exit_to_usermode_loop(regs, cached_flags);
 
+       if (unlikely(test_and_clear_thread_flag(TIF_LOAD_FPU)))
+               switch_fpu_return();
+

How about:

if (unlikely(...)) {
exit_to_usermode_loop(regs, cached_flags);
cached_flags = READ_ONCE(ti->flags);
}

Quoted 4 lines

if (ti->flags & _TIF_LOAD_FPU) {
clear_thread_flag(TIF_LOAD_FPU);
switch_fpu_return();
}

It looks like test_thread_flag should be fine for avoiding atomics, too?

  if (unlikely(test_thread_flag(TIF_LOAD_FPU))) {
      clear_thread_flag(TIF_LOAD_FPU);
      switch_fpu_return();
  }

Quoted 7 lines

arch/x86/include/asm/fpu/internal.h switch_fpu_prepare(struct fpu *old_fpu, int cpu) · line 616

Quoted 6 lines

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

+static inline void switch_fpu_finish(void)
 {
+       set_thread_flag(TIF_LOAD_FPU);
 }

Quoted 5 lines

I can imagine this causing problems with kernel code that accesses current's FPU state, e.g. get_xsave_field_ptr(). I wonder if it would
make sense to make your changes deeper into the FPU core innards so that, for example, we'd have explicit functions that cause the

Whereabouts do you have in mind?

I like your general idea, but am not sure quite where to put it.

Quoted 8 lines

in-memory state for current to be up-to-date and readable, to cause the in-memory state to be up-to-date and writable (which is the same thing + TIF_FPU_LOAD + whatever other bookkeeping), and causing the in-CPU state to be up-to-date (possibly readable and writable). TIF_LOAD_FPU would trigger the latter.

I've often found it confusing that fpu__save by itself has somewhat ill-defined effects.

Fully agreed on both. I guess that means we want a few different functions:

1) initialize FPU state, both in memory and in registers

2) cause FPU state in registers to be updated from memory, if necessary

3) cause FPU state in memory to be updated from registers, if necessary

The latter functions could use fpu->fpregs_active to see whether any action is required, and be called from various places in the code.

The signal code in particular is doing some strange things that should probably be hidden away in FPU specific functions.

Quoted 23 lines

Quoted 23 lines

arch/x86/kernel/process.c +9 -0

+/*
+ * Set up the userspace FPU context before returning to userspace.
+ */
+void switch_fpu_return(void)
+{
+       struct fpu *fpu = &current->thread.fpu;
+       bool preload;
+       /*
+        * If the task has used the math, pre-load the FPU on xsave

processors

arch/x86/kernel/process.c +11 -0

+        * or if the past 5 consecutive context-switches used math.
+        */
+       preload = static_cpu_has(X86_FEATURE_FPU) &&
+                 fpu->fpstate_active &&
+                 (use_eager_fpu() || fpu->counter > 5);
+
+       if (preload) {
+               prefetch(&fpu->state);
+               fpu->counter++;
+               __fpregs_activate(fpu);
+               trace_x86_fpu_regs_activated(fpu);

Quoted 8 lines

arch/x86/kernel/process.c int set_tsc_mode(unsigned int val) · line 220

Quoted 6 lines

arch/x86/kernel/process.c +6 -0

+
+               /* Don't change CR0.TS if we just switch! */
+               if (!__this_cpu_read(fpu_active)) {
+                       __fpregs_activate_hw();
+                       __this_cpu_write(fpu_active, true);
+               }

We should just finish getting rid of all TS uses.

Agreed. I will rebase on top of your FPU changes, that will make things easier for everybody.

--
All Rights Reversed.

Quoted 28 lines · Rik van Riel

Quoted 26 lines · Andy Lutomirski

Quoted 10 lines · On Sat, Oct 1, 2016 at 1:31 PM,  <[email protected]> wrote:

 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>

(quoted diff) +0 -0

@@ -197,6 +198,9 @@ __visible inline void
 prepare_exit_to_usermode(struct pt_regs *regs)

        if (unlikely(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS)) exit_to_usermode_loop(regs, cached_flags);

Quoted 3 lines

+       if (unlikely(test_and_clear_thread_flag(TIF_LOAD_FPU))) +               switch_fpu_return();
+

How about:

if (unlikely(...)) {
  exit_to_usermode_loop(regs, cached_flags);
  cached_flags = READ_ONCE(ti->flags);
}

if (ti->flags & _TIF_LOAD_FPU) {
  clear_thread_flag(TIF_LOAD_FPU);
  switch_fpu_return();
}

Quoted 7 lines

It looks like test_thread_flag should be fine for avoiding atomics, too?

  if (unlikely(test_thread_flag(TIF_LOAD_FPU))) {
      clear_thread_flag(TIF_LOAD_FPU);
      switch_fpu_return();
  }

True, but we've got that cached_flags variable already...

One of my objections to the current code structure is that I find it quite hard to keep track of all of the possible states of the fpu that
we have.  Off the top of my head, we have:

1. In-cpu state is valid and in-memory state is invalid.  (This is the
case when we're running user code, for example.)
2. In-memory state is valid.
2a. In-cpu state is *also* valid for some particular task.  In this state, no one may write to either FPU state copy for that task lest they get out of sync.
2b. In-cpu state is not valid.

Rik, your patches further complicate this by making it possible to have the in-cpu state be valid for a non-current task even when non-idle.

 This is why I'm thinking that we should maybe revisit the API a bit to make this clearer and to avoid scattering around all the state manipulation quite so much.  I propose, as a straw-man:

user_fpregs_copy_to_cpu() -- Makes the CPU state be valid for current.
After calling this, we're in state 1 or 2a.

user_fpregs_move_to_cpu() -- Makes the CPU state be valid and writable
for current.  Invalidates any in-memory copy.  After calling this, we're in state 1.

user_fpregs_copy_to_memory() -- Makes the in-memory state be valid for
current.  After calling this, we're in state 2a or 2b.

user_fpregs_move_to_memory() -- Makes the in-memory state be valid and
writable for current.  After calling this, we're in state 2b.

Scheduling out calls user_fpregs_copy_to_memory(), as do all the MPX and PKRU memory state readers.  MPX and PKRU will do user_fpregs_move_to_memory() before writing to memory. kernel_fpu_begin() does user_fpregs_move_to_memory().

There are a couple ways we could deal with TIF_LOAD_FPU in this scheme.  My instinct would be to rename it TIF_REFRESH_FPU, set it unconditionally on schedule-in *and* on user_fpregs_xxx_to_memory(), and make the handler do user_fpregs_move_to_cpu().

Quoted 15 lines · Andy Lutomirski

Quoted 14 lines · Rik van Riel

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · On Sat, Oct 1, 2016 at 1:31 PM, <[email protected]> wrote:

#define CREATE_TRACE_POINTS
#include <trace/events/syscalls.h>

(quoted diff) +0 -0

@@ -197,6 +198,9 @@ __visible inline void
 prepare_exit_to_usermode(struct pt_regs *regs)
        if (unlikely(cached_flags &
 EXIT_TO_USERMODE_LOOP_FLAGS))
                exit_to_usermode_loop(regs, cached_flags);
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/entry/common.c __visible inline void prepare_exit_to_usermode(struct pt_regs *regs) · line 203

Quoted 3 lines

arch/x86/entry/common.c +3 -0

+       if (unlikely(test_and_clear_thread_flag(TIF_LOAD_FPU)))
+               switch_fpu_return();
+

How about:

if (unlikely(...)) {
exit_to_usermode_loop(regs, cached_flags);
cached_flags = READ_ONCE(ti->flags);

Quoted 13 lines

Quoted 13 lines

Quoted 6 lines

}

if (ti->flags & _TIF_LOAD_FPU) {
clear_thread_flag(TIF_LOAD_FPU);
switch_fpu_return();
}

It looks like test_thread_flag should be fine for avoiding atomics, too?

  if (unlikely(test_thread_flag(TIF_LOAD_FPU))) {
      clear_thread_flag(TIF_LOAD_FPU);
      switch_fpu_return();
  }

Quoted 1 line

True, but we've got that cached_flags variable already...

The cached flag may no longer be valid after exit_to_usermode_loop() returns, because that code is preemptible.

We have to reload a fresh ti->flags after
preemption is disabled and irqs are off.

Quoted 6 lines

One of my objections to the current code structure is that I find it quite hard to keep track of all of the possible states of the fpu that
we have. Off the top of my head, we have:

Quoted 8 lines

1. In-cpu state is valid and in-memory state is invalid. (This is the
case when we're running user code, for example.)
2. In-memory state is valid.
2a. In-cpu state is *also* valid for some particular task. In this state, no one may write to either FPU state copy for that task lest they get out of sync.
2b. In-cpu state is not valid.

It gets more fun with ptrace. Look at xfpregs_get and xfpregs_set, which call fpu__activate_fpstate_read and fpu__activate_fpstate_write respectively.

It looks like those functions already deal with the cases you outline above.

Quoted 4 lines

Rik, your patches further complicate this by making it possible to have the in-cpu state be valid for a non-current task even when non-idle.

However, we never need to examine the in-cpu state for a task that is not currently running, or being scheduled to in the context switch code.

We always save the FPU register state when a task is context switched out, so for a non-current task, we still only need to consider whether the in-memory state is valid or not.

The in-cpu state does not matter.

fpu__activate_fpstate_write will disable lazy restore, and ensure a full restore from memory.

Quoted 5 lines

This is why I'm thinking that we should maybe revisit the
API a bit to make this clearer and to avoid scattering around all the state manipulation quite so much. I propose, as a straw-man:

Quoted 8 lines

user_fpregs_copy_to_cpu() -- Makes the CPU state be valid for current.
After calling this, we're in state 1 or 2a.

user_fpregs_move_to_cpu() -- Makes the CPU state be valid and writable
for current. Invalidates any in-memory copy. After calling this, we're in state 1.

How can we distinguish between these two?

If the task is running, it can change its FPU registers at any time, without any obvious thing to mark the transition from state 2a to state 1.

It sounds like the first function you name would only be useful if we prevented a switch to user space, which could immediately do whatever it wants with the registers.

Is there a use case for user_fpregs_copy_to_cpu()?

Quoted 8 lines

user_fpregs_copy_to_memory() -- Makes the in-memory state be valid for
current. After calling this, we're in state 2a or 2b.

user_fpregs_move_to_memory() -- Makes the in-memory state be valid and
writable for current. After calling this, we're in state 2b.

We can only ensure that the in memory copy stays valid, by stopping the task from writing to the in-CPU copy.

This sounds a lot like fpu__activate_fpstate_read

What we need after copying the contents to memory is a way to invalidate the in-CPU copy, if changes were made to the in-memory copy.

This is already done by fpu__activate_fpstate_write

Quoted 4 lines

Scheduling out calls user_fpregs_copy_to_memory(), as do all the MPX and PKRU memory state readers. MPX and PKRU will do user_fpregs_move_to_memory() before writing to memory. kernel_fpu_begin() does user_fpregs_move_to_memory().

This is done by switch_fpu_prepare.

Can you think of any other place in the kernel where we need a 1 -> 2a transition, besides context switching and ptrace/xfpregs_get?

Quoted 5 lines

There are a couple ways we could deal with TIF_LOAD_FPU in this scheme. My instinct would be to rename it TIF_REFRESH_FPU, set it unconditionally on schedule-in *and* on user_fpregs_xxx_to_memory(), and make the handler do user_fpregs_move_to_cpu().

I can see where you are coming from, but given that there is no way to run a task besides context switching to it, I am not sure why we would need to do anything besides disabling the lazy restore (indicating that the in-CPU state is stale) anywhere else?

If user_fpregs_move_to_cpu determines that the in-register state is still valid, it can skip the restore.

This is what switch_fpu_finish does after my first patch, and switch_fpu_prepare later in my series.

Quoted 24 lines · Rik van Riel

Quoted 7 lines

One of my objections to the current code structure is that I find it
quite hard to keep track of all of the possible states of the fpu that
we have.  Off the top of my head, we have:

Quoted 11 lines

1. In-cpu state is valid and in-memory state is invalid.  (This is
the
case when we're running user code, for example.)
2. In-memory state is valid.
2a. In-cpu state is *also* valid for some particular task.  In this
state, no one may write to either FPU state copy for that task lest
they get out of sync.
2b. In-cpu state is not valid.

It gets more fun with ptrace. Look at xfpregs_get and xfpregs_set, which call fpu__activate_fpstate_read and fpu__activate_fpstate_write respectively.

Quoted 2 lines

It looks like those functions already deal with the cases you outline above.

Hmm, maybe they could be used.  The names are IMO atrocious.  I had to
read the docs and the code to figure out WTF "activating" the "fpstate" even meant.

Quoted 17 lines

Quoted 5 lines

Rik, your patches further complicate this by making it possible to
have the in-cpu state be valid for a non-current task even when non-idle.

However, we never need to examine the in-cpu state for a task that is not currently running, or being scheduled to in the context switch code.

We always save the FPU register state when a task is context switched out, so for a non-current task, we still only need to consider whether the in-memory state is valid or not.

The in-cpu state does not matter.

Quoted 2 lines

fpu__activate_fpstate_write will disable lazy restore, and ensure a full restore from memory.

Something has to fix up owner_ctx so that the previous thread doesn't think its CPU state is still valid.  I don't think activate_fpstate_write does it, because nothing should (I think) be calling it as a mattor of course on context switch.

Quoted 15 lines

Quoted 9 lines

 This is why I'm thinking that we should maybe revisit the API a bit to make this clearer and to avoid scattering around all the
state manipulation quite so much.  I propose, as a straw-man:

user_fpregs_copy_to_cpu() -- Makes the CPU state be valid for current.
After calling this, we're in state 1 or 2a.

Quoted 5 lines

user_fpregs_move_to_cpu() -- Makes the CPU state be valid and writable
for current.  Invalidates any in-memory copy.  After calling this,
we're in state 1.

Quoted 5 lines

How can we distinguish between these two?

If the task is running, it can change its FPU registers at any time, without any obvious thing to mark the transition from state 2a to state 1.

That's true now, but I don't think it's really true any more with your
patches.  If a task is running in *kernel* mode, then I think that pretty much all of the combinations are possible.  When the task actually enters user mode, we have to make sure we're in state 1 so the CPU regs belong to the task and are writable.

Quoted 6 lines

It sounds like the first function you name would only be useful if we prevented a switch to user space, which could immediately do whatever it wants with the registers.

Is there a use case for user_fpregs_copy_to_cpu()?

With PKRU, any user memory access would need user_fpregs_copy_to_cpu().  I think we should get rid of this and just
switch to using RDPKRU and WRPKRU to eagerly update PKRU in every context switch to avoid this.  Other than that, I can't think of any use cases off the top of my head unless the signal code wanted to play
games with this stuff.

Quoted 14 lines

Quoted 9 lines

user_fpregs_copy_to_memory() -- Makes the in-memory state be valid
for
current.  After calling this, we're in state 2a or 2b.

user_fpregs_move_to_memory() -- Makes the in-memory state be valid
and
writable for current.  After calling this, we're in state 2b.

We can only ensure that the in memory copy stays valid, by stopping the task from writing to the in-CPU copy.

Quoted 5 lines

This sounds a lot like fpu__activate_fpstate_read

What we need after copying the contents to memory is a way to invalidate the in-CPU copy, if changes were made to the in-memory copy.

My proposal is that we invalidate the CPU state *before* writing the in-memory state, not after.

Quoted 9 lines

This is already done by fpu__activate_fpstate_write

Quoted 6 lines

Scheduling out calls user_fpregs_copy_to_memory(), as do all the MPX
and PKRU memory state readers.  MPX and PKRU will do user_fpregs_move_to_memory() before writing to memory. kernel_fpu_begin() does user_fpregs_move_to_memory().

Quoted 5 lines

This is done by switch_fpu_prepare.

Can you think of any other place in the kernel where we need a 1 -> 2a transition, besides context switching and ptrace/xfpregs_get?

Anything else that tries to read task xstate from memory, i.e. MPX and
PKRU.  (Although if we switch to eager-switched PKRU, then PKRU stops mattering for this purpose.)

Actually, I don't see any way your patches can be compatible with PKRU
without switching to eager-switched PKRU.

Quoted 25 lines · Andy Lutomirski

Quoted 24 lines · Rik van Riel

Quoted 8 lines

One of my objections to the current code structure is that I find it
quite hard to keep track of all of the possible states of the fpu that
we have. Off the top of my head, we have:

Quoted 11 lines

1. In-cpu state is valid and in-memory state is invalid. (This is
the
case when we're running user code, for example.)
2. In-memory state is valid.
2a. In-cpu state is *also* valid for some particular task. In this
state, no one may write to either FPU state copy for that task lest
they get out of sync.
2b. In-cpu state is not valid.

It gets more fun with ptrace. Look at xfpregs_get and xfpregs_set, which call fpu__activate_fpstate_read and fpu__activate_fpstate_write respectively.

Quoted 7 lines

Quoted 2 lines

It looks like those functions already deal with the cases you outline above.

Hmm, maybe they could be used. The names are IMO atrocious. I had to
read the docs and the code to figure out WTF "activating" the "fpstate" even meant.

If you have good naming ideas, I'd be willing to submit patches to rename them :)

Quoted 17 lines

Quoted 17 lines

Quoted 5 lines

Rik, your patches further complicate this by making it possible to
have the in-cpu state be valid for a non-current task even when non-idle.

However, we never need to examine the in-cpu state for a task that is not currently running, or being scheduled to in the context switch code.

We always save the FPU register state when a task is context switched out, so for a non-current task, we still only need to consider whether the in-memory state is valid or not.

The in-cpu state does not matter.

Quoted 7 lines

Quoted 2 lines

fpu__activate_fpstate_write will disable lazy restore, and ensure a full restore from memory.

Something has to fix up owner_ctx so that the previous thread doesn't think its CPU state is still valid. I don't think activate_fpstate_write does it, because nothing should (I think) be calling it as a mattor of course on context switch.

These functions are called on tasks that are ptraced, and currently stopped. fpu__activate_fpstate_write sets fpu->last_cpu to -1, which will cause the next context switch to the task to load the fpstate into the registers.

The status in the struct fpu keeps track of the status of things, though maybe not as explicitly as you would want.

Having two separate status booleans for "registers valid" and "memory valid" may make more sense.

Running the task in user space, would have to ensure "registers valid" is true, and make "memory valid" false, because userspace could write to the registers.

Doing a ptrace fpstate_read would make "memory valid" true, but does not need to invalidate "registers valid".

Doing a ptrace fpstate_write would make "memory valid" true, but would invalidate "registers valid".

Context switching away from a task would make "memory valid" true, by virtue of copying the fpstate to memory.

Ingo, would you have any objection to patches tracking the validity of both register and memory states independently?

We can get rid of fpu.counter, since nobody uses it any more.

Quoted 22 lines

Quoted 16 lines

Quoted 10 lines

This is why I'm thinking that we should maybe revisit the
API a bit to make this clearer and to avoid scattering around all the
state manipulation quite so much. I propose, as a straw-man:

user_fpregs_copy_to_cpu() -- Makes the CPU state be valid for current.
After calling this, we're in state 1 or 2a.

Quoted 5 lines

user_fpregs_move_to_cpu() -- Makes the CPU state be valid and writable
for current. Invalidates any in-memory copy. After calling this,
we're in state 1.

Quoted 5 lines

How can we distinguish between these two?

If the task is running, it can change its FPU registers at any time, without any obvious thing to mark the transition from state 2a to state 1.

Quoted 6 lines

That's true now, but I don't think it's really true any more with your
patches. If a task is running in *kernel* mode, then I think that pretty much all of the combinations are possible. When the task actually enters user mode, we have to make sure we're in state 1 so the CPU regs belong to the task and are writable.

That is fair.

Quoted 7 lines

Quoted 6 lines

It sounds like the first function you name would only be useful if we prevented a switch to user space, which could immediately do whatever it wants with the registers.

Is there a use case for user_fpregs_copy_to_cpu()?

Quoted 8 lines

With PKRU, any user memory access would need user_fpregs_copy_to_cpu(). I think we should get rid of this and just
switch to using RDPKRU and WRPKRU to eagerly update PKRU in every context switch to avoid this. Other than that, I can't think of any use cases off the top of my head unless the signal code wanted to play
games with this stuff.

You are right, read_pkru() and write_pkru() can only deal with the pkru state being present in registers. Is this because of an assumption in the code, or because of a hardware requirement?

Quoted 14 lines

Quoted 14 lines

Quoted 9 lines

user_fpregs_copy_to_memory() -- Makes the in-memory state be valid
for
current. After calling this, we're in state 2a or 2b.

user_fpregs_move_to_memory() -- Makes the in-memory state be valid
and
writable for current. After calling this, we're in state 2b.

We can only ensure that the in memory copy stays valid, by stopping the task from writing to the in-CPU copy.

Quoted 8 lines

Quoted 5 lines

This sounds a lot like fpu__activate_fpstate_read

What we need after copying the contents to memory is a way to invalidate the in-CPU copy, if changes were made to the in-memory copy.

My proposal is that we invalidate the CPU state *before* writing the in-memory state, not after.

Modifying the in-memory FPU state can only be done on a task that is not currently running in user space. As long as the modification is done before task runs again, that is safe.

I am probably missing some special case you are concerned about.

Quoted 15 lines

Quoted 9 lines

This is already done by fpu__activate_fpstate_write

Quoted 6 lines

Scheduling out calls user_fpregs_copy_to_memory(), as do all the MPX
and PKRU memory state readers. MPX and PKRU will do user_fpregs_move_to_memory() before writing to memory. kernel_fpu_begin() does user_fpregs_move_to_memory().

Quoted 5 lines

This is done by switch_fpu_prepare.

Can you think of any other place in the kernel where we need a 1 -> 2a transition, besides context switching and ptrace/xfpregs_get?

Quoted 8 lines

Anything else that tries to read task xstate from memory, i.e. MPX and
PKRU. (Although if we switch to eager-switched PKRU, then PKRU stops mattering for this purpose.)

Actually, I don't see any way your patches can be compatible with PKRU
without switching to eager-switched PKRU.

My patches preserve eager saving of the FPU state, they only make restore lazy. That is 1 -> 2a transitions continue to be eager.

--
All Rights Reversed.

Quoted 2 lines

Having two separate status booleans for "registers valid" and "memory valid" may make more sense.

I have no problem with the concept of "owner_ctx", and I think it's a perfectly reasonable data structure.  My problem with it is that it's subtle and knowledge of it is spread all over the place.  Just going with "registers valid" in a variable won't work, I think, because there's nowhere to put it.  We need to be able to delete a struct fpu while that struct fpu might have a valid copy in a different cpu's registers.

Anyway, feel free to tell me that I'm making this too difficult :)

Quoted 2 lines

We can get rid of fpu.counter, since nobody uses it any more.

We should definitely do this.

Maybe getting in some cleanups first (my lazy fpu deletion, fpu.counter removal, etc) first is the way to go.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

You are right, read_pkru() and write_pkru() can only deal with the pkru state being present in registers. Is this because of an assumption in the code, or because of a hardware requirement?

Quoted 11 lines · Andy Lutomirski

Quoted 2 lines

Having two separate status booleans for "registers valid" and "memory valid" may make more sense.

I have no problem with the concept of "owner_ctx", and I think it's a perfectly reasonable data structure. My problem with it is that it's subtle and knowledge of it is spread all over the place. Just going with "registers valid" in a variable won't work, I think, because there's nowhere to put it. We need to be able to delete a struct fpu while that struct fpu might have a valid copy in a different cpu's registers.

Quoted 1 line

Anyway, feel free to tell me that I'm making this too difficult :)

How about we rename fpu_want_lazy_restore to
fpu_registers_valid()? Problem solved :)

Then we can rename __cpu_disable_lazy_restore
to fpu_invalidate_registers(), and call that
before we modify any in-memory FPU state.

Quoted 6 lines

Quoted 3 lines

We can get rid of fpu.counter, since nobody uses it any more.

We should definitely do this.

Quoted 2 lines

Maybe getting in some cleanups first (my lazy fpu deletion, fpu.counter removal, etc) first is the way to go.

Sounds good. I will keep my patch 1/4 as part of the cleanup series, and will not move on to the harder stuff until after the cleanups.

Any other stuff I should clean up while we're there?

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

You are right, read_pkru() and write_pkru() can only deal with the pkru state being present in registers. Is this because of an assumption in the code, or because of a hardware requirement?

read_pkru and write_pkru would be candidates for using fpu_registers_valid, and potentially a fpu_make_registers_valid, which restores the contents of the fpu registers from memory, if fpu_registers_valid is not true.

Likewise, we can have an fpu_make_memory_valid to ensure the in kernel memory copy of the FPU registers is valid, potentially a _read and _write version that do exactly what the pstate code wants today.

Would that make sense as an API?

--
All Rights Reversed.

Quoted 10 lines

Running the task in user space, would have to ensure "registers valid" is true, and make "memory valid" false, because userspace could write to the registers.

Doing a ptrace fpstate_read would make "memory valid" true, but does not need to invalidate "registers valid".

Doing a ptrace fpstate_write would make "memory valid" true, but would invalidate "registers valid".

Quoted 7 lines

Context switching away from a task would make "memory valid" true, by virtue of copying the fpstate to memory.

Ingo, would you have any objection to patches tracking the validity of both register and memory states independently?

Quoted 3 lines

We can get rid of fpu.counter, since nobody uses it any more.

We should definitely do this.

Maybe getting in some cleanups first (my lazy fpu deletion, fpu.counter removal, etc) first is the way to go.

* Andy Lutomirski <[email protected]> wrote:

Quoted 7 lines

Quoted 2 lines

We can get rid of fpu.counter, since nobody uses it any more.

We should definitely do this.

Maybe getting in some cleanups first (my lazy fpu deletion, fpu.counter removal, etc) first is the way to go.

Could you guys please submit all pending FPU bits that aren't new functionality to -tip? I'll remove lazy FPU handling if you don't beat me to it.

Plus after lazy FPU removal we should take another good look and make the FPU state machine even more obvious and self-documenting.

Upstream merge commit 597f03f9d133 would be a good base.

Anything else that tries to read task xstate from memory, i.e. MPX and
PKRU.  (Although if we switch to eager-switched PKRU, then PKRU stops mattering for this purpose.)

Actually, I don't see any way your patches can be compatible with PKRU
without switching to eager-switched PKRU.

Quoted 9 lines · Andy Lutomirski

Anything else that tries to read task xstate from memory, i.e. MPX and
PKRU. (Although if we switch to eager-switched PKRU, then PKRU stops mattering for this purpose.)

Actually, I don't see any way your patches can be compatible with PKRU
without switching to eager-switched PKRU.

There is one case where the in-register PKRU state matters:
- user space accesses to memory

There are several cases where the in-memory PKRU state would suffice:
- get_user_pages(_fast) to the local task (could also use registers)
- setting VMA/PTE permission bits (could also use registers)

There is one case where only in-memory PKRU state works, where PKRU is currently simply ignored:
- get_user_pages to another task's memory

Dave, are there major obstacles to making read_pkru and write_pkru work with in-memory state?

Would it be better for read/write_pkru to force the FPU state to get loaded into registers, under the assumption that if things like get_user_pages_fast happens, we will likely switch back to userspace soon, anyway?

Would that assumption be wrong with KVM? :)

--
All Rights Reversed.

Quoted 6 lines · On Sat, Oct 1, 2016 at 1:31 PM,  <[email protected]> wrote:

+static inline void switch_fpu_finish(void)
 {
+       set_thread_flag(TIF_LOAD_FPU);
 }

I can imagine this causing problems with kernel code that accesses current's FPU state, e.g. get_xsave_field_ptr().

Quoted 8 lines · Andy Lutomirski

Quoted 8 lines · On Sat, Oct 1, 2016 at 1:31 PM, <[email protected]> wrote:

Quoted 4 lines

arch/x86/include/asm/fpu/internal.h switch_fpu_prepare(struct fpu *old_fpu, int cpu) · line 616

Quoted 4 lines

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

+static inline void switch_fpu_finish(void)
 {
+       set_thread_flag(TIF_LOAD_FPU);
 }

I can imagine this causing problems with kernel code that accesses current's FPU state, e.g. get_xsave_field_ptr().

That makes me wonder, what test programs do people have to verify the correctness of the FPU switching code?

Quoted 11 lines · On 10/01/2016 01:31 PM, [email protected] wrote:

 /*
+ * Check whether an FPU's register set is still loaded in the CPU.
+ */

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

+static inline bool fpu_lazy_skip_restore(struct fpu *fpu)
+{
+	bool still_loaded = (fpu->fpstate_active &&
+			     fpu->last_cpu ==

raw_smp_processor_id() &&

(quoted diff) +1 -0

+			     __this_cpu_read(fpu_fpregs_owner_ctx)

== fpu);

arch/x86/include/asm/fpu/internal.h static inline void fpregs_deactivate(struct fpu *fpu) · line 588

Quoted 4 lines

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

+
+	fpu->fpregs_active = still_loaded;
+	return still_loaded;
+}

I wonder if we should call this something more along the lines of fpregs_activate_fast(), which returns if it managed to do the activation
fast or not.  I _think_ that's more along the lines of what it is actually doing.  The fact that it can be lazy is really an implementation detail.

What are the preempt rules with this thing?  This needs to be called in
preempt-disabled contexts, right?

Quoted 2 lines

Quoted 2 lines

What are the preempt rules with this thing? This needs to be called in preempt-disabled contexts, right?

Indeed, all the FPU context switching code needs
to be called in preempt-disabled contexts.

You do not want to get preempted halfway through
saving or restoring floating point registers.

Quoted 9 lines · Rik van Riel

Quoted 6 lines

Quoted 6 lines

What are the preempt rules with this thing?  This needs to be called
in preempt-disabled contexts, right?

Indeed, all the FPU context switching code needs
to be called in preempt-disabled contexts.

Quoted 2 lines

You do not want to get preempted halfway through
saving or restoring floating point registers.

OK, cool, that's what I expected.  Could you just add a comment about it
to make it clear that it's also the case for this new fpu_lazy_skip_restore() helper?