Fix BLAKE3 tuneable and module loading on Linux and FreeBSD

[mcmilk]

Apply similar options to BLAKE3 as it is done for zfs_fletcher_4_impl.

The zfs module parameter on Linux changes from icp_blake3_impl to zfs_blake3_impl.

You can check and set it on Linux via sysfs like this:

[bash]# cat /sys/module/zfs/parameters/zfs_blake3_impl
cycle [fastest] generic sse2 sse41 avx2

[bash]# echo sse2 > /sys/module/zfs/parameters/zfs_blake3_impl
[bash]# cat /sys/module/zfs/parameters/zfs_blake3_impl
cycle fastest generic [sse2] sse41 avx2

The modprobe module parameters may also be used now:

[bash]# modprobe zfs zfs_blake3_impl=sse41
[bash]# cat /sys/module/zfs/parameters/zfs_blake3_impl
cycle fastest generic sse2 [sse41] avx2

On FreeBSD the BLAKE3 implementation can be set via sysctl like this:

[bsd]# sysctl vfs.zfs.blake3_impl
vfs.zfs.blake3_impl: cycle [fastest] generic sse2 sse41 avx2
[bsd]# sysctl vfs.zfs.blake3_impl=sse2
vfs.zfs.blake3_impl: cycle [fastest] generic sse2 sse41 avx2 \
  -> cycle fastest generic [sse2] sse41 avx2

This commit changes also some Blake3 internals like these:

• blake3_impl_ops_t was renamed to blake3_ops_t
• all functions are named blake3_impl_NAME() now

Signed-off-by: Tino Reichardt [email protected] Co-authored-by: Ryan Moeller [email protected] Fixes: #13647

Motivation and Context

Description

How Has This Been Tested?

Types of changes

• [x] 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)
• [x] 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:

• [x] My code follows the OpenZFS code style requirements.
• [ ] I have updated the documentation accordingly.
• [x] I have read the contributing document.
• [ ] I have added tests to cover my changes.
• [x] I have run the ZFS Test Suite with this change applied.
• [x] All commit messages are properly formatted and contain Signed-off-by.

[mcmilk]

Sth. strange isn't okay currently... do no apply.

[ghost]

Very strange indeed... what the heck is printing "blake3_initialized" in the filetest tests where it should be corrupting the blocks?

[ghost]

My guess is zdb is hitting one of those ASSERT(blake3_initialized).

[mcmilk]

I will fix this tomorrow...

