Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Mon, Mar 16, 2015 at 08:34:15AM -0700, Andy Lutomirski wrote: > Nice! This is the first time I've actually understood that :). I > still have no idea what "init" referred to... Haha, you're not the only one :-) We're learning as we go. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Mar 16, 2015 2:37 AM, "Borislav Petkov" wrote: > > On Sun, Mar 15, 2015 at 09:38:16PM +0100, Borislav Petkov wrote: > > How about we call this function fpu_reset_state() instead? > > IOW, something like this. Reading the usage sites actually make much > more sense to me now. It could be just me though... > > :-) > > --- > From: Borislav Petkov > Date: Mon, 16 Mar 2015 10:21:55 +0100 > Subject: [PATCH] x86/fpu: Rename drop_init_fpu() to fpu_reset_state() > > Call it what it does and in accordance with the context where it is > used: we reset the FPU state either because we were unable to restore it > from the one saved in the task or because we simply want to reset it. Nice! This is the first time I've actually understood that :). I still have no idea what "init" referred to... --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Mon, Mar 16, 2015 at 03:39:44PM +0100, Oleg Nesterov wrote: > ACK! Thanks. > Perhaps you can also find a beter name for __save_init_fpu/etc ;) The > name clearly suggests that it does "save + init" while in fact it does > "save and may be destroy FPU state". At least for the callers, the fact > that "destroy" is actually "init" doesn't really matter. > > But lets not rename it right now. This can conflict with the fixes we > need to do first. Right, so I think we should do fixes/cleanups first so that we can lose all the fat/cruft this code has grown. I'll make looking at that code easier later. I'll push out everything I have collected so far for people to see after I've finished bisecting another tip/master regression from today. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/16, Borislav Petkov wrote: > > -static inline void drop_init_fpu(struct task_struct *tsk) > +/* > + * Reset the FPU state in the eager case and drop it in the lazy case (later > use > + * will reinit it). > + */ > +static inline void fpu_reset_state(struct task_struct *tsk) ACK! Perhaps you can also find a beter name for __save_init_fpu/etc ;) The name clearly suggests that it does "save + init" while in fact it does "save and may be destroy FPU state". At least for the callers, the fact that "destroy" is actually "init" doesn't really matter. But lets not rename it right now. This can conflict with the fixes we need to do first. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
* Borislav Petkov wrote: > On Sun, Mar 15, 2015 at 09:38:16PM +0100, Borislav Petkov wrote: > > How about we call this function fpu_reset_state() instead? > > IOW, something like this. Reading the usage sites actually make much > more sense to me now. It could be just me though... > > :-) > > --- > From: Borislav Petkov > Date: Mon, 16 Mar 2015 10:21:55 +0100 > Subject: [PATCH] x86/fpu: Rename drop_init_fpu() to fpu_reset_state() > > Call it what it does and in accordance with the context where it is > used: we reset the FPU state either because we were unable to restore it > from the one saved in the task or because we simply want to reset it. Acked-by: Ingo Molnar Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sun, Mar 15, 2015 at 09:38:16PM +0100, Borislav Petkov wrote:
> How about we call this function fpu_reset_state() instead?
IOW, something like this. Reading the usage sites actually make much
more sense to me now. It could be just me though...
:-)
---
From: Borislav Petkov
Date: Mon, 16 Mar 2015 10:21:55 +0100
Subject: [PATCH] x86/fpu: Rename drop_init_fpu() to fpu_reset_state()
Call it what it does and in accordance with the context where it is
used: we reset the FPU state either because we were unable to restore it
from the one saved in the task or because we simply want to reset it.
Signed-off-by: Borislav Petkov
Cc: Oleg Nesterov
Cc: Andy Lutomirski
Cc: Linus Torvalds
Cc: Rik van Riel
Cc: Ingo Molnar
---
arch/x86/include/asm/fpu-internal.h | 8 ++--
arch/x86/kernel/i387.c | 2 +-
arch/x86/kernel/signal.c| 2 +-
arch/x86/kernel/traps.c | 2 +-
arch/x86/kernel/xsave.c | 4 ++--
5 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/fpu-internal.h
b/arch/x86/include/asm/fpu-internal.h
index 2d4adff428ac..da5e96756570 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -406,7 +406,11 @@ static inline void restore_init_xstate(void)
fxrstor_checking(_xstate_buf->i387);
}
-static inline void drop_init_fpu(struct task_struct *tsk)
+/*
+ * Reset the FPU state in the eager case and drop it in the lazy case (later
use
+ * will reinit it).
+ */
+static inline void fpu_reset_state(struct task_struct *tsk)
{
if (!use_eager_fpu())
drop_fpu(tsk);
@@ -480,7 +484,7 @@ 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);
+ fpu_reset_state(new);
}
}
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 29e982ada854..41575b9b1021 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -108,7 +108,7 @@ void __kernel_fpu_end(void)
if (__thread_has_fpu(me)) {
if (WARN_ON(restore_fpu_checking(me)))
- drop_init_fpu(me);
+ fpu_reset_state(me);
} else if (!use_eager_fpu()) {
stts();
}
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index e5042463c1bc..59eaae6185e2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -679,7 +679,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
* Ensure the signal handler starts with the new fpu state.
*/
if (used_math())
- drop_init_fpu(current);
+ fpu_reset_state(current);
}
signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 92b83e299ed3..156d75859466 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -863,7 +863,7 @@ void math_state_restore(void)
kernel_fpu_disable();
__thread_fpu_begin(tsk);
if (unlikely(restore_fpu_checking(tsk))) {
- drop_init_fpu(tsk);
+ fpu_reset_state(tsk);
force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
} else {
tsk->thread.fpu_counter++;
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 0bf82c5ac529..65c29b070e09 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -342,7 +342,7 @@ int __restore_xstate_sig(void __user *buf, void __user
*buf_fx, int size)
config_enabled(CONFIG_IA32_EMULATION));
if (!buf) {
- drop_init_fpu(tsk);
+ fpu_reset_state(tsk);
return 0;
}
@@ -416,7 +416,7 @@ int __restore_xstate_sig(void __user *buf, void __user
*buf_fx, int size)
*/
user_fpu_begin();
if (restore_user_xstate(buf_fx, xstate_bv, fx_only)) {
- drop_init_fpu(tsk);
+ fpu_reset_state(tsk);
return -1;
}
}
--
2.3.3
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
* Borislav Petkov b...@suse.de wrote: On Sun, Mar 15, 2015 at 09:38:16PM +0100, Borislav Petkov wrote: How about we call this function fpu_reset_state() instead? IOW, something like this. Reading the usage sites actually make much more sense to me now. It could be just me though... :-) --- From: Borislav Petkov b...@suse.de Date: Mon, 16 Mar 2015 10:21:55 +0100 Subject: [PATCH] x86/fpu: Rename drop_init_fpu() to fpu_reset_state() Call it what it does and in accordance with the context where it is used: we reset the FPU state either because we were unable to restore it from the one saved in the task or because we simply want to reset it. Acked-by: Ingo Molnar mi...@kernel.org Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sun, Mar 15, 2015 at 09:38:16PM +0100, Borislav Petkov wrote:
How about we call this function fpu_reset_state() instead?
IOW, something like this. Reading the usage sites actually make much
more sense to me now. It could be just me though...
:-)
---
From: Borislav Petkov b...@suse.de
Date: Mon, 16 Mar 2015 10:21:55 +0100
Subject: [PATCH] x86/fpu: Rename drop_init_fpu() to fpu_reset_state()
Call it what it does and in accordance with the context where it is
used: we reset the FPU state either because we were unable to restore it
from the one saved in the task or because we simply want to reset it.
Signed-off-by: Borislav Petkov b...@suse.de
Cc: Oleg Nesterov o...@redhat.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Linus Torvalds torva...@linux-foundation.org
Cc: Rik van Riel r...@redhat.com
Cc: Ingo Molnar mi...@kernel.org
---
arch/x86/include/asm/fpu-internal.h | 8 ++--
arch/x86/kernel/i387.c | 2 +-
arch/x86/kernel/signal.c| 2 +-
arch/x86/kernel/traps.c | 2 +-
arch/x86/kernel/xsave.c | 4 ++--
5 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/fpu-internal.h
b/arch/x86/include/asm/fpu-internal.h
index 2d4adff428ac..da5e96756570 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -406,7 +406,11 @@ static inline void restore_init_xstate(void)
fxrstor_checking(init_xstate_buf-i387);
}
-static inline void drop_init_fpu(struct task_struct *tsk)
+/*
+ * Reset the FPU state in the eager case and drop it in the lazy case (later
use
+ * will reinit it).
+ */
+static inline void fpu_reset_state(struct task_struct *tsk)
{
if (!use_eager_fpu())
drop_fpu(tsk);
@@ -480,7 +484,7 @@ 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);
+ fpu_reset_state(new);
}
}
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 29e982ada854..41575b9b1021 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -108,7 +108,7 @@ void __kernel_fpu_end(void)
if (__thread_has_fpu(me)) {
if (WARN_ON(restore_fpu_checking(me)))
- drop_init_fpu(me);
+ fpu_reset_state(me);
} else if (!use_eager_fpu()) {
stts();
}
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index e5042463c1bc..59eaae6185e2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -679,7 +679,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
* Ensure the signal handler starts with the new fpu state.
*/
if (used_math())
- drop_init_fpu(current);
+ fpu_reset_state(current);
}
signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 92b83e299ed3..156d75859466 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -863,7 +863,7 @@ void math_state_restore(void)
kernel_fpu_disable();
__thread_fpu_begin(tsk);
if (unlikely(restore_fpu_checking(tsk))) {
- drop_init_fpu(tsk);
+ fpu_reset_state(tsk);
force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
} else {
tsk-thread.fpu_counter++;
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 0bf82c5ac529..65c29b070e09 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -342,7 +342,7 @@ int __restore_xstate_sig(void __user *buf, void __user
*buf_fx, int size)
config_enabled(CONFIG_IA32_EMULATION));
if (!buf) {
- drop_init_fpu(tsk);
+ fpu_reset_state(tsk);
return 0;
}
@@ -416,7 +416,7 @@ int __restore_xstate_sig(void __user *buf, void __user
*buf_fx, int size)
*/
user_fpu_begin();
if (restore_user_xstate(buf_fx, xstate_bv, fx_only)) {
- drop_init_fpu(tsk);
+ fpu_reset_state(tsk);
return -1;
}
}
--
2.3.3
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Mon, Mar 16, 2015 at 08:34:15AM -0700, Andy Lutomirski wrote: Nice! This is the first time I've actually understood that :). I still have no idea what init referred to... Haha, you're not the only one :-) We're learning as we go. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Mar 16, 2015 2:37 AM, Borislav Petkov b...@suse.de wrote: On Sun, Mar 15, 2015 at 09:38:16PM +0100, Borislav Petkov wrote: How about we call this function fpu_reset_state() instead? IOW, something like this. Reading the usage sites actually make much more sense to me now. It could be just me though... :-) --- From: Borislav Petkov b...@suse.de Date: Mon, 16 Mar 2015 10:21:55 +0100 Subject: [PATCH] x86/fpu: Rename drop_init_fpu() to fpu_reset_state() Call it what it does and in accordance with the context where it is used: we reset the FPU state either because we were unable to restore it from the one saved in the task or because we simply want to reset it. Nice! This is the first time I've actually understood that :). I still have no idea what init referred to... --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/16, Borislav Petkov wrote: -static inline void drop_init_fpu(struct task_struct *tsk) +/* + * Reset the FPU state in the eager case and drop it in the lazy case (later use + * will reinit it). + */ +static inline void fpu_reset_state(struct task_struct *tsk) ACK! Perhaps you can also find a beter name for __save_init_fpu/etc ;) The name clearly suggests that it does save + init while in fact it does save and may be destroy FPU state. At least for the callers, the fact that destroy is actually init doesn't really matter. But lets not rename it right now. This can conflict with the fixes we need to do first. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Mon, Mar 16, 2015 at 03:39:44PM +0100, Oleg Nesterov wrote: ACK! Thanks. Perhaps you can also find a beter name for __save_init_fpu/etc ;) The name clearly suggests that it does save + init while in fact it does save and may be destroy FPU state. At least for the callers, the fact that destroy is actually init doesn't really matter. But lets not rename it right now. This can conflict with the fixes we need to do first. Right, so I think we should do fixes/cleanups first so that we can lose all the fat/cruft this code has grown. I'll make looking at that code easier later. I'll push out everything I have collected so far for people to see after I've finished bisecting another tip/master regression from today. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sun, Mar 15, 2015 at 09:04:36PM +0100, Oleg Nesterov wrote: > But please note that it is not only used after the failure. > See handle_signal() and the first drop_init_fpu() in > __restore_xstate_sig(). Yeah, that's why I said "In the most places it is used..." > I think its name is confusing a bit... Yeah, the "init" aspect affects only the eager case... How about we call this function fpu_reset_state() instead? This way it doesn't really need to be documented what it does - it simply resets the FPU state. And resetting is what we do in all call sites so the usage dictates the name and then "drop" can be differentiated from "reset" as "drop" is only a part of the "reset" operation on an FPU state. And so on and so on... Hmmm? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/15, Borislav Petkov wrote:
>
> On Sun, Mar 15, 2015 at 07:16:43PM +0100, Oleg Nesterov wrote:
>
> Anyway, we're on the same page and that was a good exercise :-)
Yes, finally ;)
> +/*
> + * In addition to "forgetting" FPU state for @tsk, we restore the
> + * default FPU state in the eager case. Note, this is not needed in the
> + * non-eager case because there we will set CR0.TS and fault and setup
> + * an FPU state lazily.
> + *
> + * We restore the default FPU state in the eager case here as a means of
> + * addressing the failure of restoring the FPU state which @tsk points
> + * to and we still need some state to use so we use the default, clean
> + * one.
> + */
> static inline void drop_init_fpu(struct task_struct *tsk)
> {
> if (!use_eager_fpu())
But please note that it is not only used after the failure.
See handle_signal() and the first drop_init_fpu() in __restore_xstate_sig().
I think its name is confusing a bit...
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sun, Mar 15, 2015 at 07:16:43PM +0100, Oleg Nesterov wrote:
> Of course, drop_init_fpu() is fine if restore_fpu_checking() fails.
>
> Did you mean this from the very beginning? In this case I agree of course.
>
> Because I misinterpreted your initial comment:
>
> One example where drop_init_fpu() seems to make sense is
> __kernel_fpu_end(): kernel is done with FPU and current was using the
> FPU prior so let's restore it for the eagerfpu case.
>
> as if you suggest to use it _instead_ of restore_fpu_checking().
Nah, not "instead" - I didn't express myself precisely enough. I was
trying to think out loud and look for an example where drop_init_fpu()
would make sense.
In most of the places it is used, it is in the error path of restoring
the FPU state, i.e. we were unable to restore for some reason, let's
reinit instead of just drop only, in the eager case.
And your patch correctly removed it from flush_thread() where it didn't
make any sense except to cause CPUs to get needlessly warmer.
Anyway, we're on the same page and that was a good exercise :-)
Thanks Oleg!
Btw, we probably should start documenting stuff like that so that we
don't have to re-fault all that info 6 months/a year from now when we
have to touch that code again. Hmm, how about something like this:
---
diff --git a/arch/x86/include/asm/fpu-internal.h
b/arch/x86/include/asm/fpu-internal.h
index 2d4adff428ac..996f20a31f0a 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -406,6 +406,17 @@ static inline void restore_init_xstate(void)
fxrstor_checking(_xstate_buf->i387);
}
+/*
+ * In addition to "forgetting" FPU state for @tsk, we restore the
+ * default FPU state in the eager case. Note, this is not needed in the
+ * non-eager case because there we will set CR0.TS and fault and setup
+ * an FPU state lazily.
+ *
+ * We restore the default FPU state in the eager case here as a means of
+ * addressing the failure of restoring the FPU state which @tsk points
+ * to and we still need some state to use so we use the default, clean
+ * one.
+ */
static inline void drop_init_fpu(struct task_struct *tsk)
{
if (!use_eager_fpu())
---
?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/15, Borislav Petkov wrote:
>
> On Sat, Mar 14, 2015 at 03:48:16PM +0100, Oleg Nesterov wrote:
> > > >
> > > > __kernel_fpu_end() needs to restore FPU from current's fpu->state
> > > > exactly
> > > > because current used FPU prior. And that state was saved by
> > > > __save_init_fpu()
> > > > in __kernel_fpu_begin().
> > >
> > > That's exactly what I mean. See: "... kernel is done with FPU and current
> > > was
> > > using the FPU prior..."
> >
> > Yes, but my point was that this is why we can _not_ use drop_init_fpu() in
> > __kernel_fpu_end().
>
> Hmm, now I'm confused.
Me too...
> void __kernel_fpu_end():
>
> ...
>
> if (__thread_has_fpu(me)) {
> if (WARN_ON(restore_fpu_checking(me)))
>
> restore_fpu_checking(current) does try to restore fpu->state and it does
> drop_init_fpu() only if it failed.
>
> Ok, now you tell me what I'm missing :)
Of course, drop_init_fpu() is fine if restore_fpu_checking() fails.
Did you mean this from the very beginning? In this case I agree of course.
Because I misinterpreted your initial comment:
One example where drop_init_fpu() seems to make sense is
__kernel_fpu_end(): kernel is done with FPU and current was using the
FPU prior so let's restore it for the eagerfpu case.
as if you suggest to use it _instead_ of restore_fpu_checking().
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sat, Mar 14, 2015 at 03:48:16PM +0100, Oleg Nesterov wrote:
> On 03/13, Borislav Petkov wrote:
> > On Fri, Mar 13, 2015 at 05:26:54PM +0100, Oleg Nesterov wrote:
> > > > One example where drop_init_fpu() seems to make sense is
> > > > __kernel_fpu_end(): kernel is done with FPU and current was using the
> > > > FPU prior so let's restore it for the eagerfpu case.
> > >
> > > No, no, this is another case or I misunderstood you.
> > >
> > > __kernel_fpu_end() needs to restore FPU from current's fpu->state exactly
> > > because current used FPU prior. And that state was saved by
> > > __save_init_fpu()
> > > in __kernel_fpu_begin().
> >
> > That's exactly what I mean. See: "... kernel is done with FPU and current
> > was
> > using the FPU prior..."
>
> Yes, but my point was that this is why we can _not_ use drop_init_fpu() in
> __kernel_fpu_end().
Hmm, now I'm confused. So __kernel_fpu_end() says kernel finished using
the FPU and we need to do the following:
* current has the FPU => let's restore it. If there was an error doing
that, we do drop_init, i.e. restore init_xstate in the eager case and
otherwise we just drop it. So that makes perfect sense to me.
* otherwise, current didn't have the FPU, we simply set CR0.TS in the
non-eager case so that we can fault on the next use of an FPU insn.
To address your comment from earlier:
> > > __kernel_fpu_end() needs to restore FPU from current's fpu->state exactly
> > > because current used FPU prior. And that state was saved by
> > > __save_init_fpu()
> > > in __kernel_fpu_begin().
And we do that:
void __kernel_fpu_end():
...
if (__thread_has_fpu(me)) {
if (WARN_ON(restore_fpu_checking(me)))
restore_fpu_checking(current) does try to restore fpu->state and it does
drop_init_fpu() only if it failed.
Ok, now you tell me what I'm missing :)
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/15, Borislav Petkov wrote:
On Sun, Mar 15, 2015 at 07:16:43PM +0100, Oleg Nesterov wrote:
Anyway, we're on the same page and that was a good exercise :-)
Yes, finally ;)
+/*
+ * In addition to forgetting FPU state for @tsk, we restore the
+ * default FPU state in the eager case. Note, this is not needed in the
+ * non-eager case because there we will set CR0.TS and fault and setup
+ * an FPU state lazily.
+ *
+ * We restore the default FPU state in the eager case here as a means of
+ * addressing the failure of restoring the FPU state which @tsk points
+ * to and we still need some state to use so we use the default, clean
+ * one.
+ */
static inline void drop_init_fpu(struct task_struct *tsk)
{
if (!use_eager_fpu())
But please note that it is not only used after the failure.
See handle_signal() and the first drop_init_fpu() in __restore_xstate_sig().
I think its name is confusing a bit...
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/15, Borislav Petkov wrote:
On Sat, Mar 14, 2015 at 03:48:16PM +0100, Oleg Nesterov wrote:
__kernel_fpu_end() needs to restore FPU from current's fpu-state
exactly
because current used FPU prior. And that state was saved by
__save_init_fpu()
in __kernel_fpu_begin().
That's exactly what I mean. See: ... kernel is done with FPU and current
was
using the FPU prior...
Yes, but my point was that this is why we can _not_ use drop_init_fpu() in
__kernel_fpu_end().
Hmm, now I'm confused.
Me too...
void __kernel_fpu_end():
...
if (__thread_has_fpu(me)) {
if (WARN_ON(restore_fpu_checking(me)))
restore_fpu_checking(current) does try to restore fpu-state and it does
drop_init_fpu() only if it failed.
Ok, now you tell me what I'm missing :)
Of course, drop_init_fpu() is fine if restore_fpu_checking() fails.
Did you mean this from the very beginning? In this case I agree of course.
Because I misinterpreted your initial comment:
One example where drop_init_fpu() seems to make sense is
__kernel_fpu_end(): kernel is done with FPU and current was using the
FPU prior so let's restore it for the eagerfpu case.
as if you suggest to use it _instead_ of restore_fpu_checking().
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sun, Mar 15, 2015 at 07:16:43PM +0100, Oleg Nesterov wrote:
Of course, drop_init_fpu() is fine if restore_fpu_checking() fails.
Did you mean this from the very beginning? In this case I agree of course.
Because I misinterpreted your initial comment:
One example where drop_init_fpu() seems to make sense is
__kernel_fpu_end(): kernel is done with FPU and current was using the
FPU prior so let's restore it for the eagerfpu case.
as if you suggest to use it _instead_ of restore_fpu_checking().
Nah, not instead - I didn't express myself precisely enough. I was
trying to think out loud and look for an example where drop_init_fpu()
would make sense.
In most of the places it is used, it is in the error path of restoring
the FPU state, i.e. we were unable to restore for some reason, let's
reinit instead of just drop only, in the eager case.
And your patch correctly removed it from flush_thread() where it didn't
make any sense except to cause CPUs to get needlessly warmer.
Anyway, we're on the same page and that was a good exercise :-)
Thanks Oleg!
Btw, we probably should start documenting stuff like that so that we
don't have to re-fault all that info 6 months/a year from now when we
have to touch that code again. Hmm, how about something like this:
---
diff --git a/arch/x86/include/asm/fpu-internal.h
b/arch/x86/include/asm/fpu-internal.h
index 2d4adff428ac..996f20a31f0a 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -406,6 +406,17 @@ static inline void restore_init_xstate(void)
fxrstor_checking(init_xstate_buf-i387);
}
+/*
+ * In addition to forgetting FPU state for @tsk, we restore the
+ * default FPU state in the eager case. Note, this is not needed in the
+ * non-eager case because there we will set CR0.TS and fault and setup
+ * an FPU state lazily.
+ *
+ * We restore the default FPU state in the eager case here as a means of
+ * addressing the failure of restoring the FPU state which @tsk points
+ * to and we still need some state to use so we use the default, clean
+ * one.
+ */
static inline void drop_init_fpu(struct task_struct *tsk)
{
if (!use_eager_fpu())
---
?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sun, Mar 15, 2015 at 09:04:36PM +0100, Oleg Nesterov wrote: But please note that it is not only used after the failure. See handle_signal() and the first drop_init_fpu() in __restore_xstate_sig(). Yeah, that's why I said In the most places it is used... I think its name is confusing a bit... Yeah, the init aspect affects only the eager case... How about we call this function fpu_reset_state() instead? This way it doesn't really need to be documented what it does - it simply resets the FPU state. And resetting is what we do in all call sites so the usage dictates the name and then drop can be differentiated from reset as drop is only a part of the reset operation on an FPU state. And so on and so on... Hmmm? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Sat, Mar 14, 2015 at 03:48:16PM +0100, Oleg Nesterov wrote:
On 03/13, Borislav Petkov wrote:
On Fri, Mar 13, 2015 at 05:26:54PM +0100, Oleg Nesterov wrote:
One example where drop_init_fpu() seems to make sense is
__kernel_fpu_end(): kernel is done with FPU and current was using the
FPU prior so let's restore it for the eagerfpu case.
No, no, this is another case or I misunderstood you.
__kernel_fpu_end() needs to restore FPU from current's fpu-state exactly
because current used FPU prior. And that state was saved by
__save_init_fpu()
in __kernel_fpu_begin().
That's exactly what I mean. See: ... kernel is done with FPU and current
was
using the FPU prior...
Yes, but my point was that this is why we can _not_ use drop_init_fpu() in
__kernel_fpu_end().
Hmm, now I'm confused. So __kernel_fpu_end() says kernel finished using
the FPU and we need to do the following:
* current has the FPU = let's restore it. If there was an error doing
that, we do drop_init, i.e. restore init_xstate in the eager case and
otherwise we just drop it. So that makes perfect sense to me.
* otherwise, current didn't have the FPU, we simply set CR0.TS in the
non-eager case so that we can fault on the next use of an FPU insn.
To address your comment from earlier:
__kernel_fpu_end() needs to restore FPU from current's fpu-state exactly
because current used FPU prior. And that state was saved by
__save_init_fpu()
in __kernel_fpu_begin().
And we do that:
void __kernel_fpu_end():
...
if (__thread_has_fpu(me)) {
if (WARN_ON(restore_fpu_checking(me)))
restore_fpu_checking(current) does try to restore fpu-state and it does
drop_init_fpu() only if it failed.
Ok, now you tell me what I'm missing :)
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/13, Borislav Petkov wrote: > > On Fri, Mar 13, 2015 at 05:26:54PM +0100, Oleg Nesterov wrote: > > > One example where drop_init_fpu() seems to make sense is > > > __kernel_fpu_end(): kernel is done with FPU and current was using the > > > FPU prior so let's restore it for the eagerfpu case. > > > > No, no, this is another case or I misunderstood you. > > > > __kernel_fpu_end() needs to restore FPU from current's fpu->state exactly > > because current used FPU prior. And that state was saved by > > __save_init_fpu() > > in __kernel_fpu_begin(). > > That's exactly what I mean. See: "... kernel is done with FPU and current was > using the FPU prior..." Yes, but my point was that this is why we can _not_ use drop_init_fpu() in __kernel_fpu_end(). Nevermind, look like I really misunderstood you. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/13, Borislav Petkov wrote: On Fri, Mar 13, 2015 at 05:26:54PM +0100, Oleg Nesterov wrote: One example where drop_init_fpu() seems to make sense is __kernel_fpu_end(): kernel is done with FPU and current was using the FPU prior so let's restore it for the eagerfpu case. No, no, this is another case or I misunderstood you. __kernel_fpu_end() needs to restore FPU from current's fpu-state exactly because current used FPU prior. And that state was saved by __save_init_fpu() in __kernel_fpu_begin(). That's exactly what I mean. See: ... kernel is done with FPU and current was using the FPU prior... Yes, but my point was that this is why we can _not_ use drop_init_fpu() in __kernel_fpu_end(). Nevermind, look like I really misunderstood you. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Fri, Mar 13, 2015 at 05:26:54PM +0100, Oleg Nesterov wrote: > > One example where drop_init_fpu() seems to make sense is > > __kernel_fpu_end(): kernel is done with FPU and current was using the > > FPU prior so let's restore it for the eagerfpu case. > > No, no, this is another case or I misunderstood you. > > __kernel_fpu_end() needs to restore FPU from current's fpu->state exactly > because current used FPU prior. And that state was saved by __save_init_fpu() > in __kernel_fpu_begin(). That's exactly what I mean. See: "... kernel is done with FPU and current was using the FPU prior..." :-D -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/13, Borislav Petkov wrote: > > On Fri, Mar 13, 2015 at 03:55:42PM +0100, Oleg Nesterov wrote: > > But please look at drop_init_fpu(). If eagerfpu == F it calls drop_fpu() and > > this is what we need. flush_thread() already has the "if > > (!use_eager_fpu())", > > we can shift drop_fpu() there. > > > > Otherwise, if eagerfpu == T, drop_init_fpu() does restore_init_xstate() and > > this just burns CPU. Until flush_thread user_has_fpu/used_math this state > > restore_init_xstate() is pointless, this state will be lost after > > preemption. > > Yeah, I was wondering why that's there. > > One example where drop_init_fpu() seems to make sense is > __kernel_fpu_end(): kernel is done with FPU and current was using the > FPU prior so let's restore it for the eagerfpu case. No, no, this is another case or I misunderstood you. __kernel_fpu_end() needs to restore FPU from current's fpu->state exactly because current used FPU prior. And that state was saved by __save_init_fpu() in __kernel_fpu_begin(). Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Fri, Mar 13, 2015 at 03:55:42PM +0100, Oleg Nesterov wrote: > But please look at drop_init_fpu(). If eagerfpu == F it calls drop_fpu() and > this is what we need. flush_thread() already has the "if (!use_eager_fpu())", > we can shift drop_fpu() there. > > Otherwise, if eagerfpu == T, drop_init_fpu() does restore_init_xstate() and > this just burns CPU. Until flush_thread user_has_fpu/used_math this state > restore_init_xstate() is pointless, this state will be lost after preemption. Yeah, I was wondering why that's there. One example where drop_init_fpu() seems to make sense is __kernel_fpu_end(): kernel is done with FPU and current was using the FPU prior so let's restore it for the eagerfpu case. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/13, Borislav Petkov wrote:
>
> On Wed, Mar 11, 2015 at 06:35:07PM +0100, Oleg Nesterov wrote:
> > drop_init_fpu() makes no sense. We need drop_fpu() and only if
>
> Oh, please explain why. I can try to rhyme it up as something like "we
> don't need to restore FPU context when flushing the thread" but I'm not
> sure...
Hmm. The changelog could be more clear. I'll send v2.
But please look at drop_init_fpu(). If eagerfpu == F it calls drop_fpu() and
this is what we need. flush_thread() already has the "if (!use_eager_fpu())",
we can shift drop_fpu() there.
Otherwise, if eagerfpu == T, drop_init_fpu() does restore_init_xstate() and
this just burns CPU. Until flush_thread user_has_fpu/used_math this state
restore_init_xstate() is pointless, this state will be lost after preemption.
> > + } else if (!used_math()) {
> > /* kthread execs. TODO: cleanup this horror. */
> > if (WARN_ON(init_fpu(current)))
> > force_sig(SIGKILL, current);
>
> Also, can we clean up the tsk/current usage here?
>
> We assign current to tsk and we work with it but then later use current
> again. Needlessly confusing.
Agreed, will do.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Wed, Mar 11, 2015 at 06:35:07PM +0100, Oleg Nesterov wrote:
> drop_init_fpu() makes no sense. We need drop_fpu() and only if
Oh, please explain why. I can try to rhyme it up as something like "we
don't need to restore FPU context when flushing the thread" but I'm not
sure...
> !use_eager_fpu().
>
> Signed-off-by: Oleg Nesterov
> ---
> arch/x86/kernel/process.c | 11 ---
> 1 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index c396de2..2e71120 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -131,14 +131,11 @@ void flush_thread(void)
> flush_ptrace_hw_breakpoint(tsk);
> memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
>
> - drop_init_fpu(tsk);
> - /*
> - * Free the FPU state for non xsave platforms. They get reallocated
> - * lazily at the first use.
> - */
> - if (!use_eager_fpu())
> + if (!use_eager_fpu()) {
> + /* FPU state will be reallocated lazily at the first use. */
> + drop_fpu(tsk);
> free_thread_xstate(tsk);
> - else if (!used_math()) {
> + } else if (!used_math()) {
> /* kthread execs. TODO: cleanup this horror. */
> if (WARN_ON(init_fpu(current)))
> force_sig(SIGKILL, current);
Also, can we clean up the tsk/current usage here?
We assign current to tsk and we work with it but then later use current
again. Needlessly confusing.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Fri, Mar 13, 2015 at 05:26:54PM +0100, Oleg Nesterov wrote: One example where drop_init_fpu() seems to make sense is __kernel_fpu_end(): kernel is done with FPU and current was using the FPU prior so let's restore it for the eagerfpu case. No, no, this is another case or I misunderstood you. __kernel_fpu_end() needs to restore FPU from current's fpu-state exactly because current used FPU prior. And that state was saved by __save_init_fpu() in __kernel_fpu_begin(). That's exactly what I mean. See: ... kernel is done with FPU and current was using the FPU prior... :-D -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Wed, Mar 11, 2015 at 06:35:07PM +0100, Oleg Nesterov wrote:
drop_init_fpu() makes no sense. We need drop_fpu() and only if
Oh, please explain why. I can try to rhyme it up as something like we
don't need to restore FPU context when flushing the thread but I'm not
sure...
!use_eager_fpu().
Signed-off-by: Oleg Nesterov o...@redhat.com
---
arch/x86/kernel/process.c | 11 ---
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c396de2..2e71120 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -131,14 +131,11 @@ void flush_thread(void)
flush_ptrace_hw_breakpoint(tsk);
memset(tsk-thread.tls_array, 0, sizeof(tsk-thread.tls_array));
- drop_init_fpu(tsk);
- /*
- * Free the FPU state for non xsave platforms. They get reallocated
- * lazily at the first use.
- */
- if (!use_eager_fpu())
+ if (!use_eager_fpu()) {
+ /* FPU state will be reallocated lazily at the first use. */
+ drop_fpu(tsk);
free_thread_xstate(tsk);
- else if (!used_math()) {
+ } else if (!used_math()) {
/* kthread execs. TODO: cleanup this horror. */
if (WARN_ON(init_fpu(current)))
force_sig(SIGKILL, current);
Also, can we clean up the tsk/current usage here?
We assign current to tsk and we work with it but then later use current
again. Needlessly confusing.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/13, Borislav Petkov wrote:
On Wed, Mar 11, 2015 at 06:35:07PM +0100, Oleg Nesterov wrote:
drop_init_fpu() makes no sense. We need drop_fpu() and only if
Oh, please explain why. I can try to rhyme it up as something like we
don't need to restore FPU context when flushing the thread but I'm not
sure...
Hmm. The changelog could be more clear. I'll send v2.
But please look at drop_init_fpu(). If eagerfpu == F it calls drop_fpu() and
this is what we need. flush_thread() already has the if (!use_eager_fpu()),
we can shift drop_fpu() there.
Otherwise, if eagerfpu == T, drop_init_fpu() does restore_init_xstate() and
this just burns CPU. Until flush_thread user_has_fpu/used_math this state
restore_init_xstate() is pointless, this state will be lost after preemption.
+ } else if (!used_math()) {
/* kthread execs. TODO: cleanup this horror. */
if (WARN_ON(init_fpu(current)))
force_sig(SIGKILL, current);
Also, can we clean up the tsk/current usage here?
We assign current to tsk and we work with it but then later use current
again. Needlessly confusing.
Agreed, will do.
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On 03/13, Borislav Petkov wrote: On Fri, Mar 13, 2015 at 03:55:42PM +0100, Oleg Nesterov wrote: But please look at drop_init_fpu(). If eagerfpu == F it calls drop_fpu() and this is what we need. flush_thread() already has the if (!use_eager_fpu()), we can shift drop_fpu() there. Otherwise, if eagerfpu == T, drop_init_fpu() does restore_init_xstate() and this just burns CPU. Until flush_thread user_has_fpu/used_math this state restore_init_xstate() is pointless, this state will be lost after preemption. Yeah, I was wondering why that's there. One example where drop_init_fpu() seems to make sense is __kernel_fpu_end(): kernel is done with FPU and current was using the FPU prior so let's restore it for the eagerfpu case. No, no, this is another case or I misunderstood you. __kernel_fpu_end() needs to restore FPU from current's fpu-state exactly because current used FPU prior. And that state was saved by __save_init_fpu() in __kernel_fpu_begin(). Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
On Fri, Mar 13, 2015 at 03:55:42PM +0100, Oleg Nesterov wrote: But please look at drop_init_fpu(). If eagerfpu == F it calls drop_fpu() and this is what we need. flush_thread() already has the if (!use_eager_fpu()), we can shift drop_fpu() there. Otherwise, if eagerfpu == T, drop_init_fpu() does restore_init_xstate() and this just burns CPU. Until flush_thread user_has_fpu/used_math this state restore_init_xstate() is pointless, this state will be lost after preemption. Yeah, I was wondering why that's there. One example where drop_init_fpu() seems to make sense is __kernel_fpu_end(): kernel is done with FPU and current was using the FPU prior so let's restore it for the eagerfpu case. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
drop_init_fpu() makes no sense. We need drop_fpu() and only if
!use_eager_fpu().
Signed-off-by: Oleg Nesterov
---
arch/x86/kernel/process.c | 11 ---
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c396de2..2e71120 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -131,14 +131,11 @@ void flush_thread(void)
flush_ptrace_hw_breakpoint(tsk);
memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
- drop_init_fpu(tsk);
- /*
-* Free the FPU state for non xsave platforms. They get reallocated
-* lazily at the first use.
-*/
- if (!use_eager_fpu())
+ if (!use_eager_fpu()) {
+ /* FPU state will be reallocated lazily at the first use. */
+ drop_fpu(tsk);
free_thread_xstate(tsk);
- else if (!used_math()) {
+ } else if (!used_math()) {
/* kthread execs. TODO: cleanup this horror. */
if (WARN_ON(init_fpu(current)))
force_sig(SIGKILL, current);
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread()
drop_init_fpu() makes no sense. We need drop_fpu() and only if
!use_eager_fpu().
Signed-off-by: Oleg Nesterov o...@redhat.com
---
arch/x86/kernel/process.c | 11 ---
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c396de2..2e71120 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -131,14 +131,11 @@ void flush_thread(void)
flush_ptrace_hw_breakpoint(tsk);
memset(tsk-thread.tls_array, 0, sizeof(tsk-thread.tls_array));
- drop_init_fpu(tsk);
- /*
-* Free the FPU state for non xsave platforms. They get reallocated
-* lazily at the first use.
-*/
- if (!use_eager_fpu())
+ if (!use_eager_fpu()) {
+ /* FPU state will be reallocated lazily at the first use. */
+ drop_fpu(tsk);
free_thread_xstate(tsk);
- else if (!used_math()) {
+ } else if (!used_math()) {
/* kthread execs. TODO: cleanup this horror. */
if (WARN_ON(init_fpu(current)))
force_sig(SIGKILL, current);
--
1.5.5.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
