* [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups
@ 2014-09-05 13:43 Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 1/5] x86, fpu: introduce per-cpu "bool in_kernel_fpu" Oleg Nesterov
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-09-05 13:43 UTC (permalink / raw)
To: Suresh Siddha, H. Peter Anvin
Cc: Fenghua Yu, Ingo Molnar, Linus Torvalds, x86, linux-kernel
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(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/5] x86, fpu: introduce per-cpu "bool in_kernel_fpu"
2014-09-05 13:43 [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups Oleg Nesterov
@ 2014-09-05 13:43 ` Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 2/5] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_{begin,end}() Oleg Nesterov
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-09-05 13:43 UTC (permalink / raw)
To: Suresh Siddha, H. Peter Anvin
Cc: Fenghua Yu, Ingo Molnar, Linus Torvalds, x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/5] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_{begin,end}()
2014-09-05 13:43 [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 1/5] x86, fpu: introduce per-cpu "bool in_kernel_fpu" Oleg Nesterov
@ 2014-09-05 13:43 ` Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 3/5] x86, fpu: irq_fpu_usable: always return true if use_eager_fpu() Oleg Nesterov
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-09-05 13:43 UTC (permalink / raw)
To: Suresh Siddha, H. Peter Anvin
Cc: Fenghua Yu, Ingo Molnar, Linus Torvalds, x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/5] x86, fpu: irq_fpu_usable: always return true if use_eager_fpu()
2014-09-05 13:43 [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 1/5] x86, fpu: introduce per-cpu "bool in_kernel_fpu" Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 2/5] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_{begin,end}() Oleg Nesterov
@ 2014-09-05 13:43 ` Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 4/5] x86, fpu: fix math_state_restore() race with kernel_fpu_begin() Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 5/5] x86, fpu: introduce try_to_restore_fpu() Oleg Nesterov
4 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-09-05 13:43 UTC (permalink / raw)
To: Suresh Siddha, H. Peter Anvin
Cc: Fenghua Yu, Ingo Molnar, Linus Torvalds, x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 4/5] x86, fpu: fix math_state_restore() race with kernel_fpu_begin()
2014-09-05 13:43 [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups Oleg Nesterov
` (2 preceding siblings ...)
2014-09-05 13:43 ` [PATCH v2 3/5] x86, fpu: irq_fpu_usable: always return true if use_eager_fpu() Oleg Nesterov
@ 2014-09-05 13:43 ` Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 5/5] x86, fpu: introduce try_to_restore_fpu() Oleg Nesterov
4 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-09-05 13:43 UTC (permalink / raw)
To: Suresh Siddha, H. Peter Anvin
Cc: Fenghua Yu, Ingo Molnar, Linus Torvalds, x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 5/5] x86, fpu: introduce try_to_restore_fpu()
2014-09-05 13:43 [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups Oleg Nesterov
` (3 preceding siblings ...)
2014-09-05 13:43 ` [PATCH v2 4/5] x86, fpu: fix math_state_restore() race with kernel_fpu_begin() Oleg Nesterov
@ 2014-09-05 13:43 ` Oleg Nesterov
4 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-09-05 13:43 UTC (permalink / raw)
To: Suresh Siddha, H. Peter Anvin
Cc: Fenghua Yu, Ingo Molnar, Linus Torvalds, x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-05 13:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-05 13:43 [PATCH v2 0/5] x86, fpu: kernel_fpu_begin/end fixes/cleanups Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 1/5] x86, fpu: introduce per-cpu "bool in_kernel_fpu" Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 2/5] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_{begin,end}() Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 3/5] x86, fpu: irq_fpu_usable: always return true if use_eager_fpu() Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 4/5] x86, fpu: fix math_state_restore() race with kernel_fpu_begin() Oleg Nesterov
2014-09-05 13:43 ` [PATCH v2 5/5] x86, fpu: introduce try_to_restore_fpu() Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).