Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Linux 5.16 broke FPU handling #13042

Closed
AttilaFueloep opened this issue Jan 30, 2022 · 5 comments · Fixed by #13059
Closed

Linux 5.16 broke FPU handling #13042

AttilaFueloep opened this issue Jan 30, 2022 · 5 comments · Fixed by #13059
Labels
Type: Defect Incorrect behavior (e.g. crash, hang)

Comments

@AttilaFueloep
Copy link
Contributor

System information

Type Version/Name
Distribution Name Arch Linux
Distribution Version rolling
Kernel Version 5.16.3-arch1-1
Architecture x86
OpenZFS Version zfs-2.1.99-717_g73e972af7a, zfs-kmod-2.1.99-1

Describe the problem you're observing

Recent master broke SIMD acceleration for AES-GCM, I suspect the recent ICP changes. Both, pclmulqdq and avx are missing.

Sorry, I can't look into this before midweek.

Describe how to reproduce the problem

cat /sys/module/icp/parameters/icp_gcm_impl 
cycle [fastest] generic
@AttilaFueloep AttilaFueloep added the Type: Defect Incorrect behavior (e.g. crash, hang) label Jan 30, 2022
@AttilaFueloep
Copy link
Contributor Author

@nabijaczleweli

@rincebrain
Copy link
Contributor

rincebrain commented Jan 30, 2022 •
edited
Loading 

$ sudo zfs version
zfs-2.1.99-715_gc70bb2f61
zfs-kmod-2.1.99-715_gc70bb2f61
$ grep . /sys/module/icp/parameters/icp_*impl
/sys/module/icp/parameters/icp_aes_impl:cycle [fastest] generic x86_64 aesni
/sys/module/icp/parameters/icp_gcm_impl:cycle [fastest] avx generic pclmulqdq
$ uname -r
4.19.0-18-amd64
$

So I don't know that it's recent master that's the problem - I would wonder about 5.16 having more fallout.

e: so torvalds/linux@2f27b50 broke our fpu_internal check I'm pretty sure, and with kernel_fpu_license failing, that leaves no acceleration for us.

...and torvalds/linux@df95b0f double-broke it by hiding XSTATE_XSAVE. "great."

(from my Fedora Rawhide guest running 5.16)

configure:106056: checking whether kernel fpu is available
configure:106083: result: unavailable

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Jan 30, 2022 •
edited
Loading

Can't wait to get blamed for every vaguely-ICP-related breakage this calendar year. Bisect first, @ me second.

@AttilaFueloep
Copy link
Contributor Author

@nabijaczleweli Sorry, could've sworn that it worked with Linux 5.16.2 and 17b2ae0, but obviously it did not. I really should've been more thorough and not acting in a rush before tagging you, my apologies.

@rincebrain Had a quick look and I concur, those two commits broke fpu handling. IIUC we are not using struct fpu but just union fpstate_regs to allocate our own memory region where we store the FPU registers (but even if, we can still access the union via fpu->fpstate). The move of XSTATE_XSAVE seems to be no big deal either, it shouldn't be too hard to roll our own code to XSAVE{,OPT,S} the FPU state. I'll try to put something together by midweek.

@AttilaFueloep AttilaFueloep changed the title modules: icp: SIMD accelarated GCM disappeared Linux 5.16 broke FPU handling Jan 31, 2022
@rincebrain rincebrain mentioned this issue Feb 2, 2022
Closed
@AttilaFueloep AttilaFueloep mentioned this issue Feb 3, 2022
Merged
13 tasks
@zdykstra zdykstra mentioned this issue Feb 4, 2022
Merged
@dm17
Copy link

dm17 commented Feb 6, 2022

https://arstechnica.com/gadgets/2020/01/linus-torvalds-zfs-statements-arent-right-heres-the-straight-dope/ - for the sake of background, seems related. (Yes, I'm a ZFS admin who has no plans of moving off of ZFS, no matter what Linus says.)

ericonr added a commit to ericonr/void-packages that referenced this issue Feb 6, 2022
@ericonr
zfs: add patch for Linux 5.16 compat.
6b197cc 
Still missing patch for [1].

[1] openzfs/zfs#13042
ericonr added a commit to ericonr/void-packages that referenced this issue Feb 6, 2022
@ericonr
zfs: add patch for Linux 5.16 compat.
d0c9ea6 
Still missing patch for [1]. Lack of FPU acceleration for encryption is
a performance issue and annoying, but it doesn't explicitly break
anything, and we should wait for fixes to actually be merged.

