Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567339 > unrolled thread

[PATCH 0/7] x86/fpu: Simplify the FPU state machine

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] x86/fpu: Simplify the FPU state machine Ingo Molnar <mingo@kernel.org> - 2017-01-26 12:30 +0100
    [PATCH 4/7] x86/fpu: Split the state handling in fpu__drop() Ingo Molnar <mingo@kernel.org> - 2017-01-26 12:30 +0100
    [PATCH 7/7] x86/fpu: Remove struct fpu::fpregs_active Ingo Molnar <mingo@kernel.org> - 2017-01-26 12:30 +0100
    [PATCH 2/7] x86/fpu: Simplify fpu->fpregs_active use Ingo Molnar <mingo@kernel.org> - 2017-01-26 12:30 +0100
      Re: [PATCH 2/7] x86/fpu: Simplify fpu->fpregs_active use Andy Lutomirski <luto@amacapital.net> - 2017-01-26 17:40 +0100
    [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached Ingo Molnar <mingo@kernel.org> - 2017-01-26 12:30 +0100
      Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and  rename it to fpu->fpregs_cached Rik van Riel <riel@redhat.com> - 2017-01-26 15:30 +0100
        Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename  it to fpu->fpregs_cached Ingo Molnar <mingo@kernel.org> - 2017-01-26 16:00 +0100
          [PATCH] x86/fpu: Unify the naming of the FPU register cache validity  flags Ingo Molnar <mingo@kernel.org> - 2017-01-26 16:10 +0100
            Re: [PATCH] x86/fpu: Unify the naming of the FPU register cache  validity flags Peter Zijlstra <peterz@infradead.org> - 2017-01-26 16:40 +0100
      Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and  rename it to fpu->fpregs_cached Rik van Riel <riel@redhat.com> - 2017-01-26 16:00 +0100
        Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename  it to fpu->fpregs_cached Ingo Molnar <mingo@kernel.org> - 2017-01-26 16:20 +0100
        Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename  it to fpu->fpregs_cached Andy Lutomirski <luto@amacapital.net> - 2017-01-26 18:00 +0100

#1567339 — [PATCH 0/7] x86/fpu: Simplify the FPU state machine

Now that the lazy FPU switching code has been removed this series capitalizes
on the simpler FPU context switching machinery and simplifies the x86 FPU
handling state machine code.

The new code relies on just two obvious fields, ::fpstate_active and ::fpregs_cached:

/*
 * Highest level per task FPU state data structure that
 * contains the FPU register state plus various FPU
 * state fields:
 */
struct fpu {
	/*
	 * @fpstate_active:
	 *
	 * This flag indicates whether this context is active: if the task
	 * is not running then we can restore from this context, if the task
	 * is running then we should save into this context.
	 */
	unsigned char			fpstate_active;

	/*
	 * @fpregs_cached:
	 *
	 * This flag tells us whether this context is loaded into a CPU
	 * right now.
	 *
	 * This is set to 0 if a task is migrated to another CPU.
	 */
	unsigned char			fpregs_cached;

	/*
	 * @state:
	 *
	 * In-memory copy of all FPU registers that we save/restore
	 * over context switches. If the task is using the FPU then
	 * the registers in the FPU are more recent than this state
	 * copy. If the task context-switches away then they get
	 * saved here and represent the FPU state.
	 */
	union fpregs_state		state;
	/*
	 * WARNING: 'state' is dynamically-sized.  Do not put
	 * anything after it here.
	 */
};

In particular ::last_cpu is replaced by a single ::fpregs_cached field
(which is invalidated on migration), plus ::fpregs_state is gone.

There's some code size reduction:

 7 files changed, 41 insertions(+), 87 deletions(-)

... and the code is a lot more obvious now as well, I think.

Lightly tested.

The code can also be fetched from:

   git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.x86/fpu

Thanks,

	Ingo

Ingo Molnar (7):
  x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached
  x86/fpu: Simplify fpu->fpregs_active use
  x86/fpu: Make the fpu state change in fpu__clear() scheduler-atomic
  x86/fpu: Split the state handling in fpu__drop()
  x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active
  x86/fpu: Decouple fpregs_activate()/fpregs_deactivate() from fpu->fpregs_active
  x86/fpu: Remove struct fpu::fpregs_active

 arch/x86/include/asm/fpu/internal.h | 42 ++++++++++--------------------------------
 arch/x86/include/asm/fpu/types.h    | 37 +++++--------------------------------
 arch/x86/include/asm/switch_to.h    | 10 ++++++++++
 arch/x86/include/asm/trace/fpu.h    |  5 +----
 arch/x86/kernel/fpu/core.c          | 29 ++++++++++++++++-------------
 arch/x86/kernel/fpu/signal.c        |  9 +++++----
 arch/x86/mm/pkeys.c                 |  3 +--
 drivers/lguest/x86/core.c           |  2 +-
 kernel/sched/core.c                 |  2 ++
 kernel/sched/sched.h                |  8 ++++++++
 10 files changed, 59 insertions(+), 88 deletions(-)

[toc] | [next] | [standalone]

#1567340 — [PATCH 4/7] x86/fpu: Split the state handling in fpu__drop()

Prepare fpu__drop() to use fpu->fpregs_active.

There are two distinct usecases for fpu__drop() in this context:
exit_thread() when called for 'current' in exit(), and when called
for another task in fork().

This patch does not change behavior, it only adds a couple of
debug checks and structures the code to make the ->fpregs_active
change more obviously correct.

All the complications will be removed later on.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/core.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7e1c77ac90e8..2973afdb39f1 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -414,12 +414,19 @@ void fpu__drop(struct fpu *fpu)
 {
 	preempt_disable();
 
-	if (fpu->fpregs_active) {
-		/* Ignore delayed exceptions from user space */
-		asm volatile("1: fwait\n"
-			     "2:\n"
-			     _ASM_EXTABLE(1b, 2b));
-		fpregs_deactivate(fpu);
+	if (fpu == &current->thread.fpu) {
+		WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
+
+		if (fpu->fpregs_active) {
+			/* Ignore delayed exceptions from user space */
+			asm volatile("1: fwait\n"
+				     "2:\n"
+				     _ASM_EXTABLE(1b, 2b));
+			if (fpu->fpregs_active)
+				fpregs_deactivate(fpu);
+		}
+	} else {
+		WARN_ON_FPU(fpu->fpregs_active);
 	}
 
 	fpu->fpstate_active = 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567344 — [PATCH 7/7] x86/fpu: Remove struct fpu::fpregs_active

The previous changes paved the way for the removal of the
fpu::fpregs_active state flag - we now only have the
fpu::fpstate_active and fpu::fpregs_cached flags left.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/internal.h |  5 -----
 arch/x86/include/asm/fpu/types.h    | 23 -----------------------
 arch/x86/include/asm/trace/fpu.h    |  5 +----
 arch/x86/kernel/fpu/core.c          |  9 ---------
 arch/x86/kernel/fpu/signal.c        |  2 --
 5 files changed, 1 insertion(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 0f4590fbc471..e62eee2e989e 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -526,14 +526,12 @@ static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu)
  */
 static inline void fpregs_deactivate(struct fpu *fpu)
 {
-	fpu->fpregs_active = 0;
 	this_cpu_write(fpu_fpregs_owner_ctx, NULL);
 	trace_x86_fpu_regs_deactivated(fpu);
 }
 
 static inline void fpregs_activate(struct fpu *fpu)
 {
-	fpu->fpregs_active = 1;
 	this_cpu_write(fpu_fpregs_owner_ctx, fpu);
 	trace_x86_fpu_regs_activated(fpu);
 }
@@ -552,8 +550,6 @@ static inline void fpregs_activate(struct fpu *fpu)
 static inline void
 switch_fpu_prepare(struct fpu *old_fpu, int cpu)
 {
-	WARN_ON_FPU(old_fpu->fpregs_active != old_fpu->fpstate_active);
-
 	if (old_fpu->fpstate_active) {
 		if (!copy_fpregs_to_fpstate(old_fpu))
 			old_fpu->fpregs_cached = 0;
@@ -561,7 +557,6 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
 			old_fpu->fpregs_cached = 1;
 
 		/* But leave fpu_fpregs_owner_ctx! */
-		old_fpu->fpregs_active = 0;
 		trace_x86_fpu_regs_deactivated(old_fpu);
 	} else {
 		old_fpu->fpregs_cached = 0;
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 3090b0d7b232..07452fbd7867 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -285,29 +285,6 @@ struct fpu {
 	unsigned char			fpstate_active;
 
 	/*
-	 * @fpregs_active:
-	 *
-	 * This flag determines whether a given context is actively
-	 * loaded into the FPU's registers and that those registers
-	 * represent the task's current FPU state.
-	 *
-	 * Note the interaction with fpstate_active:
-	 *
-	 *   # task does not use the FPU:
-	 *   fpstate_active == 0
-	 *
-	 *   # task uses the FPU and regs are active:
-	 *   fpstate_active == 1 && fpregs_active == 1
-	 *
-	 *   # the regs are inactive but still match fpstate:
-	 *   fpstate_active == 1 && fpregs_active == 0 && fpregs_owner == fpu
-	 *
-	 * The third state is what we use for the lazy restore optimization
-	 * on lazy-switching CPUs.
-	 */
-	unsigned char			fpregs_active;
-
-	/*
 	 * @fpregs_cached:
 	 *
 	 * This flag tells us whether this context is loaded into a CPU
diff --git a/arch/x86/include/asm/trace/fpu.h b/arch/x86/include/asm/trace/fpu.h
index 342e59789fcd..da565aae9fd2 100644
--- a/arch/x86/include/asm/trace/fpu.h
+++ b/arch/x86/include/asm/trace/fpu.h
@@ -12,7 +12,6 @@ DECLARE_EVENT_CLASS(x86_fpu,
 
 	TP_STRUCT__entry(
 		__field(struct fpu *, fpu)
-		__field(bool, fpregs_active)
 		__field(bool, fpstate_active)
 		__field(u64, xfeatures)
 		__field(u64, xcomp_bv)
@@ -20,16 +19,14 @@ DECLARE_EVENT_CLASS(x86_fpu,
 
 	TP_fast_assign(
 		__entry->fpu		= fpu;
-		__entry->fpregs_active	= fpu->fpregs_active;
 		__entry->fpstate_active	= fpu->fpstate_active;
 		if (boot_cpu_has(X86_FEATURE_OSXSAVE)) {
 			__entry->xfeatures = fpu->state.xsave.header.xfeatures;
 			__entry->xcomp_bv  = fpu->state.xsave.header.xcomp_bv;
 		}
 	),
-	TP_printk("x86/fpu: %p fpregs_active: %d fpstate_active: %d xfeatures: %llx xcomp_bv: %llx",
+	TP_printk("x86/fpu: %p fpstate_active: %d xfeatures: %llx xcomp_bv: %llx",
 			__entry->fpu,
-			__entry->fpregs_active,
 			__entry->fpstate_active,
 			__entry->xfeatures,
 			__entry->xcomp_bv
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index a025be6637cc..217e37029585 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -147,8 +147,6 @@ void fpu__save(struct fpu *fpu)
 	WARN_ON_FPU(fpu != &current->thread.fpu);
 
 	preempt_disable();
-	WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
-
 	trace_x86_fpu_before_save(fpu);
 	if (fpu->fpstate_active) {
 		if (!copy_fpregs_to_fpstate(fpu)) {
@@ -191,7 +189,6 @@ EXPORT_SYMBOL_GPL(fpstate_init);
 
 int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 {
-	dst_fpu->fpregs_active = 0;
 	dst_fpu->fpregs_cached = 0;
 
 	if (!src_fpu->fpstate_active || !static_cpu_has(X86_FEATURE_FPU))
@@ -264,7 +261,6 @@ EXPORT_SYMBOL_GPL(fpu__activate_curr);
  */
 void fpu__activate_fpstate_read(struct fpu *fpu)
 {
-	WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
 	/*
 	 * If fpregs are active (in the current CPU), then
 	 * copy them to the fpstate:
@@ -365,7 +361,6 @@ void fpu__current_fpstate_write_end(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
-	WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
 	/*
 	 * 'fpu' now has an updated copy of the state, but the
 	 * registers may still be out of date.  Update them with
@@ -419,8 +414,6 @@ void fpu__drop(struct fpu *fpu)
 	preempt_disable();
 
 	if (fpu == &current->thread.fpu) {
-		WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
-
 		if (fpu->fpstate_active) {
 			/* Ignore delayed exceptions from user space */
 			asm volatile("1: fwait\n"
@@ -428,8 +421,6 @@ void fpu__drop(struct fpu *fpu)
 				     _ASM_EXTABLE(1b, 2b));
 			fpregs_deactivate(fpu);
 		}
-	} else {
-		WARN_ON_FPU(fpu->fpregs_active);
 	}
 
 	fpu->fpstate_active = 0;
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index a88083ba7f8b..629106e51a29 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -171,8 +171,6 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_32 __user *) buf) ? -1 : 1;
 
-	WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
-
 	if (fpu->fpstate_active || using_compacted_format()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567345 — [PATCH 2/7] x86/fpu: Simplify fpu->fpregs_active use

The fpregs_active() inline function is pretty pointless - in almost
all the callsites it can be replaced with a direct fpu->fpregs_active
access.

Do so and eliminate the extra layer of obfuscation.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/internal.h | 17 +----------------
 arch/x86/kernel/fpu/core.c          |  2 +-
 arch/x86/kernel/fpu/signal.c        |  9 +++++----
 arch/x86/mm/pkeys.c                 |  2 +-
 drivers/lguest/x86/core.c           |  2 +-
 5 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 2eaf93cf11cc..d6307e76837c 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -543,21 +543,6 @@ static inline void fpregs_activate(struct fpu *fpu)
 }
 
 /*
- * The question "does this thread have fpu access?"
- * is slightly racy, since preemption could come in
- * and revoke it immediately after the test.
- *
- * However, even in that very unlikely scenario,
- * we can just assume we have FPU access - typically
- * to save the FP state - we'll just take a #NM
- * fault and get the FPU access back.
- */
-static inline int fpregs_active(void)
-{
-	return current->thread.fpu.fpregs_active;
-}
-
-/*
  * FPU state switching for scheduling.
  *
  * This is a two-stage process:
@@ -618,7 +603,7 @@ static inline void user_fpu_begin(void)
 	struct fpu *fpu = &current->thread.fpu;
 
 	preempt_disable();
-	if (!fpregs_active())
+	if (!fpu->fpregs_active)
 		fpregs_activate(fpu);
 	preempt_enable();
 }
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 287f1cb32b59..8775343ca75b 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -367,7 +367,7 @@ void fpu__current_fpstate_write_end(void)
 	 * registers may still be out of date.  Update them with
 	 * an XRSTOR if they are active.
 	 */
-	if (fpregs_active())
+	if (fpu->fpregs_active)
 		copy_kernel_to_fpregs(&fpu->state);
 
 	/*
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 2d682dac35d4..684025654d0c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -155,7 +155,8 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
  */
 int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 {
-	struct xregs_state *xsave = &current->thread.fpu.state.xsave;
+	struct fpu *fpu = &current->thread.fpu;
+	struct xregs_state *xsave = &fpu->state.xsave;
 	struct task_struct *tsk = current;
 	int ia32_fxstate = (buf != buf_fx);
 
@@ -170,13 +171,13 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_32 __user *) buf) ? -1 : 1;
 
-	if (fpregs_active() || using_compacted_format()) {
+	if (fpu->fpregs_active || using_compacted_format()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
 			return -1;
 		/* Update the thread's fxstate to save the fsave header. */
 		if (ia32_fxstate)
-			copy_fxregs_to_kernel(&tsk->thread.fpu);
+			copy_fxregs_to_kernel(fpu);
 	} else {
 		/*
 		 * It is a *bug* if kernel uses compacted-format for xsave
@@ -189,7 +190,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			return -1;
 		}
 
-		fpstate_sanitize_xstate(&tsk->thread.fpu);
+		fpstate_sanitize_xstate(fpu);
 		if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
 			return -1;
 	}
diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
index 2dab69a706ec..e2c23472233e 100644
--- a/arch/x86/mm/pkeys.c
+++ b/arch/x86/mm/pkeys.c
@@ -45,7 +45,7 @@ int __execute_only_pkey(struct mm_struct *mm)
 	 */
 	preempt_disable();
 	if (!need_to_set_mm_pkey &&
-	    fpregs_active() &&
+	    current->thread.fpu.fpregs_active &&
 	    !__pkru_allows_read(read_pkru(), execute_only_pkey)) {
 		preempt_enable();
 		return execute_only_pkey;
diff --git a/drivers/lguest/x86/core.c b/drivers/lguest/x86/core.c
index d71f6323ac00..328974ae690d 100644
--- a/drivers/lguest/x86/core.c
+++ b/drivers/lguest/x86/core.c
@@ -289,7 +289,7 @@ void lguest_arch_run_guest(struct lg_cpu *cpu)
 	 * a different CPU. So all the critical stuff should be done
 	 * before this.
 	 */
-	else if (cpu->regs->trapnum == 7 && !fpregs_active())
+	else if (cpu->regs->trapnum == 7 && !current->thread.fpu.fpstate_active)
 		fpu__restore(&current->thread.fpu);
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567590 — Re: [PATCH 2/7] x86/fpu: Simplify fpu->fpregs_active use

On Thu, Jan 26, 2017 at 3:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
> The fpregs_active() inline function is pretty pointless - in almost
> all the callsites it can be replaced with a direct fpu->fpregs_active
> access.
>
> Do so and eliminate the extra layer of obfuscation.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1567346 — [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached

fpu->last_cpu records the last CPU a given FPU context structure was used on.
This enables an important optimization: if a task schedules out to a kernel
thread and then gets scheduled back after only FPU-inactive kernel threads
executed, the FPU state in the registers is still intact and the FPU restore
can be skipped - speeding up the context switch.

The same logic can be implemented slightly simpler, by using a single boolean
flag: fpu->fpregs_cached tells us whether the context's FPU registers are
cached in the CPU.

The only difference is that this flag has to be invalidated when a task is
migrated away from its CPU - but that is a slow path compared to context
switches.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/internal.h | 15 ++++++++-------
 arch/x86/include/asm/fpu/types.h    | 24 ++++++++++--------------
 arch/x86/include/asm/switch_to.h    | 10 ++++++++++
 arch/x86/kernel/fpu/core.c          |  2 +-
 kernel/sched/core.c                 |  2 ++
 kernel/sched/sched.h                |  8 ++++++++
 6 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 255645f60ca2..2eaf93cf11cc 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -490,7 +490,7 @@ DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
 
 /*
  * The in-register FPU state for an FPU context on a CPU is assumed to be
- * valid if the fpu->last_cpu matches the CPU, and the fpu_fpregs_owner_ctx
+ * valid if fpu->fpregs_cached is still set, and if the fpu_fpregs_owner_ctx
  * matches the FPU.
  *
  * If the FPU register state is valid, the kernel can skip restoring the
@@ -512,12 +512,12 @@ static inline void __cpu_invalidate_fpregs_state(void)
 
 static inline void __fpu_invalidate_fpregs_state(struct fpu *fpu)
 {
-	fpu->last_cpu = -1;
+	fpu->fpregs_cached = 0;
 }
 
 static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu)
 {
-	return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu;
+	return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && fpu->fpregs_cached;
 }
 
 /*
@@ -573,15 +573,16 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
 {
 	if (old_fpu->fpregs_active) {
 		if (!copy_fpregs_to_fpstate(old_fpu))
-			old_fpu->last_cpu = -1;
+			old_fpu->fpregs_cached = 0;
 		else
-			old_fpu->last_cpu = cpu;
+			old_fpu->fpregs_cached = 1;
 
 		/* But leave fpu_fpregs_owner_ctx! */
 		old_fpu->fpregs_active = 0;
 		trace_x86_fpu_regs_deactivated(old_fpu);
-	} else
-		old_fpu->last_cpu = -1;
+	} else {
+		old_fpu->fpregs_cached = 0;
+	}
 }
 
 /*
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 3c80f5b9c09d..3090b0d7b232 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -276,20 +276,6 @@ union fpregs_state {
  */
 struct fpu {
 	/*
-	 * @last_cpu:
-	 *
-	 * Records the last CPU on which this context was loaded into
-	 * FPU registers. (In the lazy-restore case we might be
-	 * able to reuse FPU registers across multiple context switches
-	 * this way, if no intermediate task used the FPU.)
-	 *
-	 * A value of -1 is used to indicate that the FPU state in context
-	 * memory is newer than the FPU state in registers, and that the
-	 * FPU state should be reloaded next time the task is run.
-	 */
-	unsigned int			last_cpu;
-
-	/*
 	 * @fpstate_active:
 	 *
 	 * This flag indicates whether this context is active: if the task
@@ -322,6 +308,16 @@ struct fpu {
 	unsigned char			fpregs_active;
 
 	/*
+	 * @fpregs_cached:
+	 *
+	 * This flag tells us whether this context is loaded into a CPU
+	 * right now.
+	 *
+	 * This is set to 0 if a task is migrated to another CPU.
+	 */
+	unsigned char			fpregs_cached;
+
+	/*
 	 * @state:
 	 *
 	 * In-memory copy of all FPU registers that we save/restore
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index fcc5cd387fd1..a7146dadb31d 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -72,4 +72,14 @@ do {									\
 	((last) = __switch_to_asm((prev), (next)));			\
 } while (0)
 
+
+/*
+ * The task-migration arch callback clears the FPU registers cache:
+ */
+static inline void arch_task_migrate(struct task_struct *p)
+{
+	p->thread.fpu.fpregs_cached = 0;
+}
+#define arch_task_migrate arch_task_migrate
+
 #endif /* _ASM_X86_SWITCH_TO_H */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index e1114f070c2d..287f1cb32b59 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -190,7 +190,7 @@ EXPORT_SYMBOL_GPL(fpstate_init);
 int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 {
 	dst_fpu->fpregs_active = 0;
-	dst_fpu->last_cpu = -1;
+	dst_fpu->fpregs_cached = 0;
 
 	if (!src_fpu->fpstate_active || !static_cpu_has(X86_FEATURE_FPU))
 		return 0;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c56fb57f2991..7eb2f3041fde 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1253,6 +1253,8 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 			p->sched_class->migrate_task_rq(p);
 		p->se.nr_migrations++;
 		perf_event_task_migrate(p);
+
+		arch_task_migrate(p);
 	}
 
 	__set_task_cpu(p, new_cpu);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 7b34c7826ca5..ff8a894132e4 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1824,3 +1824,11 @@ static inline void cpufreq_update_this_cpu(struct rq *rq, unsigned int flags) {}
 #else /* arch_scale_freq_capacity */
 #define arch_scale_freq_invariant()	(false)
 #endif
+
+/*
+ * Default task-migration arch callback:
+ */
+#ifndef arch_task_migrate
+static inline void arch_task_migrate(struct task_struct *p) { }
+#endif
+
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567463 — Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached

On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
> 
> @@ -322,6 +308,16 @@ struct fpu {
>  	unsigned char			fpregs_active;
>  
>  	/*
> +	 * @fpregs_cached:
> +	 *
> +	 * This flag tells us whether this context is loaded into a
> CPU
> +	 * right now.

Not quite. You are still checking against fpu_fpregs_owner_ctx.

How about something like

      * This flag tells us whether this context was loaded into
      * its current CPU; fpu_fpregs_owner_ctx will tell us whether
      * this context is actually in the registers.

> +	 *
> +	 * This is set to 0 if a task is migrated to another CPU.
> +	 */
> +	unsigned char			fpregs_cached;
> +
> +	/*
>  	 * @state:
>  	 *
>  	 * In-memory copy of all FPU registers that we save/restore

[toc] | [prev] | [next] | [standalone]

#1567487 — Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached

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

> On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
> > 
> > @@ -322,6 +308,16 @@ struct fpu {
> >  	unsigned char			fpregs_active;
> >  
> >  	/*
> > +	 * @fpregs_cached:
> > +	 *
> > +	 * This flag tells us whether this context is loaded into a
> > CPU
> > +	 * right now.
> 
> Not quite. You are still checking against fpu_fpregs_owner_ctx.

> How about something like
> 
>       * This flag tells us whether this context was loaded into
>       * its current CPU; fpu_fpregs_owner_ctx will tell us whether
>       * this context is actually in the registers.

That's still not quite accurate: if ->fpregs_cached is 0 and fpu_fpregs_owner_ctx 
is still pointing to the FPU structure then the context is not actually in the 
registers anymore - it's a stale copy of some past version.

These values simply tell us whether an in-memory FPU context's latest version is 
in CPU registers or not: both have to be valid for the in-CPU registers to be 
valid and current. The fpu_fpregs_owner_ctx pointer is a per-CPU data structure 
that tells us this fact, the ->fpregs_cached flag tells us the same - but it is 
placed into the task/fpu structure.

Clearing any of those values invalidates the cache and the point of keeping them 
split is implementation efficiency: for some invalidations it's easier to use the 
per-cpu structure, for some others (such as ptrace access) it's easier to access 
the per-task flag. The FPU switch-in code has easy access to both values so 
there's no extra cost from having the cache validity flag split into two parts.

A consequence of this is that a correct implementation could in theory eliminate 
any of the two flags:

 - We could use only fpu_fpregs_owner_ctx and remove ->fpregs_cached, in this case
   the ptrace codepaths would have to invalidate the fpu_fpregs_owner_ctx pointer 
   which requires some care as it's not just a local CPU modification, i.e. a 
   single cmpxchg() would be required to invalidate the register state.

 - Or we could use only ->fpregs_cached and eliminate fpu_fpregs_owner_ctx: this 
   would be awkward from the kernel_fpu_begin()/end() API codepaths, which has no 
   easy access to the task that has its FPU context cached in the CPU registers. 
   (Which might not be the current task.)

So I think the best implementation is to have both flags, and to use the one that 
is the most efficient to access to drive the invalidations from.

What we could do is to unify the naming to explain all this a bit better - right 
now there's very little indication that ->fpregs_cached is closely related to 
fpu_fpregs_owner_ctx.

For example we could rename them to:

	->fpregs_cached         =>     ->fpregs_owner        [bool]
	fpu_fpregs_owner_ctx    =>       fpregs_owner_ctx    [ptr]

?

Clearing ->fpregs_owner or setting fpregs_owner_ctx to NULL invalidates the cache 
and it's clear from the naming that the two values are closely related.

Would this work with you?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1567501 — [PATCH] x86/fpu: Unify the naming of the FPU register cache validity flags

* Ingo Molnar <mingo@kernel.org> wrote:

> What we could do is to unify the naming to explain all this a bit better - right 
> now there's very little indication that ->fpregs_cached is closely related to 
> fpu_fpregs_owner_ctx.
> 
> For example we could rename them to:
> 
> 	->fpregs_cached         =>     ->fpregs_owner        [bool]
> 	fpu_fpregs_owner_ctx    =>       fpregs_owner_ctx    [ptr]
> 
> ?
> 
> Clearing ->fpregs_owner or setting fpregs_owner_ctx to NULL invalidates the 
> cache and it's clear from the naming that the two values are closely related.

Something like the patch below - only minimally tested.

Thanks,

	Ingo

================>
From d5b99e1e25f86d4880bf85588eb4a4769610dd47 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Thu, 26 Jan 2017 16:01:00 +0100
Subject: [PATCH] x86/fpu: Unify the naming of the FPU register cache validity flags

Rik pointed out that the fpu_fpregs_owner_ctx and the ->fpregs_cached
flags are still described in a confusing way.

Clarify this some more by renaming them to:

        ->fpregs_cached         =>     ->fpregs_owner        [bool]
        fpu_fpregs_owner_ctx    =>       fpregs_owner_ctx    [ptr]

... which better expresses that they are a cache validity flag
split into two parts, where the cache can be invalidated if any
of the flags is cleared.

Also describe this relationship more accurately in the fpu/types.h header.

No change in functionality.

Reported-by: Rik van Riel <riel@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/internal.h | 22 +++++++++++-----------
 arch/x86/include/asm/fpu/types.h    | 12 ++++++++++--
 arch/x86/include/asm/switch_to.h    |  2 +-
 arch/x86/kernel/fpu/core.c          |  4 ++--
 arch/x86/kernel/smpboot.c           |  2 +-
 5 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index e62eee2e989e..bbee00aac864 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -486,11 +486,11 @@ extern int copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size)
  * FPU context switch related helper methods:
  */
 
-DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
+DECLARE_PER_CPU(struct fpu *, fpregs_owner_ctx);
 
 /*
  * The in-register FPU state for an FPU context on a CPU is assumed to be
- * valid if fpu->fpregs_cached is still set, and if the fpu_fpregs_owner_ctx
+ * valid if fpu->fpregs_owner is still set, and if the fpregs_owner_ctx
  * matches the FPU.
  *
  * If the FPU register state is valid, the kernel can skip restoring the
@@ -507,17 +507,17 @@ DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
  */
 static inline void __cpu_invalidate_fpregs_state(void)
 {
-	__this_cpu_write(fpu_fpregs_owner_ctx, NULL);
+	__this_cpu_write(fpregs_owner_ctx, NULL);
 }
 
 static inline void __fpu_invalidate_fpregs_state(struct fpu *fpu)
 {
-	fpu->fpregs_cached = 0;
+	fpu->fpregs_owner = 0;
 }
 
 static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu)
 {
-	return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && fpu->fpregs_cached;
+	return fpu == this_cpu_read_stable(fpregs_owner_ctx) && fpu->fpregs_owner;
 }
 
 /*
@@ -526,13 +526,13 @@ static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu)
  */
 static inline void fpregs_deactivate(struct fpu *fpu)
 {
-	this_cpu_write(fpu_fpregs_owner_ctx, NULL);
+	this_cpu_write(fpregs_owner_ctx, NULL);
 	trace_x86_fpu_regs_deactivated(fpu);
 }
 
 static inline void fpregs_activate(struct fpu *fpu)
 {
-	this_cpu_write(fpu_fpregs_owner_ctx, fpu);
+	this_cpu_write(fpregs_owner_ctx, fpu);
 	trace_x86_fpu_regs_activated(fpu);
 }
 
@@ -552,14 +552,14 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
 {
 	if (old_fpu->fpstate_active) {
 		if (!copy_fpregs_to_fpstate(old_fpu))
-			old_fpu->fpregs_cached = 0;
+			old_fpu->fpregs_owner = 0;
 		else
-			old_fpu->fpregs_cached = 1;
+			old_fpu->fpregs_owner = 1;
 
-		/* But leave fpu_fpregs_owner_ctx! */
+		/* But leave fpregs_owner_ctx! */
 		trace_x86_fpu_regs_deactivated(old_fpu);
 	} else {
-		old_fpu->fpregs_cached = 0;
+		old_fpu->fpregs_owner = 0;
 	}
 }
 
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 07452fbd7867..d15cbfe0e8c4 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -285,14 +285,22 @@ struct fpu {
 	unsigned char			fpstate_active;
 
 	/*
-	 * @fpregs_cached:
+	 * @fpregs_owner:
 	 *
 	 * This flag tells us whether this context is loaded into a CPU
 	 * right now.
 	 *
 	 * This is set to 0 if a task is migrated to another CPU.
+	 *
+	 * NOTE: the fpregs_owner_ctx percpu pointer also has to point to
+	 *       this FPU context for the register cache to be valid. If any
+	 *       of these two flags is cleared then the cache is invalid.
+	 *       Some internals can access the context-flag more easily,
+	 *       others have easier access to the percpu variable. The
+	 *       FPU context-switching code has access to both so there's
+	 *       very little cost of having the cache indexed in two ways:
 	 */
-	unsigned char			fpregs_cached;
+	unsigned char			fpregs_owner;
 
 	/*
 	 * @state:
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index a7146dadb31d..7a4915dd0547 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -78,7 +78,7 @@ do {									\
  */
 static inline void arch_task_migrate(struct task_struct *p)
 {
-	p->thread.fpu.fpregs_cached = 0;
+	p->thread.fpu.fpregs_owner = 0;
 }
 #define arch_task_migrate arch_task_migrate
 
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 217e37029585..1b3bf98072fe 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -39,7 +39,7 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
 /*
  * Track which context is using the FPU on the CPU:
  */
-DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
+DEFINE_PER_CPU(struct fpu *, fpregs_owner_ctx);
 
 static void kernel_fpu_disable(void)
 {
@@ -189,7 +189,7 @@ EXPORT_SYMBOL_GPL(fpstate_init);
 
 int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
 {
-	dst_fpu->fpregs_cached = 0;
+	dst_fpu->fpregs_owner = 0;
 
 	if (!src_fpu->fpstate_active || !static_cpu_has(X86_FEATURE_FPU))
 		return 0;
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 46732dc3b73c..8bf24aa01878 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1118,7 +1118,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 		return err;
 
 	/* the FPU context is blank, nobody can own it */
-	per_cpu(fpu_fpregs_owner_ctx, cpu) = NULL;
+	per_cpu(fpregs_owner_ctx, cpu) = NULL;
 
 	common_cpu_up(cpu, tidle);
 

[toc] | [prev] | [next] | [standalone]

#1567537 — Re: [PATCH] x86/fpu: Unify the naming of the FPU register cache validity flags

On Thu, Jan 26, 2017 at 04:05:25PM +0100, Ingo Molnar wrote:
> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index a7146dadb31d..7a4915dd0547 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -78,7 +78,7 @@ do {									\
>   */
>  static inline void arch_task_migrate(struct task_struct *p)
>  {
> -	p->thread.fpu.fpregs_cached = 0;
> +	p->thread.fpu.fpregs_owner = 0;
>  }
>  #define arch_task_migrate arch_task_migrate

I still really dislike having this callback..

[toc] | [prev] | [next] | [standalone]

#1567483 — Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached

On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:

> index c56fb57f2991..7eb2f3041fde 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1253,6 +1253,8 @@ void set_task_cpu(struct task_struct *p,
> unsigned int new_cpu)
>  			p->sched_class->migrate_task_rq(p);
>  		p->se.nr_migrations++;
>  		perf_event_task_migrate(p);
> +
> +		arch_task_migrate(p);
>  	}
> 

Does it really count as a "simplification" if you add a
scheduler callback?

This code does not seem any easier to understand than
the old code...

[toc] | [prev] | [next] | [standalone]

#1567511 — Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached

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

> On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
> 
> > index c56fb57f2991..7eb2f3041fde 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1253,6 +1253,8 @@ void set_task_cpu(struct task_struct *p,
> > unsigned int new_cpu)
> >  			p->sched_class->migrate_task_rq(p);
> >  		p->se.nr_migrations++;
> >  		perf_event_task_migrate(p);
> > +
> > +		arch_task_migrate(p);
> >  	}
> > 
> 
> Does it really count as a "simplification" if you add a
> scheduler callback?
> 
> This code does not seem any easier to understand than
> the old code...

See the extra commit I added on top:

  7deff4369276 x86/fpu: Unify the naming of the FPU register cache validity flags

which makes it clearer, we now have:
	
	->fpregs_owner             [bool]
          fpregs_owner_ctx         [ptr]

That are set to 1 and the context pointer when a task with no FPU state is 
scheduled in and where the state of the previous task is preserved (cached) in the 
FPU registers - and which FPU register state cache can be invalidated after this 
by clearing any of the two flags.

That should make its overall meaning clearer, in that they represent a single 
'cache' where the cache validity flag is split into two copies, where any of which 
can be used to invalidate the cache.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1567598 — Re: [PATCH 1/7] x86/fpu: Simplify the fpu->last_cpu logic and rename it to fpu->fpregs_cached

On Thu, Jan 26, 2017 at 6:54 AM, Rik van Riel <riel@redhat.com> wrote:
> On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
>
>> index c56fb57f2991..7eb2f3041fde 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -1253,6 +1253,8 @@ void set_task_cpu(struct task_struct *p,
>> unsigned int new_cpu)
>>                       p->sched_class->migrate_task_rq(p);
>>               p->se.nr_migrations++;
>>               perf_event_task_migrate(p);
>> +
>> +             arch_task_migrate(p);
>>       }
>>
>
> Does it really count as a "simplification" if you add a
> scheduler callback?
>
> This code does not seem any easier to understand than
> the old code...

I think I lean toward liking Ingo's version better.  The old code most
likely saved an instruction, but the new code gets the point across
quite nicely.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web