[RFC] SIMD implementation of vdev_raidz generate and reconstruct routines

[ironMann]

based on top of @zfsonlinux:master

This is a new implementation of RAIDZ1/2/3 routines using x86_64 scalar, SSE, and AVX2 instruction sets. Included are 3 parity generation routines (P, PQ, and PQR) and 7 reconstruction routines, for all RAIDZ level. On module load, a quick benchmark of supported routines will select the fastest for each operation and they will be used in runtime. Original implementation is still present and can be selected via module parameter.

Patch contains:

• specialized gen/rec routines for all RAIDZ levels,
• new scalar raidz implementation (unrolled for speed),
• two x86_64 SIMD implementations (SSE and AVX2 instructions sets),
• fastest routines selected on module load (benchmark).
• cmd/raidz_test tool for math exactness verification, code coverage and benchmark

New zfs module parameter:

- zfs_vdev_raidz_impl (string, can be changed online): selects the implementation to use
    "fastest" - use the fastest math available
    "original" - use the original raidz code
    "cycle" - cycle through all available impl for each new vdev_raidz (default in userspace tests)
    "scalar" - new scalar impl
    "sse" - new SSE impl if available
    "avx2" - new AVX2 impl if available

[behlendorf]

Nice work. It would be great to see this kind of optimization added to ZFS. In order to make that happen it would be great if you (@ironMann) and @rdolbeau's, who has an implementation in #3374, could review each others work. Then come up with a proposed patch set you're both happy with to review and start rigorously validating for correctness and performance.

Part of what's prevented this functionality from being being added in the need for some kind of torture test to verify it. That's functionality which should be added to ztest. Maybe something like have it randomly change the raidz implementation every few seconds. Or even a dedicated test which verifies that all implementations generate the same parity.

[ironMann]

I was not aware of previous efforts. I took a look at @rdolbeau's work. Basically, two implementations differ only in minor details. He has more hand inlined assembly, whereas I utilize smaller asm macros to achieve practically the same (both unroll 4x). I opted for this because it lets me have generic algorithm functions for all methods (vdev_raidz_math_impl.h and vdev_raidz_math_x86simd.h) that are using asm macros for different instruction sets (SSE and AVX2). I chose not to do AVX128 version because it lacks shuffle instructions needed for reconstruction methods, and also because that even SSE variant approaches memory throughput of a single CPU core. Lastly, I used non-temporal prefetch and streaming store instructions so that CPU caches are not trashed by the parity operations.
These are the differences I noticed, but performance wise, I bet they are pretty close.

Regarding testing, I have an external test tool that verifies bit exactness for all methods I wrote. I'll try to port it into ztest. I also left the 'old' implementation in, so switching in runtime between them is not a problem.
Suggestions and comments are welcome.

[rdolbeau]

The 'good' branch for my code is abd2, since at the time the abd changes were meant to be merged. I don't know what the status is now, I haven't had time to follow ZFS developement closely.

