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.

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

Linux x86 SIMD: factor out unneeded kernel dependencies #13102

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

AttilaFueloep
Copy link
Contributor

@AttilaFueloep AttilaFueloep commented Feb 14, 2022

Motivation and Context

Cleanup the kernel SIMD code by removing kernel dependencies.

Description

  • Replace XSTATE_XSAVE with our own XSAVE implementation for all
    kernels not exporting kernel_fpu{begin,end}(), see Linux 5.16 compat: don't use XSTATE_XSAVE to save FPU state #13059

  • Replace union fpregs_state by a uint8_t * buffer and get the size
    of the buffer from the hardware via the CPUID instruction

  • Replace kernels xgetbv() by our own implementation which was
    already there for userspace.

How Has This Been Tested?

Made a 3 disk raidz1 pool, created four datasets on it, one encrypted/fletcher3, one unencrypted/sha512, one unencrypted/skein and one unencrypted/edonr. Ran a 4 thread/32 files fio on each of them, started a mprime -t torture test and let all of this run for eight hours on a six vCPU 16G mem kvm vm. This was done for a version using internal kfpu and one using kernels kernel_fpu_{begin,end}(). Both went well, no mprime failures, no checksum error, no I/O errors, scrub showed no errors either.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@AttilaFueloep AttilaFueloep marked this pull request as draft February 14, 2022 16:19
@AttilaFueloep
Copy link
Contributor Author

Still not thoroughly tested. Hopefully the build bots will cover the case where kernel_fpu_{begin,end} is still exported by the kernel, since I'd need to find and install a distribution where this is still the case. Any insight and initial reviews are welcome.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 14, 2022
@tonyhutter
Copy link
Contributor

@AttilaFueloep I see this is still marked as a draft - are you ready for us to take a look at it?

@AttilaFueloep
Copy link
Contributor Author

Yes, that would be great. The HAVE_KERNEL_FPU_INTERNAL case should be ready to go (modulo stress testing). What I'm unsure about is the KERNEL_EXPORTS_X86_FPU case, and indeed the build fails on Ubuntu 18.04 i386. I've to look into that.

@AttilaFueloep
Copy link
Contributor Author

AttilaFueloep commented Feb 17, 2022

While trying to test this using the kernel versions of kernel_fpu_{begin,end}(), I realized that our configure test fails on newer kernels since kernel_fpu_begin() is now an inline function which calls kernel_fpu_begin_mask(). This could easily be fixed by testing for kernel_fpu_end() instead. The question is, do we want people to be able to change the license inside the META file to GPL and get the kernel versions of kernel_fpu_{begin,end}()?

@AttilaFueloep
Copy link
Contributor Author

Rebased to get scripts/zfs-tests-color.sh and make the testers happy.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding detecting kernel_fpu_{begin,end}() symbol compatibility, we should be checking if both symbols are available and compatible with the license in the META file. It'd be good to avoid making any assumptions about the implementation of those functions beyond if they're exported and available to us.

config/kernel-fpu.m4 Outdated Show resolved Hide resolved
include/os/linux/kernel/linux/simd_x86.h Outdated Show resolved Hide resolved
include/os/linux/kernel/linux/simd_x86.h Outdated Show resolved Hide resolved
@AttilaFueloep
Copy link
Contributor Author

AttilaFueloep commented Feb 19, 2022

So, if I understand you right, we shouldn't be using the ZFS_LINUX_TEST_RESULT_SYMBOL macro but ZFS_LINUX_TEST_RESULT, correct? ZFS_LINUX_TEST_RESULT makes sure we can build a kernel module, using our license from META, which calls both functions, so they are available to us. The additional step ZFS_LINUX_TEST_RESULT_SYMBOL takes is searching for kernel_fpu_begin in the kernels Module.symvers file, and this is an assumption about implementation (it requires it to be a "real" function) we should avoid.

On the other hand, I'd guess that we were using ZFS_LINUX_TEST_RESULT_SYMBOL for a reason, so I may miss something obvious here.

In either case, I think it would be best to open a separate PR for this, which could be merged first and added to the 2.1.3 branch alongside #13059 and #13089. It would make the check work as intended for kernels >= 5.11. I can then rebase this PR to include the change. What do you think?

@AttilaFueloep
Copy link
Contributor Author

Updated the "How Has This Been Tested" section in the PR description.

@behlendorf
Copy link
Contributor

The additional step ZFS_LINUX_TEST_RESULT_SYMBOL takes is searching for kernel_fpu_begin in the kernels Module.symvers file, and this is an assumption about implementation (it requires it to be a "real" function) we should avoid.

In the past there were some cases where this functionality was helpful, but you're right in this case we should be able to use ZFS_LINUX_TEST_RESULT. We could do it as a separate PR to make it easier to backport, but I'm not sure we'd want to when things are already working as intended.

@AttilaFueloep
Copy link
Contributor Author

OK, thanks for confirming.

We could do it as a separate PR to make it easier to backport, but I'm not sure we'd want to when things are already working as intended.

Well, the current test fails for Linux 5.11 and newer, since kernel_fpu_begin isn't a symbol anymore but an inline function which calls kernel_fpu_begin_mask(). I'll open a separate PR tomorrow.

@AttilaFueloep
Copy link
Contributor Author

Created #13147 with the discussed fix.

@behlendorf
Copy link
Contributor

Merged #13147

@AttilaFueloep AttilaFueloep marked this pull request as ready for review February 24, 2022 23:08
@AttilaFueloep
Copy link
Contributor Author

Thanks for the heads up! Addressed review feedback, squashed and rebased. Should be ready to go.

Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Signed-off-by: Attila Fülöp <attila@fueloep.org>
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for taking care of this cleanup, it's to nice have finally shed those last few kernel dependencies.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 1, 2022
@behlendorf behlendorf merged commit ce7a5db into openzfs:master Mar 9, 2022
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Aug 30, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 2, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Cleanup the kernel SIMD code by removing kernel dependencies.

 - Replace XSTATE_XSAVE with our own XSAVE implementation for all
   kernels not exporting kernel_fpu{begin,end}(), see openzfs#13059

 - Replace union fpregs_state by a uint8_t * buffer and get the size
   of the buffer from the hardware via the CPUID instruction

 - Replace kernels xgetbv() by our own implementation which was
   already there for userspace.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Attila Fülöp <attila@fueloep.org>
Closes openzfs#13102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants