x86/fpu: Reduce unnecessary FNINIT and MXCSR usage

arch/x86/kernel/fpu/core.c +3 -3

index eb86a2b831b1..d4a71596c41e 100644
@@ -121,7 +121,7 @@ int copy_fpregs_to_fpstate(struct fpu *fpu)
 }
 EXPORT_SYMBOL(copy_fpregs_to_fpstate);
 
-void kernel_fpu_begin(void)
+void kernel_fpu_begin_mask(unsigned int kfpu_mask)
 {
 	preempt_disable();
 

@@ -141,13 +141,18 @@ void kernel_fpu_begin(void)
 	}
 	__cpu_invalidate_fpregs_state();
 
-	if (boot_cpu_has(X86_FEATURE_XMM))
-		ldmxcsr(MXCSR_DEFAULT);
+	/* Put sane initial values into the control registers. */
+	if (likely(kfpu_mask & KFPU_MXCSR)) {

arch/x86/kernel/fpu/core.c +1 -1

 }
-EXPORT_SYMBOL_GPL(kernel_fpu_begin);
+EXPORT_SYMBOL_GPL(kernel_fpu_begin_mask);
 
 void kernel_fpu_end(void)
 {

Signature · 1 line

2.29.2

This series fixes two regressions: a boot failure on AMD K7 and a performance regression on everything.

I did a double-take here -- the regressions were reported by different people, both named Krzysztof :)

Changes from v1:
- Fix MMX better -- MMX really does need FNINIT.
- Improve the EFI code.
- Rename the KFPU constants.
- Changelog improvements.

Andy Lutomirski (4):
x86/fpu: Add kernel_fpu_begin_mask() to selectively initialize state x86/mmx: Use KFPU_387 for MMX string operations
x86/fpu: Make the EFI FPU calling convention explicit x86/fpu/64: Don't FNINIT in kernel_fpu_begin()

The remaining callers of kernel_fpu_begin() in 64-bit kernels don't use 387 instructions, so there's no need to sanitize the FPU state. Skip it to get most of the performance we lost back.

Reported-by Krzysztof Olędzki <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/fpu/api.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/x86/include/asm/fpu/api.h +0 -0

index 38f4936045ab..435bc59d539b 100644

Currently, requesting kernel FPU access doesn't distinguish which parts of the extended ("FPU") state are needed. This is nice for simplicity, but there are a few cases in which it's suboptimal:

- The vast majority of in-kernel FPU users want XMM/YMM/ZMM state but do not use legacy 387 state. These users want MXCSR initialized but don't care about the FPU control word. Skipping FNINIT would save time. (Empirically, FNINIT is several times slower than LDMXCSR.)

- Code that wants MMX doesn't want or need MXCSR initialized. _mmx_memcpy(), for example, can run before CR4.OSFXSR gets set, and initializing MXCSR will fail.

- Any future in-kernel users of XFD (eXtended Feature Disable)-capable dynamic states will need special handling.

This patch adds a more specific API that allows callers specify exactly what they want.

Quoted 16 lines · Andy Lutomirski

Currently, requesting kernel FPU access doesn't distinguish which parts of the extended ("FPU") state are needed. This is nice for simplicity, but there are a few cases in which it's suboptimal:

- The vast majority of in-kernel FPU users want XMM/YMM/ZMM state but do not use legacy 387 state. These users want MXCSR initialized but don't care about the FPU control word. Skipping FNINIT would save time. (Empirically, FNINIT is several times slower than LDMXCSR.)

- Code that wants MMX doesn't want or need MXCSR initialized. _mmx_memcpy(), for example, can run before CR4.OSFXSR gets set, and initializing MXCSR will fail.

- Any future in-kernel users of XFD (eXtended Feature Disable)-capable dynamic states will need special handling.

Quoted 2 lines

This patch adds a more specific API that allows callers specify exactly what they want.

Same nitpicks:

https://lkml.kernel.org/r/[email protected]

Signature · 5 lines

-- 
Regards/Gruss,
   Boris.

https://people.kernel.org/tglx/notes-about-netiquette

