diff mbox series

[07/11] x86/pkeys: Drop the preempt-disable section

Message ID 20181004140547.13014-8-bigeasy@linutronix.de (mailing list archive)
State New, archived
Headers show
Series x86: load FPU registers on return to userland | expand

Commit Message

Sebastian Andrzej Siewior Oct. 4, 2018, 2:05 p.m. UTC 
From: Rik van Riel <riel@surriel.com>

The fpu->initialized flag should not be changed underneath us. This might be a
fallout during the removal of the LazyFPU support. The FPU is marked
initialized as soon as the state has been set to an initial value. It does not
signal if the CPU's FPU registers are loaded.

Signed-off-by: Rik van Riel <riel@surriel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/mm/pkeys.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Dave Hansen Oct. 12, 2018, 5:58 p.m. UTC | #1 
On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> The fpu->initialized flag should not be changed underneath us. This might be a
> fallout during the removal of the LazyFPU support. The FPU is marked
> initialized as soon as the state has been set to an initial value. It does not
> signal if the CPU's FPU registers are loaded.

If this is true, then the check for fpu->initialized should also go away.

If we were paranoid, we might also add a WARN_ON_ONCE() to make sure our
assumption holds true.
Andy Lutomirski Oct. 12, 2018, 6:07 p.m. UTC | #2 
On Fri, Oct 12, 2018 at 10:58 AM Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> > The fpu->initialized flag should not be changed underneath us. This might be a
> > fallout during the removal of the LazyFPU support. The FPU is marked
> > initialized as soon as the state has been set to an initial value. It does not
> > signal if the CPU's FPU registers are loaded.
>
> If this is true, then the check for fpu->initialized should also go away.
>

All these "if" statements in the FPU code are messy and make
understanding and reviewing this code hard.

Can you prepare a patch for the beginning of your series that removes
fpu->initialized and just ensures that the fpu is always initialized?
This will regress performance, but you should get all that performance
back with TIF_LOAD_FPU.
Sebastian Andrzej Siewior Oct. 12, 2018, 8:26 p.m. UTC | #3 
On 2018-10-12 11:07:28 [-0700], Andy Lutomirski wrote:
> All these "if" statements in the FPU code are messy and make
> understanding and reviewing this code hard.
> 
> Can you prepare a patch for the beginning of your series that removes
> fpu->initialized and just ensures that the fpu is always initialized?
> This will regress performance, but you should get all that performance
> back with TIF_LOAD_FPU.

okay, will do.

Sebastian
diff mbox series

Patch

diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
index a150984171684..eeb03b0f0f514 100644
--- a/arch/x86/mm/pkeys.c
+++ b/arch/x86/mm/pkeys.c
@@ -40,17 +40,13 @@  int __execute_only_pkey(struct mm_struct *mm)
 	 * dance to set PKRU if we do not need to.  Check it
 	 * first and assume that if the execute-only pkey is
 	 * write-disabled that we do not have to set it
-	 * ourselves.  We need preempt off so that nobody
-	 * can make fpregs inactive.
+	 * ourselves.
 	 */
-	preempt_disable();
 	if (!need_to_set_mm_pkey &&
 	    current->thread.fpu.initialized &&
 	    !__pkru_allows_read(read_pkru(), execute_only_pkey)) {
-		preempt_enable();
 		return execute_only_pkey;
 	}
-	preempt_enable();
 
 	/*
 	 * Set up PKRU so that it denies access for everything