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

FreeBSD: Use a statement expression to implement SET_ERROR() #16284

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

markjdb
Copy link
Contributor

@markjdb markjdb commented Jun 19, 2024

Use a statement expression to implement SET_ERROR().

Motivation and Context

The current implementation makes assumptions about how SDT_PROBE* is implemented on FreeBSD. These assumptions will become false after a forthcoming FreeBSD kernel change.

Description

SET_ERROR() can be implemented using a statement expression in a way that avoids making assumptions about SDT internals. OpenZFS uses statement expressions elsewhere in the kernel module, so I presume that doing so here is allowed.

How Has This Been Tested?

Interactively, using dtrace -n 'sdt:::set-error {printf("%d", args[0]); stack();}' and using a kernel debugger to validate dtrace's output.

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:

This way we can avoid making assumptions about the SDT probe
implementation.  No functional change intended.

Signed-off-by: Mark Johnston <[email protected]>
markjdb added a commit to markjdb/freebsd that referenced this pull request Jun 19, 2024
This way we can avoid making assumptions about the SDT probe
implementation.  No functional change intended.

This was submitted upstream as openzfs/zfs#16284
freebsd-git pushed a commit to freebsd/freebsd-src that referenced this pull request Jun 19, 2024
This way we can avoid making assumptions about the SDT probe
implementation.  No functional change intended.

This was submitted upstream as openzfs/zfs#16284

MFC after:	1 week
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I don't know why SET_ERROR() ended up different different, but ZFS uses DTRACE_PROBE1() macro in plenty of places. I am not sure what is the difference from SDT_PROBE1(), but unification would probably be good.

@markjdb
Copy link
Contributor Author

markjdb commented Jun 20, 2024

I don't know why SET_ERROR() ended up different different, but ZFS uses DTRACE_PROBE1() macro in plenty of places. I am not sure what is the difference from SDT_PROBE1(), but unification would probably be good.

The difference is just that SDT_* makes you define a probe's argument types in advance, while DTRACE_PROBE* does everything in one statement. So, DTRACE_PROBE* is more useful for ad-hoc tracing, while SDT_* lets you collect probe definitions and argument types in one place.

It is a bit of an odd difference. It occurs also partly because DTRACE_PROBE* comes from illumos, while SDT_* is a FreeBSD interface. At some point we (FreeBSD) should try to unify them, but it is not much of a priority for me at the moment.

From my POV, using SDT_* for SET_ERROR() makes some sense, since this macro is used frequently. If it was replaced with DTRACE_PROBE*, we'd just end up bloating zfs.ko a bit with redundant probe definitions.

@tonyhutter tonyhutter merged commit f72e081 into openzfs:master Jul 9, 2024
20 of 25 checks passed
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.

None yet

6 participants