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

Fixes for SPARC support #6750

Merged
merged 4 commits into from Oct 12, 2017
Merged

Fixes for SPARC support #6750

merged 4 commits into from Oct 12, 2017

Conversation

KireinaHoro
Copy link
Contributor

@KireinaHoro KireinaHoro commented Oct 11, 2017

Description

Current code base almost compiles on SPARC, yet a few fixes are required for the code to compile
(and work efficiently). Code in this PR comes from OpenZFS project, nearly all of them dates back
to OpenZFS launch (~12 yrs ago).

Motivation and Context

Fixes #6738.
Fixes #6733.

How Has This Been Tested?

Compile passed (including all compile-time unit tests) on an UltraSPARC T2 machine (namely a Sun Enterprise T5120).
ztest finished successfully with return value 0.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

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.

The patches looks good just a few minor suggestions. Thanks for going back to the source and adding back in these sparc optimizations, I didn't realize we'd dropped them in the original port.

SHA1Transform((ctx)->state[0], (ctx)->state[1], (ctx)->state[2], \
(ctx)->state[3], (ctx)->state[4], (ctx), (in))
SHA1Transform((ctx)->state[0], (ctx)->state[1], (ctx)->state[2], \
(ctx)->state[3], (ctx)->state[4], (ctx), (in))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It looks like this style fix should be in 3b05b50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rebased the branch and that styling issue fix is now in its correct place.

@@ -13,7 +13,8 @@ DEFAULT_INCLUDES += \
-I$(top_srcdir)/lib/libspl/include

AM_CCASFLAGS = \
-I$(top_srcdir)/lib/libspl/include
-I$(top_srcdir)/lib/libspl/include \
$(CFLAGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you can remove -I$(top_srcdir)/lib/libspl/include from AM_CCASFLAGS while you're here to simplify things. CPPASCOMPILE and LTCPPASCOMPILE both already include DEFAULT_INCLUDES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new commit to remove that line.

@codecov
Copy link

codecov bot commented Oct 11, 2017

Codecov Report

Merging #6750 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6750     +/-   ##
=========================================
+ Coverage    74.1%   74.21%   +0.1%     
=========================================
  Files         295      295             
  Lines       93905    93905             
=========================================
+ Hits        69591    69687     +96     
+ Misses      24314    24218     -96
Flag Coverage Δ
#kernel 74.61% <0%> (+0.33%) ⬆️
#user 63.49% <100%> (+0.08%) ⬆️
Impacted Files Coverage Δ
module/icp/algs/sha1/sha1.c 97.5% <100%> (ø) ⬆️
module/zfs/dmu_tx.c 81.87% <0%> (-4.19%) ⬇️
module/zfs/mmp.c 95.7% <0%> (-1.23%) ⬇️
module/zfs/dsl_userhold.c 88.49% <0%> (-1.2%) ⬇️
cmd/zed/agents/zfs_mod.c 59.29% <0%> (-1.06%) ⬇️
cmd/zed/agents/zfs_diagnosis.c 34.31% <0%> (-0.59%) ⬇️
module/zfs/vdev_raidz.c 97.6% <0%> (-0.46%) ⬇️
module/zfs/dnode.c 92.63% <0%> (-0.21%) ⬇️
module/zfs/ddt.c 96.57% <0%> (-0.19%) ⬇️
module/zfs/dsl_dir.c 80.25% <0%> (-0.13%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4404c3...d9ee0e2. Read the comment docs.

Passing arguments explicitly into SHA1Transform() increases the number of
registers abailable to the compiler, hence leaving more local and out registers
available. The missing symbol of sha1_consts[], which prevents compiling on
SPARC, is added back, which speeds up the process of utilizing the relative
constants.
This should fix #6738.

Signed-off-by: Pengcheng Xu <i@jsteward.moe>
Normally a SPARC processor runs in big endian mode. Save the extra labor
needed for little endian machines when the target is a big endian one
(sparc).

Signed-off-by: Pengcheng Xu <i@jsteward.moe>
It's important to respect the user's CFLAGS as mismatched -mcpu
will directly result in the assembler not able to produce correct
code. Fixes #6733.

Signed-off-by: Pengcheng Xu <i@jsteward.moe>
CPPASCOMPILE and LTCPPASCOMPILE all include DEFAULT_INCLUDES,
hence it's unnecessary to add the includes again.

Signed-off-by: Pengcheng Xu <i@jsteward.moe>
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.

Thanks, looks good.

@behlendorf behlendorf merged commit e0922b0 into openzfs:master Oct 12, 2017
@behlendorf behlendorf added this to PR Needed in 0.7.3 Oct 12, 2017
aerusso pushed a commit to aerusso/zfs that referenced this pull request Oct 12, 2017
The current code base almost compiles on SPARC, but a few fixes are
required for the code to compile (and work efficiently). Code in this 
PR comes from OpenZFS project which was initially dropped when porting
the crypto framework.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Pengcheng Xu <i@jsteward.moe>
Closes openzfs#6733 
Closes openzfs#6738 
Closes openzfs#6750
tonyhutter pushed a commit that referenced this pull request Oct 16, 2017
The current code base almost compiles on SPARC, but a few fixes are
required for the code to compile (and work efficiently). Code in this
PR comes from OpenZFS project which was initially dropped when porting
the crypto framework.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Pengcheng Xu <i@jsteward.moe>
Closes #6733
Closes #6738
Closes #6750
@tonyhutter tonyhutter moved this from PR Needed to Merged to 0.7.3 in 0.7.3 Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.7.3
Merged to 0.7.3
2 participants