Re: [PATCH 08/22] x86/fpu: Remove user_fpu_begin()

[Borislav Petkov]

On Wed, Jan 09, 2019 at 12:47:30PM +0100, Sebastian Andrzej Siewior wrote:
> user_fpu_begin() sets fpu_fpregs_owner_ctx to task's fpu struct. This is
> always the case since there is no lazy FPU anymore.
> 
> fpu_fpregs_owner_ctx is used during context switch to decide if it needs
> to load the saved registers or if the currently loaded registers are
> valid. It could be skipped during
> 	taskA -> kernel thread -> taskA
> 
> because the switch to kernel thread would not alter the CPU's FPU state.
> 
> Since this field is always updated during context switch and never
> invalidated, setting it manually (in user context) makes no difference.
> A kernel thread with kernel_fpu_begin() block could set
> fpu_fpregs_owner_ctx to NULL but a kernel thread does not use
> user_fpu_begin().
> This is a leftover from the lazy-FPU time.
> 
> Remove user_fpu_begin(), it does not change fpu_fpregs_owner_ctx's
> content.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/fpu/internal.h | 17 -----------------
>  arch/x86/kernel/fpu/core.c          |  4 +---
>  arch/x86/kernel/fpu/signal.c        |  1 -
>  3 files changed, 1 insertion(+), 21 deletions(-)

Reviewed-by: Borislav Petkov <bp@xxxxxxx>

Should we do this microoptimization in addition, to save us the
activation when the kernel thread here:

	taskA -> kernel thread -> taskA

doesn't call kernel_fpu_begin() and thus fpu_fpregs_owner_ctx remains
the same?

It would be a bit more correct as it won't invoke the
trace_x86_fpu_regs_activated() TP in case the FPU context is the same.

---
diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index bfe0bfc7d0d1..ee1ac46a7820 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -510,7 +510,7 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
  * Set up the userspace FPU context for the new task, if the task
  * has used the FPU.
  */
-static inline void switch_fpu_finish(struct fpu *new_fpu, int cpu)
+static inline void switch_fpu_finish(struct fpu *prev_fpu, struct fpu *new_fpu, int cpu)
 {
 	if (static_cpu_has(X86_FEATURE_FPU)) {
 		if (!fpregs_state_valid(new_fpu, cpu)) {
@@ -518,7 +518,8 @@ static inline void switch_fpu_finish(struct fpu *new_fpu, int cpu)
 				copy_kernel_to_fpregs(&new_fpu->state);
 		}
 
-		fpregs_activate(new_fpu);
+		if (prev_fpu != new_fpu)
+			fpregs_activate(new_fpu);
 	}
 }
 
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 77d9eb43ccac..f8205df2df1d 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -290,7 +290,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 
 	this_cpu_write(current_task, next_p);
 
-	switch_fpu_finish(next_fpu, cpu);
+	switch_fpu_finish(prev_fpu, next_fpu, cpu);
 
 	/* Load the Intel cache allocation PQR MSR. */
 	resctrl_sched_in();
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index ffea7c557963..5f153b963180 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -572,7 +572,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	this_cpu_write(current_task, next_p);
 	this_cpu_write(cpu_current_top_of_stack, task_top_of_stack(next_p));
 
-	switch_fpu_finish(next_fpu, cpu);
+	switch_fpu_finish(prev_fpu, next_fpu, cpu);
 
 	/* Reload sp0. */
 	update_task_stack(next_p);


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.