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: Add XSAVEC support to kfpu_begin() #14558

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AttilaFueloep
Copy link
Contributor

Motivation and Context

Since there is a register restore bug in XSAVES and XRSTORS add and use XSAVEC if available.

How Has This Been Tested?

Builds,, more tests needed.

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:

Since there is a register restore bug in XSAVES and XRSTORS add and
use XSAVEC if available.

Signed-off-by: Attila Fülöp <attila@fueloep.org>
@AttilaFueloep AttilaFueloep marked this pull request as draft March 1, 2023 18:10
@AttilaFueloep
Copy link
Contributor Author

Needs more testing and maybe some refinement.

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

We need to decide if we prefer XSAVES or XSAVEC if both are available. Please see the discussion in #14557 (comment) for more details. Linux prefers XSAVES, this PR currently XSAVEC. I'd lean towards XSAVES for the sake of the modified optimization. Comments welcome!

@behlendorf
Copy link
Contributor

Based on your nice analysis of the issue in #14557 my inclination would be to prefer XSAVES as well. Not only because of the extra optimization but also because that code path has a lot of real world miles on it and is known to work well (errata aside).

@AttilaFueloep
Copy link
Contributor Author

also because that code path has a lot of real world miles on it and is known to work well (errata aside).

That's a good point. I'll update the PR, do the usual mprime test and remove the draft tag.

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, pending the results of the mprime testing.

@AttilaFueloep
Copy link
Contributor Author

I think I may have found some issues, need to do more thorough checks.

@behlendorf
Copy link
Contributor

@AttilaFueloep any update on this?

@rincebrain
Copy link
Contributor

#14989 may be germane.

@tonyhutter
Copy link
Contributor

Related? #16452

@rincebrain
Copy link
Contributor

Not unrelated, but as far as I can tell, the erratum this is for is either incomplete or not the problem there, since xsaves is forcibly disabled and this is still coming up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants