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

Add SIMD metadata in /proc on Linux #16530

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

rincebrain
Copy link
Contributor

Too many times, people's performance problems have amounted to "somehow your SIMD support isn't working", and determining that at runtime is difficult to describe to people.

This adds a /proc/spl/kstat/zfs/simd node, which exposes metadata about which instructions ZFS thinks it can use, on AArch64 and x86_64 Linux, to make investigating things like this much easier.

e.g.

$ cat /proc/spl/kstat/zfs/simd
kfpu_allowed            1
kernel_neon             1
kernel_mode_neon        1
neon                    1
sha256                  0
sha512                  0

Motivation and Context

Things like #16452 where we got to patch a printk for similar functionality in to determine runtime state.

Description

Added a new /proc/spl/kstat/zfs/simd, which should print empty on other platforms and the various state checks on x86_64/arm/aarch64 Linux. (Since FreeBSD isn't a hostile upstream, this shouldn't be necessary to expose there...)

How Has This Been Tested?

I tried running it on a couple of x86_64 and aarch64 Linux boxes.

Notably, my machine with FreeBSD VMs is inaccessible at the moment, so I'd appreciate it if someone could try building it there if they get a chance before I make a new one...

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:

Too many times, people's performance problems have amounted to
"somehow your SIMD support isn't working", and determining that
at runtime is difficult to describe to people.

This adds a /proc/spl/kstat/zfs/simd node, which exposes
metadata about which instructions ZFS thinks it can use,
on AArch64 and x86_64 Linux, to make investigating things
like this much easier.

Signed-off-by: Rich Ercolani <[email protected]>
@rincebrain
Copy link
Contributor Author

Cute, I think I dropped a cleanup commit in the rebase removing things like "shut up".

Oops. I'll fix it when I inevitably end up pushing again to address someone's comments.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 17, 2024
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.

This will definitely be handy. Some feedback inline and I see you accidentally included an unrelated draidcfg.gz file in this PR.

The new CI GitHub actions workflows were merged so if you rebase this on master you'll get FreeBSD builds again. Don't be surprised to see some unrelated failures though.

lib/libzpool/Makefile.am Outdated Show resolved Hide resolved
module/Kbuild.in Outdated Show resolved Hide resolved
module/zcommon/simd_stat.c Show resolved Hide resolved
module/zcommon/simd_stat.c Outdated Show resolved Hide resolved
@Harry-Chen
Copy link
Contributor

It would be even better to show implementations selected in addition to CPU SIMD features, i.e., are we using the generic C version or optimized assembly for blake3, aes, sha256, etc?

@rincebrain
Copy link
Contributor Author

I'd prefer to have that as a separate feature, I think.

This was intended to make it not require kernel patching to answer questions about "does our code at runtime think you're missing this thing", and trying to expose all the different _impl tunable values in one place would, to do "right", involve a bunch of boilerplate reworking to have them all handle it in a consistent abstraction and then teaching something to serialize that abstraction, and I'd rather not block "I would like to not have to ask people to apply a kernel patch as a first step in debugging" on that.

Signed-off-by: Rich Ercolani <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
@behlendorf
Copy link
Contributor

It's not entirely comprehensive but the /proc/spl/kstat/zfs/*_bench micro benchmark files do show the performance of the various implementations. For the files which have a "fastest" row you can also see which one ZFS will pick by default.

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.

LGTM. I went ahead and resubmitted the failed CI run although it looks like it may take a while to run.

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

I'm not sure what happened here with the zfs-qemu runners but it did run cleanly on the zfs-linux ubuntu runners. Merged.

@behlendorf behlendorf merged commit 5d01243 into openzfs:master Sep 20, 2024
19 of 31 checks passed
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Sep 20, 2024
Too many times, people's performance problems have amounted to
"somehow your SIMD support isn't working", and determining that
at runtime is difficult to describe to people.

This adds a /proc/spl/kstat/zfs/simd node, which exposes
metadata about which instructions ZFS thinks it can use,
on AArch64 and x86_64 Linux, to make investigating things
like this much easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes openzfs#16530
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Sep 21, 2024
Too many times, people's performance problems have amounted to
"somehow your SIMD support isn't working", and determining that
at runtime is difficult to describe to people.

This adds a /proc/spl/kstat/zfs/simd node, which exposes
metadata about which instructions ZFS thinks it can use,
on AArch64 and x86_64 Linux, to make investigating things
like this much easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes openzfs#16530
@adamdmoss
Copy link
Contributor

Not sure it's working as intended.

% cat simd 
kfpu_allowed    	1

That's all I see. This is a boring ubuntu22 x64 box:

% arch
x86_64

@rincebrain
Copy link
Contributor Author

Cute. Let me see what's going on, because it was working on x86_64.

Of course, I wouldn't be surprised if Ubuntu broke something again...which kernel, specifically?

@rincebrain
Copy link
Contributor Author

Ah, I see, I broke the x86_64 part when fixing something at some point, apparently. Good job, me.

Simple enough fix, I think...

@adamdmoss
Copy link
Contributor

Confirmed working now, thanks!

ptr1337 pushed a commit to CachyOS/zfs that referenced this pull request Nov 14, 2024
Too many times, people's performance problems have amounted to
"somehow your SIMD support isn't working", and determining that
at runtime is difficult to describe to people.

This adds a /proc/spl/kstat/zfs/simd node, which exposes
metadata about which instructions ZFS thinks it can use,
on AArch64 and x86_64 Linux, to make investigating things
like this much easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes openzfs#16530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants