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 and enhancements of SIMD raidz parity #4849

Closed
wants to merge 1 commit into from

Conversation

ironMann
Copy link
Contributor

@ironMann ironMann commented Jul 13, 2016

Fixes and enhancements of SIMD raidz parity

  • Implementation lock replaced with atomic variable
  • Trailing whitespace is removed from user specified parameter, to enhance
    experience when using commands that add newline, e.g. echo
  • raidz_test: remove dependency on getrusage() and RUSAGE_THREAD, Issue Fails to compile with musl: zed_log.c #4813
  • silence cppcheck in vdev_raidz, partial solution of Issue Fix errors detected by cppcheck #1392
  • Minor fixes and cleanups

Commit 2:

  • Enable use of original parity methods in [fastest] configuration.
    New opaque original ops structure, representing original methods, is added
    to supported raidz methods. Original parity methods are executed if selected
    implementation has NULL fn pointer.

@ironMann
Copy link
Contributor Author

@thegreatgazoo @behlendorf This patch tries to add more insight in what exactly is [fastest] executing. I expanded existing kstat to include an unique ID for each physical raidz parity method. Then IDs of [fastest] will point to actual method selected by micro-benchmark. Sample output here

@behlendorf
Copy link
Contributor

I believe the main reason fastest may be confusing is because it doesn't strictly map to sse2, ssse3, scalar, etc. It's possible to end up with a mix of physical raidz parity methods depending on the benchmark results. Users assume it implies one specific instruction set.

I think you're on the right track extending the kstat to show this additional information but I find the unique ID scheme difficult to read. How about either using a KSTAT_TYPE_RAW to give you more control over the output:

One option would be to put it in a table format like this. Then you could generate a two line header which included the fastest method as a string. It would still be easy for a machine to parse and a little more human readable. Or maybe there's a better way to present this data.

name                   gen_p      gen_pq     gen_pqr    rec_p      rec_q      rec_r      rec_pq     rec_pr    rec_qr    req_pqr
fastest                avx2       avx2       avx2       avx2       avx2       avx2       avx2       avx2      avx2      avx2
scalar                 1118289501 484500377  195820574  919417040  247690624  173231104  179224397  149657438 90724194  64720286
sse2                   3625460092 1434870357 724015474  3024805633 896621503  630538504  647007725  553314659 343958902 232100474
ssse3                  3624525351 1629358067 723437399  3017729206 1179305293 630538504  948837939  814881094 630538504 573245679
avx2                   5759696521 3234664240 5232794938 2249398193 1526734805 1619749260 1302228273 948837939 814881094 724015474
original               1351779696 312592152  112843654  1276534247 258874722  33854689   117104671  19853760  19896993  13197118

I'd suggest splitting the kstat changes, however you decide to do it, in to a separate patch from the other minor fixes. Then I'll be able to merge those changes independently while we decide what to do about the kstats.

@ironMann
Copy link
Contributor Author

I completely overlooked the KSTAT_TYPE_RAW functionality. Presenting info like that will make so much more sense. I'll split the patch as you suggested.

@behlendorf
Copy link
Contributor

LGTM, I probably makes sense to squash these two commits as you've done in 7501b7c in #4860. @ironMann I can cherry-pick that commit if you're also happy with it.

@ironMann
Copy link
Contributor Author

@behlendorf Yes, commit 7501b7c in #4860 is the same thing

- Implementation lock replaced with atomic variable

- Trailing whitespace is removed from user specified parameter, to enhance
experience when using commands that add newline, e.g. `echo`

- raidz_test: remove dependency on `getrusage()` and RUSAGE_THREAD, Issue openzfs#4813

- silence `cppcheck` in vdev_raidz, partial solution of Issue openzfs#1392

- Minor fixes and cleanups

- Enable use of original parity methods in [fastest] configuration.
New opaque original ops structure, representing native methods, is added
to supported raidz methods. Original parity methods are executed if selected
implementation has NULL fn pointer.

Signed-off-by: Gvozden Neskovic <neskovic@gmail.com>
@ironMann
Copy link
Contributor Author

squashed and rebased on master

@behlendorf
Copy link
Contributor

LGTM

@behlendorf
Copy link
Contributor

Merged as:

c9187d8 Fixes and enhancements of SIMD raidz parity

@behlendorf behlendorf closed this Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants