[PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups

[PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups

[Oleg Nesterov]

Hello,

Suresh, please review when you have time. FYI, I will be travelling
till Sep 10 and I won't be able to reply.

Not for inclusion yet, just for review.

v2:

	1-3: No changes except the minor updates in the changelogs
	     and comments

	4-5: New. 5/5 is purely cosmetic.

Oleg.

 arch/x86/include/asm/fpu-internal.h |   15 ++++++++---
 arch/x86/include/asm/i387.h         |    6 ++++-
 arch/x86/kernel/i387.c              |   45 +++++++++++++++++++++-------------
 arch/x86/kernel/traps.c             |   16 ++++-------
 4 files changed, 50 insertions(+), 32 deletions(-)

[PATCH v2 1/5] x86, fpu: introduce per-cpu "bool in_kernel_fpu"

[Oleg Nesterov]

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 doesn't look clean.

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 some cleanups and fixes.

Note that the current code looks racy. Say, kernel_fpu_begin() right
after math_state_restore()->__thread_fpu_begin() will overwrite the
regs we are going to restore. This patch doesn't try to fix this, it
only adds a comment, but "in_kernel_fpu" can also be used to implement
kernel_fpu_disable() / kernel_fpu_enable().

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 <oleg@redhat.com>
---
 arch/x86/include/asm/i387.h |    2 +-
 arch/x86/kernel/i387.c      |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index ed8089d..5e275d3 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -40,8 +40,8 @@ extern void __kernel_fpu_end(void);
 
 static inline void kernel_fpu_begin(void)
 {
-	WARN_ON_ONCE(!irq_fpu_usable());
 	preempt_disable();
+	WARN_ON_ONCE(!irq_fpu_usable());
 	__kernel_fpu_begin();
 }
 
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index d5dd808..8fb8868 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -19,6 +19,8 @@
 #include <asm/fpu-internal.h>
 #include <asm/user.h>
 
+static DEFINE_PER_CPU(bool, in_kernel_fpu);
+
 /*
  * Were we in an interrupt that interrupted kernel mode?
  *
@@ -33,6 +35,9 @@
  */
 static inline bool interrupted_kernel_fpu_idle(void)
 {
+	if (this_cpu_read(in_kernel_fpu))
+		return false;
+
 	if (use_eager_fpu())
 		return __thread_has_fpu(current);
 
@@ -73,6 +78,9 @@ void __kernel_fpu_begin(void)
 {
 	struct task_struct *me = current;
 
+	this_cpu_write(in_kernel_fpu, true);
+
+	/* FIXME: race with math_state_restore()-like code */
 	if (__thread_has_fpu(me)) {
 		__thread_clear_has_fpu(me);
 		__save_init_fpu(me);
@@ -99,6 +107,8 @@ void __kernel_fpu_end(void)
 	} else {
 		stts();
 	}
+
+	this_cpu_write(in_kernel_fpu, false);
 }
 EXPORT_SYMBOL(__kernel_fpu_end);
 
-- 
1.5.5.1

[PATCH v2 2/5] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_{begin,end}()

[Oleg Nesterov]

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().

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 <oleg@redhat.com>
---
 arch/x86/kernel/i387.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 8fb8868..19dd36d 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -82,9 +82,7 @@ void __kernel_fpu_begin(void)
 
 	/* FIXME: race with math_state_restore()-like code */
 	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();
@@ -94,17 +92,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);
+	} else if (!use_eager_fpu()) {
 		stts();
 	}
 
-- 
1.5.5.1

[PATCH v2 3/5] x86, fpu: irq_fpu_usable: always return true if use_eager_fpu()

[Oleg Nesterov]

According to the comment and the changelog in 5187b28f "x86: Allow FPU
to be used at interrupt time even with eagerfpu", the __thread_has_fpu()
check was added to avoid the nested kernel_fpu_begin(). Now that we have
in_kernel_fpu we can remove this check and always return true.

__thread_has_fpu() can be false even if use_eager_fpu(), but this case
doesn't differ from !use_eager_fpu() case except we should not worry
about X86_CR0_TS, __kernel_fpu_begin/end will not touch this bit. And I
still think that "use_eager_fpu && (!__thread_has_fpu || !used_math)"
special cases should die, but this is off-topic right now.

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

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 19dd36d..7bc8236 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -30,8 +30,8 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
  * 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)
 {
@@ -39,7 +39,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

[PATCH v2 4/5] x86, fpu: fix math_state_restore() race with kernel_fpu_begin()

[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.

Perhaps we should export in_kernel_fpu and make these helpers inline.
Probably they will have more users.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/i387.h |    4 ++++
 arch/x86/kernel/i387.c      |   12 +++++++++++-
 arch/x86/kernel/traps.c     |   12 +++++-------
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index 5e275d3..605b1eb 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -51,6 +51,10 @@ static inline void kernel_fpu_end(void)
 	preempt_enable();
 }
 
+/* Must be called woth preempt disabled */
+extern void kernel_fpu_disable(void);
+extern void kernel_fpu_enable(void);
+
 /*
  * Some instructions like VIA's padlock instructions generate a spurious
  * DNA fault but don't modify SSE registers. And these instructions
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 7bc8236..ece91cf 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -21,6 +21,17 @@
 
 static DEFINE_PER_CPU(bool, in_kernel_fpu);
 
+void kernel_fpu_disable(void)
+{
+	WARN_ON(this_cpu_read(in_kernel_fpu));
+	this_cpu_write(in_kernel_fpu, true);
+}
+
+void kernel_fpu_enable(void)
+{
+	this_cpu_write(in_kernel_fpu, false);
+}
+
 /*
  * Were we in an interrupt that interrupted kernel mode?
  *
@@ -80,7 +91,6 @@ void __kernel_fpu_begin(void)
 
 	this_cpu_write(in_kernel_fpu, true);
 
-	/* FIXME: race with math_state_restore()-like code */
 	if (__thread_has_fpu(me)) {
 		__save_init_fpu(me);
 	} else if (!use_eager_fpu()) {
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 0d0e922..c632843 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -667,18 +667,16 @@ void math_state_restore(void)
 		local_irq_disable();
 	}
 
+	/* Avoid __kernel_fpu_begin() right after __thread_fpu_begin() */
+	kernel_fpu_disable();
 	__thread_fpu_begin(tsk);
-
-	/*
-	 * Paranoid restore. send a SIGSEGV if we fail to restore the state.
-	 */
 	if (unlikely(restore_fpu_checking(tsk))) {
 		drop_init_fpu(tsk);
 		force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
-		return;
+	} else {
+		tsk->thread.fpu_counter++;
 	}
-
-	tsk->thread.fpu_counter++;
+	kernel_fpu_enable();
 }
 EXPORT_SYMBOL_GPL(math_state_restore);
 