The default kernel_fpu_begin() doesn't work on systems that support XMM but haven't yet enabled CR4.OSFXSR. This causes crashes when _mmx_memcpy() is called too early because LDMXCSR generates #UD when the aforementioned bit is clear.

Fix it by using kernel_fpu_begin_mask(KFPU_387) explicitly.

Fixes 7ad816762f9b ("x86/fpu: Reset MXCSR to default in kernel_fpu_begin()")

Reported-by Krzysztof Mazur <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/lib/mmx_32.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

arch/x86/lib/mmx_32.c +0 -0

index 4321fa02e18d..2a6ad7aa148a 100644
@@ -26,6 +26,16 @@
 #include <asm/fpu/api.h>

Quoted 15 lines · Andy Lutomirski

The remaining callers of kernel_fpu_begin() in 64-bit kernels don't use 387 instructions, so there's no need to sanitize the FPU state. Skip it to get most of the performance we lost back.

Reported-by Krzysztof Olędzki <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/fpu/api.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/x86/include/asm/fpu/api.h +0 -0

index 38f4936045ab..435bc59d539b 100644

arch/x86/include/asm/fpu/api.h extern void fpregs_mark_activate(void); · line 40

Quoted 9 lines

arch/x86/include/asm/fpu/api.h +6 -0

@@ -32,7 +32,19 @@ extern void fpregs_mark_activate(void);
 /* Code that is unaware of kernel_fpu_begin_mask() can use this */
 static inline void kernel_fpu_begin(void)
 {
+#ifdef CONFIG_X86_64
+	/*
+	 * Any 64-bit code that uses 387 instructions must explicitly request
+	 * KFPU_387.
+	 */
+	kernel_fpu_begin_mask(KFPU_MXCSR);

I'm also still sitting on this:

git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/fpu

what do we do with that?

Quoted 12 lines · Andy Lutomirski

This series fixes two regressions: a boot failure on AMD K7 and a performance regression on everything.

I did a double-take here -- the regressions were reported by different people, both named Krzysztof :)

Changes from v1:
- Fix MMX better -- MMX really does need FNINIT.
- Improve the EFI code.
- Rename the KFPU constants.
- Changelog improvements.

Quoted 5 lines

Andy Lutomirski (4):
x86/fpu: Add kernel_fpu_begin_mask() to selectively initialize state x86/mmx: Use KFPU_387 for MMX string operations
x86/fpu: Make the EFI FPU calling convention explicit x86/fpu/64: Don't FNINIT in kernel_fpu_begin()

Hi Andy.

I have quickly tested the new version on E3-1280 V2.

* 5.10.9 + 7ad816762f9bf89e940e618ea40c43138b479e10 reverted (aka unfixed)
xor: measuring software checksum speed

    avx             : 38616 MB/sec
    prefetch64-sse  : 25797 MB/sec
    generic_sse     : 23147 MB/sec

xor: using function: avx (38616 MB/sec)

* 5.10.9 (the original)
xor: measuring software checksum speed

    avx             : 23318 MB/sec
    prefetch64-sse  : 22562 MB/sec
    generic_sse     : 20431 MB/sec

xor: using function: avx (23318 MB/sec)

* 5.10.9 + "Reduce unnecessary FNINIT and MXCSR usage" v2
xor: measuring software checksum speed

    avx             : 26451 MB/sec
    prefetch64-sse  : 25777 MB/sec
    generic_sse     : 23178 MB/sec

xor: using function: avx (26451 MB/sec)

Overall, kernel xor benchmark reports better performance on 5.10.9 than on 5.4.90 (see my prev e-mail), but the general trend is the same.

The "unfixed" kernel is much faster for all of avx, prefetch64-sse and generic_sse. With the fix, we see the expected perf regression.

Now, with your patchset, both prefetch64-sse and generic_sse are able to recover the full performance, as seen on 5.4. However, this is not the case for avx. While there is still an improvement, it is nowhere close to where it used to be.

I wonder why AVX still sees a regression and if anything more can be done about it?

Will do more tests tomorrow.