Edit: should work now, I initialized some things not properly :-(

[ghost]

I'm still unclear why this has to be more complicated than it is for fletcher. There should be no distinction between the module parameter and the chosen impl. It seems to be used to restore the parameter after benchmarking. It would be much simpler to save the original value in a local temporary variable before doing the benchmark and restore it after as is done with sel_savein fletcher_4_benchmark_impl. Is there some other use for it being separate?

[mcmilk]

It's the simpliest variant I have choosen currently.

The module paramter gets saved into blake3_impl_module ... when set, it will be used instead of the fastest method. The blake3_impl_chosen is the currently really used variant, it's default is also the fastest variant... this fastest variant is set to generic first, and can be updated via the set_fastest() call by the chksum interface.

It's a first step for implementing some modularization of:

1. the algo with it's different implementations (defined by xyz_ops_t)
2. the checksum benchmark for testing and getting the fastest method
3. the iteration over the diffenrent impls

These functions are new, for implementing this generic interface for multiple variants of some algorithms:

extern uint32_t blake3_impl_getcnt(void);
extern uint32_t blake3_impl_getid(void);
extern const char *blake3_impl_getname(void);
extern void blake3_impl_set_fastest(uint32_t id);
extern void blake3_impl_setid(uint32_t id);
extern int blake3_impl_setname(const char *name);

They provide the info and iteration over the multiple variants... currently only BLAKE3 is using this via:

• the ztest command
• the blake3_test command
• the chksum-interface makes use of this interface for benching, and setting the fastest method

What can be done better here ... and where. When I move the whole benchmarking back to the implementation... it's like fletcher... but then I can delete the zfs_chcksum.c file again ;-)

What I mean with interface you can see here maybe better: https://github.com/mcmilk/zfs/blob/sha2-rework/include/sys/zfs_impl.h

This is also some RFC for getting more into it?

[ghost]

I don't think blake3_impl_set_fastest() should actually change the chosen impl. It should just update what the fastest one is, and whatever the chosen impl already was can stay the same whether it was the default (fastest) or something else set by the user. Then blake3_param_set()/blake3_param() don't need to update blake3_impl_module after setting blake3_impl_chosen via blake3_impl_setname(), and chksum_benchmark() just has to save the chosen impl with blake3_impl_getid() before the benchmarks and restore it with blake3_impl_setid() when finished. Nothing else touches blake3_impl_module so it can go away.

That also makes it less magical inchksum_benchmark() where we currently seem to change the impl with reckless abandon. It is more obviously correct if we explicitly save the original value and restore it.

[mcmilk]

I don't think blake3_impl_set_fastest() should actually change the chosen impl. It should just update what the fastest one is, and whatever the chosen impl already was can stay the same whether it was the default (fastest) or something else set by the user. Then blake3_param_set()/blake3_param() don't need to update blake3_impl_module after setting blake3_impl_chosen via blake3_impl_setname(), and chksum_benchmark() just has to save the chosen impl with blake3_impl_getid() before the benchmarks and restore it with blake3_impl_setid() when finished. Nothing else touches blake3_impl_module so it can go away.

That also makes it less magical inchksum_benchmark() where we currently seem to change the impl with reckless abandon. It is more obviously correct if we explicitly save the original value and restore it.

Okay, I can change it to this way... but how should I set the fastest implementation in BLAKE3 then? When you say via setid() or setname() ... then I have to request the fastest impl. first?

When the module loads, I have no fastest implementation.... so I set it to generic. Cause flatcher4, aes and all other can set thefastest impl on module loading, while the benchmarks are in it.

[ghost]

You set the fastest impl by calling blake3_impl_set_fastest(). Then next time something calls blake3_impl_get_ops() it will get the new impl (assuming the chosen impl is IMPL_FASTEST).

[mcmilk]

You set the fastest impl by calling blake3_impl_set_fastest(). Then next time something calls blake3_impl_get_ops() it will get the new impl (assuming the chosen impl is IMPL_FASTEST).

This is how it works currently, yes.

[ghost]

Put another way, in blake3_impl_set_fastest() this:

        /* when module paramter was set, take this impl */
        if (blake3_impl_module != IMPL_FASTEST) {
                blake3_impl_setid(blake3_impl_module);
        } else {
                blake3_impl_setid(IMPL_FASTEST);
        }

is equivalent to this:

        blake3_impl_setid(blake3_impl_module);

which is effectively restoring the original value of blake3_impl_chosen which was either the default IMPL_FASTEST or set by blake3_param_set() via blake3_impl_setname().

I notice now too that blake3_param() is actually missing the atomic_swap_32(&blake3_impl_module, blake3_impl_chosen); part found in blake3_param_set(), so when the module param is set on load it will be lost after the benchmarks run on FreeBSD. All the more reason to simplify it out.

[ghost]

This is what I'm proposing: https://github.com/openzfs/zfs/commit/95e0d3131436c1ed6da19c5b8b2b6c6d09363f21

[mcmilk]

if (blake3_impl_module != IMPL_FASTEST) {

I first had an extra define of IMPL_UNSET for blake3_impl_module ... but I found IMPL_FASTEST is the default for unset module parameter, so I re-used it for blake3_impl_module.

This is what I'm proposing: 95e0d31

This will set BLAKE3 always to generic.

[ghost]

This will set BLAKE3 always to generic.

I don't see why. It still sets the fastest impl in blake3_fastest_impl.

[mcmilk]

I will re-check... ok Edit: yes, it works this way, you are right.

[vipsland]

sha256 is super slow. hope to have blake3 soon. i have tonnes of files that need this dedup with blake3 and using ubuntu 22.04 and it's still not available with zfs 2.1.5... pls push to ubuntu 22.04 upgrades :) thx for all the hard work.

[ghost]

Though this PR changes the parameter name to zfs_blake3_impl, it is still a parameter for the icp module (on Linux), which has no manpage.

[behlendorf]

True, but in the master branch the icp kmod has now been merged in the zfs kmod on Linux.

[mcmilk]

Looks good, thanks for sorting this out.

One thing I did notice when reviewing this was this module option was never added to the zfs() man page. Please add an entry for it, you can model it after the zfs_fletcher_4_impl text.

I rebased to master and added a second commit for the man page update.

[mcmilk]

Seems good apart from the one man page nit.

I have squashed the manpage fix with the manpage commit. And also the userspace fix with the cpu feature tests commit.

So the three commits should be ready then. I changed nothing else... just the squashing.