-- 
1.5.5.1

[PATCH v2 5/5] x86, fpu: introduce try_to_restore_fpu()

[Oleg Nesterov]

Cosmetic. Every caller of restore_fpu_checking() does drop_init_fpu()
on failure, add the trivial helper and update the callers.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h |   15 +++++++++++----
 arch/x86/kernel/i387.c              |    8 +++-----
 arch/x86/kernel/traps.c             |    8 +++-----
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 4cbb40e..e71c7f5 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -384,6 +384,15 @@ static inline void drop_init_fpu(struct task_struct *tsk)
 	}
 }
 
+static inline bool try_to_restore_fpu(struct task_struct *tsk)
+{
+	if (unlikely(restore_fpu_checking(tsk))) {
+		drop_init_fpu(tsk);
+		return false;
+	}
+	return true;
+}
+
 /*
  * FPU state switching for scheduling.
  *
@@ -462,10 +471,8 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
  */
 static inline void switch_fpu_finish(struct task_struct *new, fpu_switch_t fpu)
 {
-	if (fpu.preload) {
-		if (unlikely(restore_fpu_checking(new)))
-			drop_init_fpu(new);
-	}
+	if (fpu.preload)
+		try_to_restore_fpu(new);
 }
 
 /*
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index ece91cf..1918f67 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -104,12 +104,10 @@ void __kernel_fpu_end(void)
 {
 	struct task_struct *me = current;
 
-	if (__thread_has_fpu(me)) {
-		if (WARN_ON(restore_fpu_checking(me)))
-			drop_init_fpu(me);
-	} else if (!use_eager_fpu()) {
+	if (__thread_has_fpu(me))
+		WARN_ON(!try_to_restore_fpu(me));
+	else if (!use_eager_fpu())
 		stts();
-	}
 
 	this_cpu_write(in_kernel_fpu, false);
 }
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c632843..f516cf1 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -670,12 +670,10 @@ void math_state_restore(void)
 	/* Avoid __kernel_fpu_begin() right after __thread_fpu_begin() */
 	kernel_fpu_disable();
 	__thread_fpu_begin(tsk);
-	if (unlikely(restore_fpu_checking(tsk))) {
-		drop_init_fpu(tsk);
-		force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
-	} else {
+	if (likely(try_to_restore_fpu(tsk)))
 		tsk->thread.fpu_counter++;
-	}
+	else
+		force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
 	kernel_fpu_enable();
 }
 EXPORT_SYMBOL_GPL(math_state_restore);
-- 
1.5.5.1