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

icp: rip out everything we don't use #16209

Closed
wants to merge 10 commits into from
Closed

icp: rip out everything we don't use #16209

wants to merge 10 commits into from

Conversation

robn
Copy link
Member

@robn robn commented May 19, 2024

Motivation and Context

After #16135, I started looking around the ICP module for gremlins, and goodness me there's a lot of stuff in there. I'd like to simplify it all a great deal, but for now, it seems reasonable to remove everything we're not using, to reduce surface area and make it easier to understand what's there. This PR does just that!

Description

See commits. I ran through removing everything that could just be cut with no or minimal code changes.

How Has This Been Tested?

Compile checked on Linux and FreeBSD, full ZTS run completed.

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:

@adamdmoss
Copy link
Contributor

I always wondered where ICP came from - looks like it's a lump of stuff from illumos. OpenZFS is far beyond the point where it's practical to take upstream updates from illumos, right, which would be the only good reason to keep this code reasonably unmolested? In which case 👍

@robn
Copy link
Member Author

robn commented May 20, 2024

@adamdmoss yeah, that's my understanding of the history, and yeah, same conclusion here - we don't get anything much from it being this way, but we pay for maintenance overhead.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 25, 2024
@behlendorf behlendorf self-requested a review May 25, 2024 02:59
behlendorf
behlendorf approved these changes May 29, 2024
View reviewed changes
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.

I'm all for slimming this down to what we actually need. This looks good, although I'm curious how you ended up identifying the unsued functionality. Was it all through code inspection? It'd be interesting to see what a code coverage tool reports about how much of the remaining code the ZTS exercises.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 29, 2024
@bghira
Copy link

bghira commented May 30, 2024

icp was added during the crypto feature work i believe to avoid GPL libraries.

robn added 10 commits May 30, 2024 09:15
@robn
icp: remove unused blowfish_ctx and des_ctx
dbe8479 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: brutally remove unused AES modes
799a722 
Still retaining the struture, for now.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove unusued incremental cipher methods
668cbbc 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove unused KCF_ macros
2a5167c 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove digest entry points
3e70539 
For whatever reason, we call digest mechanisms directly, not through the
KCF digest provider. So we can remove those entry points entirely.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: reorganise SHA2 digest mechanisms
777a9c8 
sha2_mech_type_t serves double-duty, as the list of MAC providers and
also the algo type for direct callers to SHA2Init. Until we disentangle
that, reorganise it to make the separation more clear. While we're
there, remove the digest mechs we don't use.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove unused SHA2 HMAC mechanisms
92dd87c 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove skein module
c1729b9 
Nothing calls it through the KCF interface, so this is all unused.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove unused headers
1fd0daf 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
icp: remove redundant FreeBSD check
7138fea 
We don't build illumos-crypto for FreeBSD.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
Copy link
Member Author

robn commented May 30, 2024

Last push updates author and sponsor tags.

@behlendorf yes, just reading code and cutting things out, keeping the build running. I deliberately limited myself to removing things, keeping refactoring to a minimum.

behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: brutally remove unused AES modes
57249bc 
Still retaining the struture, for now.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove unusued incremental cipher methods
4ed91dc 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove unused KCF_ macros
94f1e56 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove digest entry points
1291c46 
For whatever reason, we call digest mechanisms directly, not through the
KCF digest provider. So we can remove those entry points entirely.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: reorganise SHA2 digest mechanisms
10de12e 
sha2_mech_type_t serves double-duty, as the list of MAC providers and
also the algo type for direct callers to SHA2Init. Until we disentangle
that, reorganise it to make the separation more clear. While we're
there, remove the digest mechs we don't use.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove unused SHA2 HMAC mechanisms
f39241a 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove skein module
ae51262 
Nothing calls it through the KCF interface, so this is all unused.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove unused headers
4e714c0 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
behlendorf pushed a commit that referenced this pull request May 31, 2024
@robn @behlendorf
icp: remove redundant FreeBSD check
a72751a 
We don't build illumos-crypto for FreeBSD.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes #16209
@mcmilk
Copy link
Contributor

mcmilk commented Jun 5, 2024

@robn - thanks for the icp cleaning up patch series 👍

@robn robn mentioned this pull request Jul 20, 2024
Merged
13 tasks
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove unused blowfish_ctx and des_ctx
1d473c7 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: brutally remove unused AES modes
5b35c72 
Still retaining the struture, for now.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove unusued incremental cipher methods
decf0a9 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove unused KCF_ macros
8576e58 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove digest entry points
aadd18a 
For whatever reason, we call digest mechanisms directly, not through the
KCF digest provider. So we can remove those entry points entirely.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: reorganise SHA2 digest mechanisms
9e69033 
sha2_mech_type_t serves double-duty, as the list of MAC providers and
also the algo type for direct callers to SHA2Init. Until we disentangle
that, reorganise it to make the separation more clear. While we're
there, remove the digest mechs we don't use.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove unused SHA2 HMAC mechanisms
46f850a 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove skein module
c4f0dab 
Nothing calls it through the KCF interface, so this is all unused.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove unused headers
5b0d3fb 
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@robn @lundman
icp: remove redundant FreeBSD check
7f3161a 
We don't build illumos-crypto for FreeBSD.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allanjude allanjude allanjude left review comments

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@robn @adamdmoss @bghira @mcmilk @behlendorf @allanjude