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

Groups > linux.kernel > #1567334 > unrolled thread

[PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active Ingo Molnar <mingo@kernel.org> - 2017-01-26 12:30 +0100
    Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to  fpu->fpstate_active Rik van Riel <riel@redhat.com> - 2017-01-26 15:50 +0100
      Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to  fpu->fpstate_active Ingo Molnar <mingo@kernel.org> - 2017-01-26 16:20 +0100
        Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to  fpu->fpstate_active Rik van Riel <riel@redhat.com> - 2017-01-26 17:30 +0100
          Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to  fpu->fpstate_active Ingo Molnar <mingo@kernel.org> - 2017-01-26 17:30 +0100
            Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active Andy Lutomirski <luto@amacapital.net> - 2017-01-26 18:10 +0100
              Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to  fpu->fpstate_active Rik van Riel <riel@redhat.com> - 2017-01-26 19:10 +0100

#1567334 — [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

We want to simplify the FPU state machine by eliminating fpu->fpregs_active,
and we can do that because the two state flags (::fpregs_active and
::fpstate_active) are set essentially together.

The old lazy FPU switching code used to make a distinction - but there's
no lazy switching code anymore, we always switch in an 'eager' fashion.

Do this by first changing all substantial uses of fpu->fpregs_active
to fpu->fpstate_active and adding a few debug checks to double check
our assumption is correct.

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 |  4 +++-
 arch/x86/kernel/fpu/core.c          | 16 ++++++++++------
 arch/x86/kernel/fpu/signal.c        |  4 +++-
 arch/x86/mm/pkeys.c                 |  3 +--
 4 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index d6307e76837c..3a1cd8344a6b 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -556,7 +556,9 @@ static inline void fpregs_activate(struct fpu *fpu)
 static inline void
 switch_fpu_prepare(struct fpu *old_fpu, int cpu)
 {
-	if (old_fpu->fpregs_active) {
+	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;
 		else
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 2973afdb39f1..4fed80d08bd7 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -100,7 +100,7 @@ void __kernel_fpu_begin(void)
 
 	kernel_fpu_disable();
 
-	if (fpu->fpregs_active) {
+	if (fpu->fpstate_active) {
 		/*
 		 * Ignore return value -- we don't care if reg state
 		 * is clobbered.
@@ -116,7 +116,7 @@ void __kernel_fpu_end(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
-	if (fpu->fpregs_active)
+	if (fpu->fpstate_active)
 		copy_kernel_to_fpregs(&fpu->state);
 
 	kernel_fpu_enable();
@@ -147,8 +147,10 @@ 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->fpregs_active) {
+	if (fpu->fpstate_active) {
 		if (!copy_fpregs_to_fpstate(fpu)) {
 			copy_kernel_to_fpregs(&fpu->state);
 		}
@@ -262,11 +264,12 @@ 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:
 	 */
-	if (fpu->fpregs_active) {
+	if (fpu->fpstate_active) {
 		fpu__save(fpu);
 	} else {
 		if (!fpu->fpstate_active) {
@@ -362,12 +365,13 @@ 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
 	 * an XRSTOR if they are active.
 	 */
-	if (fpu->fpregs_active)
+	if (fpu->fpstate_active)
 		copy_kernel_to_fpregs(&fpu->state);
 
 	/*
@@ -417,7 +421,7 @@ void fpu__drop(struct fpu *fpu)
 	if (fpu == &current->thread.fpu) {
 		WARN_ON_FPU(fpu->fpstate_active != fpu->fpregs_active);
 
-		if (fpu->fpregs_active) {
+		if (fpu->fpstate_active) {
 			/* Ignore delayed exceptions from user space */
 			asm volatile("1: fwait\n"
 				     "2:\n"
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 684025654d0c..a88083ba7f8b 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -171,7 +171,9 @@ 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 (fpu->fpregs_active || using_compacted_format()) {
+	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))
 			return -1;
diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
index e2c23472233e..4d24269c071f 100644
--- a/arch/x86/mm/pkeys.c
+++ b/arch/x86/mm/pkeys.c
@@ -18,7 +18,6 @@
 
 #include <asm/cpufeature.h>             /* boot_cpu_has, ...            */
 #include <asm/mmu_context.h>            /* vma_pkey()                   */
-#include <asm/fpu/internal.h>           /* fpregs_active()              */
 
 int __execute_only_pkey(struct mm_struct *mm)
 {
@@ -45,7 +44,7 @@ int __execute_only_pkey(struct mm_struct *mm)
 	 */
 	preempt_disable();
 	if (!need_to_set_mm_pkey &&
-	    current->thread.fpu.fpregs_active &&
+	    current->thread.fpu.fpstate_active &&
 	    !__pkru_allows_read(read_pkru(), execute_only_pkey)) {
 		preempt_enable();
 		return execute_only_pkey;
-- 
2.7.4

[toc] | [next] | [standalone]

#1567477 — Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
> We want to simplify the FPU state machine by eliminating fpu-
> >fpregs_active,
> and we can do that because the two state flags (::fpregs_active and
> ::fpstate_active) are set essentially together.
> 
> The old lazy FPU switching code used to make a distinction - but
> there's
> no lazy switching code anymore, we always switch in an 'eager'
> fashion.

I've been working for a while now to fix that for
KVM VCPU threads.

Currently when we switch to a VCPU thread, we first
load that thread's userspace FPU context, and then
soon after we save that, and load the guest side FPU
context.

When a VCPU thread goes idle, we also go through
two FPU context transitions.

In order to skip the unnecessary FPU context switches
for VCPU threads, I have been relying on separate
fpstate_active and fpregs_active states.

Do you have any ideas on how I could implement that
kind of change without separate fpstate_active and
fpregs_active states?

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

#1567510 — Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

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

> On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
> > We want to simplify the FPU state machine by eliminating fpu-
> > >fpregs_active,
> > and we can do that because the two state flags (::fpregs_active and
> > ::fpstate_active) are set essentially together.
> > 
> > The old lazy FPU switching code used to make a distinction - but
> > there's
> > no lazy switching code anymore, we always switch in an 'eager'
> > fashion.
> 
> I've been working for a while now to fix that for
> KVM VCPU threads.
> 
> Currently when we switch to a VCPU thread, we first
> load that thread's userspace FPU context, and then
> soon after we save that, and load the guest side FPU
> context.
> 
> When a VCPU thread goes idle, we also go through
> two FPU context transitions.
> 
> In order to skip the unnecessary FPU context switches
> for VCPU threads, I have been relying on separate
> fpstate_active and fpregs_active states.
> 
> Do you have any ideas on how I could implement that
> kind of change without separate fpstate_active and
> fpregs_active states?

So the vCPU threads have host side FPU (user-space) state - whatever FPU state 
Qemu has?

One solution to that overhead, without complicating the FPU state machine in any 
way, would be to add a facility to drop/reacquire that FPU state.

That should automatically result in zero FPU state switching AFAICS: kernel 
threads don't do FPU state switching either.

The vCPU threads sometimes do return to user-space, when they get some deep 
exception that needs to be handled by Qemu, right? This aspect shouldn't be a big 
problem either, because the regular calling convention is to call (synchronous) 
system calls without holding FPU state, right?

I.e. the vCPU /dev/kvm ioctl() could drop/re-map the FPU state with very little 
overhead (i.e. no full save/restore required in that code path either), when it 
enters/exits vCPU mode.

Thanks,

	Ingo

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

#1567581 — Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

On Thu, 2017-01-26 at 16:16 +0100, Ingo Molnar wrote:
> * Rik van Riel <riel@redhat.com> wrote:
> 
> > On Thu, 2017-01-26 at 12:26 +0100, Ingo Molnar wrote:
> > > We want to simplify the FPU state machine by eliminating fpu-
> > > > fpregs_active,
> > > 
> > > and we can do that because the two state flags (::fpregs_active
> > > and
> > > ::fpstate_active) are set essentially together.
> > > 
> > > The old lazy FPU switching code used to make a distinction - but
> > > there's
> > > no lazy switching code anymore, we always switch in an 'eager'
> > > fashion.
> > 
> > I've been working for a while now to fix that for
> > KVM VCPU threads.
> > 
> > Currently when we switch to a VCPU thread, we first
> > load that thread's userspace FPU context, and then
> > soon after we save that, and load the guest side FPU
> > context.
> > 
> > When a VCPU thread goes idle, we also go through
> > two FPU context transitions.
> > 
> > In order to skip the unnecessary FPU context switches
> > for VCPU threads, I have been relying on separate
> > fpstate_active and fpregs_active states.
> > 
> > Do you have any ideas on how I could implement that
> > kind of change without separate fpstate_active and
> > fpregs_active states?
> 
> So the vCPU threads have host side FPU (user-space) state - whatever
> FPU state 
> Qemu has?

Indeed.

> I.e. the vCPU /dev/kvm ioctl() could drop/re-map the FPU state with
> very little 
> overhead (i.e. no full save/restore required in that code path
> either), when it 
> enters/exits vCPU mode.

Remapping might be best.  If we remap, we do not need to call
kernel_fpu_begin/end around actually going into the guest, and
we can hang onto the guest FPU context while doing stuff inside
the host kernel, even while going to sleep in the host kernel.

Let me go totally reimplement this whole project in a different
way...

At least I found some good FPU bugs and cleanups along the way.

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

#1567586 — Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

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

> Let me go totally reimplement this whole project in a different way...

Note that I can still be convinced about complicating the FPU state machine as 
well if that ends up being the best approach for KVM - but it appears to me (from 
a very superficial look) that turning vCPU threads into no-FPU kthreads or 
representing the guest FPU state directly with the host FPU context would be even 
more beneficial, from the simplicity and KVM performance POV?

> At least I found some good FPU bugs and cleanups along the way.

Absolutely, and your efforts are much appreciated! This ptrace state handling 
madness that bit you on SkyLake was something I missed entirely.

Thanks,

	Ingo

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

#1567610

On Thu, Jan 26, 2017 at 7:53 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Rik van Riel <riel@redhat.com> wrote:
>
>> Let me go totally reimplement this whole project in a different way...
>
> Note that I can still be convinced about complicating the FPU state machine as
> well if that ends up being the best approach for KVM - but it appears to me (from
> a very superficial look) that turning vCPU threads into no-FPU kthreads or
> representing the guest FPU state directly with the host FPU context would be even
> more beneficial, from the simplicity and KVM performance POV?

I may be misunderstanding you, but I don't see how this would work
without getting either messy or slow.

But I think that your series may still be a good base for Rik's work.
With your series applied, there are three possible FPU states: regs
active (regs are in the CPU), regs inactive (in memory), and regs
cached (in memory *and* regs).  What Rik's series does doesn't really
complicate the state machine -- there are still just these three
states.  The difference is that it's possible for the regs to be
inactive or cached even for the current task so long as we're not in
user mode.  The point being that the user vCPU thread can enter the
kernel, get its FPU state inactivated, enter the guest, and reenter
the kernel without reactivating its regs.

Rik, if you think about it that way, does your work map cleanly onto
Ingo's patches?

Ingo, as far as I know, the only serious conceptual complication is
that this change has the potential to interact poorly with PKRU, but
that should be manageable.

--Andy

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

#1567639 — Re: [PATCH 5/7] x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active

On Thu, 2017-01-26 at 09:00 -0800, Andy Lutomirski wrote:
> On Thu, Jan 26, 2017 at 7:53 AM, Ingo Molnar <mingo@kernel.org>
> wrote:
> > 
> > * Rik van Riel <riel@redhat.com> wrote:
> > 
> > > Let me go totally reimplement this whole project in a different
> > > way...
> > 
> > Note that I can still be convinced about complicating the FPU state
> > machine as
> > well if that ends up being the best approach for KVM - but it
> > appears to me (from
> > a very superficial look) that turning vCPU threads into no-FPU
> > kthreads or
> > representing the guest FPU state directly with the host FPU context
> > would be even
> > more beneficial, from the simplicity and KVM performance POV?
> 
> I may be misunderstanding you, but I don't see how this would work
> without getting either messy or slow.
> 
> But I think that your series may still be a good base for Rik's work.
> With your series applied, there are three possible FPU states: regs
> active (regs are in the CPU), regs inactive (in memory), and regs
> cached (in memory *and* regs).  What Rik's series does doesn't really
> complicate the state machine -- there are still just these three
> states.  The difference is that it's possible for the regs to be
> inactive or cached even for the current task so long as we're not in
> user mode.  The point being that the user vCPU thread can enter the
> kernel, get its FPU state inactivated, enter the guest, and reenter
> the kernel without reactivating its regs.
> 
> Rik, if you think about it that way, does your work map cleanly onto
> Ingo's patches?

It does, but the discussion with Ingo also led me to reconsider
an approach I looked at before.

A task could have multiple FPU structures associated with it.
In kvm_vcpu_ioctl(KVM_RUN) we could save the userspace context,
and load the guest FPU context.

Once we are about ready to return to userspace, we can save the
guest FPU context, and load the userspace FPU context.

The only complication is that signal handling and ptrace need
to access the _userspace_ FPU context, even if it is not the
currently used one for the task.

That means we cannot just swap out the contents of
current->thread.fpu, but we need to keep a pointer to the
currently used FPU in current->thread, and have the signal
and ptrace code always work on the userspace FPU data,
which means the in-register data if it is loaded, or the
memory data if it isn't.

On the KVM side, we should be able to drop kernel_fpu_begin
and kernel_fpu_end from entering/leaving the guest. All we
need to swap out in that spot will be the PKRU keys.

The "is the FPU still loaded?" stuff at context switch time
would ensure that guest FPU state loading can be skipped if
all that was run between guest exit and re-entry is kernel
threads.

I suspect this could be slightly lower complexity than the
approach I had been working on, for essentially the same
performance benefit.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web