abd2 also has simple benchmarking, AVX-512 (deactivated, as I've yet to be able to test if on actual hardware) & ARMv8/NEON. However, I don't have reconstruction and I didn't add explicit prefetch since I wasn't sure whether there was any data reuse between the parity code and other parts of ZFS.

I agree that AVX-128 is a bit redundant with SSE, since the kernel is probably no affected by the SSE/AVX transition (all SSE/AVX usage has to be explicit).

[sempervictus]

I think the ABD changes are still meant to be merged, @tuxoko keeps improving the stack anyway, but it is kind of a problem for those of us building test systems with physical hardware for things like this (lots of rebuilds) that its still out of tree.

We still have a production host with SSE running the abd2 branch derived work, though i believe its from @tomgarcia's fork of @rdolbeau's work.

If there are advantages to this approach, whether in maintainability or performance, it would be nice to see work along this vein merged in sooner rather than later, as it's esoteric enough to become difficult to merge after any significant divergence upstream.

Thank you @ironMann and @rdolbeau for building out these low level solutions.

[ironMann]

@behlendorf: I've added some code to use ztest for testing. Default behaviour for userspace now is to cycle through all supported implementations on each new zio. This should help with math stress testing. I'm running ztest -r 11 -R 3 -a 0 -T 3600. Maybe ztest should corrupt data more frequently?
@rdolbeau: wow, AVX-512, that was preemptive 👍 I also agree with you. In any case, each inst set will be in a separate compilation unit, so tweaking it later for maximal performance should not be an issue.
@sempervictus: I'll bootstrap a couple of VM's to lighten the load on test servers. It's surprisingly not easy to keep up with all different platforms, distros, kernels, and compilers. As for performance, here is a kernel-mode benchmark (including fpu costs) from our test server: results

[rdolbeau]

2016-02-18 17:31 GMT+01:00 Gvozden Neskovic [email protected]:

@rdolbeau: wow, AVX-512, that was preemptive

With the masking and scatter/gather and CDI, AVX-512 seems to be "SIMD done
right at last" :-) There's two versions in the code, without (KNL) or with
(SKX) AVX512BW.

But they apparently forgot some instructions - there's not vpxorb in
AVX512BW. Someone must have thought that since there was just the one
variant in SSE and AVX, they didn't need more in AVX-512 since it's
bit-wise... completely forgetting about the masking :-( So we can't easily
do XOR-with-masking from a mask register. Too bad.

Cordially,

Romain Dolbeau

[behlendorf]

@ironMann I know this is still a WIP but I made a few comments inline. It looks like this is moving in a good direction, thanks for working on it. I'm happy to make another pass perhaps once it's passing the buildbot.

Maybe ztest should corrupt data more frequently?

For testing purposes you could definitely increase the frequency of ztest_fault_inject() in ztest.c.

[behlendorf]

To aid in the automated testing of these optimizations a Intel Xeon E5-2676v3 (Haswell) has been added to the TEST builders. It supports the following cpu flags.

model name  : Intel(R) Xeon(R) CPU E5-2676 v3 @ 2.40GHz
flags       : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36
              clflush mmx fxsr sse sse2 ht syscall nx rdtscp lm constant_tsc rep_good nopl
              xtopology eagerfpu pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 x2apic
              movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand hypervisor
              lahf_lm abm fsgsbase bmi1 avx2 smep bmi2 erms invpcid xsaveopt

[ironMann]

@behlendorf @sempervictus I adapted the code to ABD patch (in #4439), but in the process, I had to reimplement all of the vector code to be able to consume HighMem pages efficiently. From this point on, I would prefer to concentrate on a single branch for further improvements. Basically the vector code is more or less stable on both branches, and now I want to work more on testing, verification and glue code for all implementation.
Any preference on which branch to proceed?

@behlendorf I seem that 2 zfstests on SIMD machine are consistently failing:

Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool_scrub/zpool_scrub_002_pos (run as root) [00:01] [FAIL]
Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool_scrub/zpool_scrub_003_pos (run as root) [00:02] [FAIL]

[behlendorf]

@ironMann I'd recommend working on top of @tuxoko's work. There's definitely ongoing work to get those changes in to state which can be maintained long term, but the intention is to move in that direction.

I suspect this is caused by a racy test case. It looks like the scrub is either already finished by the time the test tries to stop it, or maybe it hasn't started yet. Either way, I've seen this occasionally on other test instances and it's unrelated to these changes. I'll open a new issue so we can track and fix the tests.

From the log:

Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool_scrub/zpool_scrub_002_pos (run as root) [00:01] [FAIL]
18:37:37.51 ASSERTION: Verify scrub -s works correctly.
18:37:38.55 SUCCESS: /sbin/zpool scrub testpool.1405
18:37:38.55 cannot cancel scrubbing testpool.1405: there is no active scrub
18:37:38.55 ERROR: /sbin/zpool scrub -s testpool.1405 exited 1
Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool_scrub/zpool_scrub_003_pos (run as root) [00:02] [FAIL]
18:37:38.56 ASSERTION: scrub command terminates the existing scrub process and starts a new scrub.
18:37:39.81 SUCCESS: /sbin/zpool scrub testpool.1405
18:37:40.96 SUCCESS: /sbin/zpool scrub testpool.1405
18:37:40.96 zpool scrub don't stop existing scrubbing process.

[ofaaland]

@ironMann thanks for the explanation. Do you allow those options for testing purposes? Or are they there so the user can choose the implementation, for cases when vdev_raidz_math_init() chooses an implementation that is problematic?

[ironMann]

@ofaaland Well, initially it was for testing, but then it morphed to 'freedom of choice' kind of thing. I'm still not sure if 'cycle' option should be given to users (it is only useful for unit and coverage testing). Maybe it should be removed in non-debug builds.

In theory, vdev_raidz_math_init() could choose slower method due to bunch of reasons, but that is highly unlikely. But not, problematic implementation, in sense that it would not work correctly.

[behlendorf]

@ironMann having 'cycle' available for testing has been helpful to me to build confidence in the patch. But now that most of the testing done I wouldn't object to removing it as a option from the kernel module. We'll of course want to leave it in user space for ztest though.

Thus far I haven't observed any problems with the patch. I'll leave my testing running over the weekend and if I don't encounter any problems I'll sign off on this patch so it can be merged. That just leaves the very minor issue of removing 'cycle' as mentioned above.

[ofaaland]

@ironMann, feel free to reject it, but how about this text for the manpage?

Parameter for selecting raidz implementation to use.

Options marked (always) below may be selected on module load as they are
supported on all systems.

The remaining options may only be set after the module is loaded, as they
are available only if the implementations are compiled in and supported
on the running system.

Once the module is loaded, the content of
/sys/module/zfs/parameters/zfs_vdev_raidz_impl will show available options
with the currently selected one enclosed in [].

Possible options are:
  fastest  - (always) implementation selected using built-in benchmark
  original - (always) original raidz implementation
  scalar   - (always) scalar raidz implementation
  sse      - implementation using SSE instruction set (64bit x86 only)
  avx2     - implementation using AVX2 instruction set (64bit x86 only)
.sp

[ironMann]

@ofaaland thanks, it's more clear.

@behlendorf In addition, I would also skip benchmark in userspace, and use the highest supported impl as the 'fastest'. This avoids startup delay of tools that call kernel_init()?

[behlendorf]

@ironMann the kernel_init() optimization is a nice idea even if it should really only impact ztest. It also looks like vdev_zaps_005_pos is failing on all the testers so that's going to need to be explained.

[rdolbeau]

2016-06-18 12:53 GMT+02:00 Gvozden Neskovic [email protected]:

@behlendorf In addition, I would also skip benchmark in userspace, and
use the highest supported impl as the 'fastest'. This avoids startup delay
of tools that call kernel_init()?

Which is the fastest might be obvious on X86-64 (it's likely
AVX512>AVX2>SSE>scalar), but for other architectures it's not necessarily
the case. There's a lot of e.g. Aarch64 cores out there, and it's possible
a wide-issue scalar core would outperform a 64-bits wide NEON pipe in some
cases. And the NEON itself might need to be tuned differently for different
cores.

Cordially,

Romain Dolbeau

[ironMann]

@rdolbeau You are right, but there's currently no real use-case for having 'real fastest' impl in userspace tools. These apps cycle through all supported impl. for better test coverage. Real benchmark is still performed on 'zfs' module load, but removing it in userspace saves a lot of time on startup of ztest and zdb.

@behlendorf vdev_zaps_* seem to lack proper setup. If I run them standalone on my dev VM 005 fails, but they all pass in the whole suite. I've messed with the balance by adding default_cleanup in /raidz/raidz_*. I've tried adding default_setup_noexit ${DISKS%% *} to zaps/setup.ksh but that caused zaps_001 to fail, and all others to pass... Sorry, I don't have any more time now to debug this further.

[behlendorf]

@ironMann thanks for looking at the vdev_zaps_* failures. It's clear these failures are unrelated to this PR and the test cases themselves need to be improved. Given that they're transient and seem to be infrequent I'm OK with tackling them as their own issue. So they don't need to hold this up.

The testing I've done over the last week on real hardware has been as abusive as I could make it, and it was designed to cover as much of the testing space as possible: raidz[123] geometries, 4k-16M block sizes, 1-20 device-per-raidz groups, full device rebuilds, all new algorithms checked. I wasn't able to uncover any problems with the patch.

This patch LGTM. Nice job! Let me know if your happy with this as a final version so it can be merged. We can always tweak the administrative aspects of it a little if needed after it's merged. But the core of it looks completely solid.

[ironMann]

@behlendorf Thanks, I think the patch is in a ok shape, too. The core raidz framework should be generic enough to support addition of new instruction set implementation with minimal effort (also 32bit SSE/AVX2 variants if relevant).

[behlendorf]

Merged to master as:

ab9f4b0 SIMD implementation of vdev_raidz generate and reconstruct routines

@ironMann thanks again for implementing this and building a solid generic framework we can extend. @rdolbeau you should be all set to extend this for NEON.

[thegreatgazoo]

I'm testing master branch today. A few comments/questions on this patch:

# cat /sys/module/zfs/parameters/zfs_vdev_raidz_impl
[fastest] original scalar sse avx2

So I'm using the fastest option, but which one is that? As an end-user, I'd want to know which implementation is actually in use, but "fastest" doesn't give me that information.

# ls -l /sys/module/zfs/parameters/zfs_vdev_raidz_impl
-rw-r--r-- 1 root root 4096 Jul 12 17:15 /sys/module/zfs/parameters/zfs_vdev_raidz_impl
# echo original > /sys/module/zfs/parameters/zfs_vdev_raidz_impl
-bash: echo: write error: Invalid argument
# echo "original" > /sys/module/zfs/parameters/zfs_vdev_raidz_impl
-bash: echo: write error: Invalid argument

The file is writable so I guess I'd be able to change it at run-time, but it ended up with write errors. Did I do something wrong here?

[thegreatgazoo]

I'd rather call it raidz parity implementation, just to be clear.

[ironMann]

@thegreatgazoo The fastest impl. is envisioned to be found at runtime. So there's 10 raidz parity routines in total, and in theory the fastest can pick up function pointers from all supported implementations, if that is preferable. But in practice, it's probably going to use the widest SIMD implementation your CPU supports.

The file is writable so I guess I'd be able to change it at run-time, but it ended up with write errors. Did I do something wrong here?

That's news. Can you try with printf "original" > /sys/module/zfs/parameters/zfs_vdev_raidz_impl
It might be that echo adds a newline.

[behlendorf]

It might be that echo adds a newline.

This is exactly what's happening, we'll want to trim the trailing white space. I definitely tested this so I'm not sure how I missed it.

[ironMann]

@behlendorf It seems that echo is inconsistent with that. I'll fold these fixes into #4815

[rdolbeau]

2016-07-12 20:05 GMT+02:00 Gvozden Neskovic [email protected]:

But in practice, it's probably going to use the widest SIMD
implementation your CPU supports.

... on x86-64. On Aarch64 (#4801), it depends on the core. Some have fast, wide
NEON support... some don't :-(

Cordially,

Romain Dolbeau

[thegreatgazoo]

@ironMann Yes newline was the problem, echo -n worked. Since mere echo works with setting other parameters under /sys/module/zfs/parameters/, I'd think it should be fixed. BTW, I saw in the code:

module_param_call(zfs_vdev_raidz_impl, zfs_vdev_raidz_impl_set,
        zfs_vdev_raidz_impl_get, NULL, 0644);

The zfs_vdev_raidz_impl_get() grabs vdev_raidz_impl_lock, but zfs_vdev_raidz_impl_set() does not - that doesn't seem right to me.

[thegreatgazoo]

@ironMann Thanks the explanation on [fastest]. My real question is, as the admin, I want to know exactly what that [fastest] is, but /sys/module/zfs/parameters/zfs_vdev_raidz_impl doesn't tell me that. Another scenario would be bug reporting, if a user suspects/hits a bug in the parity routine, we'd at least need to know what that [fastest] points to, preferrably without patching and rebooting since technically [fastest] is dynamic.

[ironMann]

@thegreatgazoo That locking is indeed a bug. Thing is, that method can be called before the lock is even initialized (when module parameter is specified), and also through api from userspace. will fix.

That's a legitimate point about [fastest] option. Currently there's a kstat for measured throughput of all methods, and contents of [fastest] can be deduced from that, but there should exist a better way. I can extend kstat data to explicitly show what instruction set is used.