[1] openzfs/zfs#13042
ericonr added a commit to void-linux/void-packages that referenced this issue Feb 7, 2022
@ericonr
zfs: add patch for Linux 5.16 compat.
d7c9036 
Still missing patch for [1]. Lack of FPU acceleration for encryption is
a performance issue and annoying, but it doesn't explicitly break
anything, and we should wait for fixes to actually be merged.

[1] openzfs/zfs#13042
behlendorf pushed a commit that referenced this issue Feb 9, 2022
@AttilaFueloep
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
8e94ac0 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes #13042
Closes #13059
ElDifinitivo pushed a commit to ElDifinitivo/void-packages that referenced this issue Feb 13, 2022
@ericonr @ElDifinitivo
zfs: add patch for Linux 5.16 compat.
9b6f051 
Still missing patch for [1]. Lack of FPU acceleration for encryption is
a performance issue and annoying, but it doesn't explicitly break
anything, and we should wait for fixes to actually be merged.

[1] openzfs/zfs#13042
ElDifinitivo pushed a commit to ElDifinitivo/void-packages that referenced this issue Feb 14, 2022
@ericonr @ElDifinitivo
zfs: add patch for Linux 5.16 compat.
8002293 
Still missing patch for [1]. Lack of FPU acceleration for encryption is
a performance issue and annoying, but it doesn't explicitly break
anything, and we should wait for fixes to actually be merged.

[1] openzfs/zfs#13042
ElDifinitivo pushed a commit to ElDifinitivo/void-packages that referenced this issue Feb 14, 2022
@ericonr @ElDifinitivo
zfs: add patch for Linux 5.16 compat.
baeb00c 
Still missing patch for [1]. Lack of FPU acceleration for encryption is
a performance issue and annoying, but it doesn't explicitly break
anything, and we should wait for fixes to actually be merged.

[1] openzfs/zfs#13042
tonyhutter pushed a commit to tonyhutter/zfs that referenced this issue Feb 15, 2022
@AttilaFueloep @tonyhutter
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
2ae2d02 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13042
Closes openzfs#13059
tonyhutter pushed a commit to tonyhutter/zfs that referenced this issue Feb 16, 2022
@AttilaFueloep @tonyhutter
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
c801e91 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13042
Closes openzfs#13059
tonyhutter pushed a commit to tonyhutter/zfs that referenced this issue Feb 17, 2022
@AttilaFueloep @tonyhutter
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
3b52ccd 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13042
Closes openzfs#13059
maciozo pushed a commit to maciozo/void-packages that referenced this issue Feb 19, 2022
@ericonr @maciozo
zfs: add patch for Linux 5.16 compat.
03cdcc9 
Still missing patch for [1]. Lack of FPU acceleration for encryption is
a performance issue and annoying, but it doesn't explicitly break
anything, and we should wait for fixes to actually be merged.

[1] openzfs/zfs#13042
jonathonf pushed a commit to jonathonf/zfs that referenced this issue Feb 21, 2022
@AttilaFueloep @jonathonf
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
124355c 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13042
Closes openzfs#13059
@jonathonf jonathonf mentioned this issue Feb 21, 2022
Closed
nicman23 pushed a commit to nicman23/zfs that referenced this issue Aug 22, 2022
@AttilaFueloep @nicman23
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
5ca2404 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13042
Closes openzfs#13059
nicman23 pushed a commit to nicman23/zfs that referenced this issue Aug 22, 2022
@AttilaFueloep @nicman23
Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state
6c0670e 
Linux 5.16 moved XSTATE_XSAVE and XSTATE_XRESTORE out of our reach,
so add our own XSAVE{,OPT,S} code and use it for Linux 5.16.

Please note that this differs from previous behavior in that it
won't handle exceptions created by XSAVE an XRSTOR. This is sensible
for three reasons.

 - Exceptions during XSAVE and XRSTOR can only occur if the feature
   is not supported or enabled or the memory operand isn't aligned
   on a 64 byte boundary. If this happens something else went
   terribly wrong, and it may be better to stop execution.

 - Previously we just printed a warning and didn't handle the fault,
   this is arguable for the above reason.

 - All other *SAVE instruction also don't handle exceptions, so this
   at least aligns behavior.

Finally add a test to catch such a regression in the future.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes openzfs#13042
Closes openzfs#13059
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Type: Defect Incorrect behavior (e.g. crash, hang)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state AttilaFueloep/zfs
4 participants
@rincebrain @dm17 @nabijaczleweli @AttilaFueloep