Skip to content

Help compiller optimize out abd_verify(). #12280

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

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

amotin
Copy link
Member

@amotin amotin commented Jun 24, 2021

While abd_verify() does nothing when built without debug, compiler
can't optimize it out by itself due to calls to external list_*()
and abd_verify_scatter(). This commit makes it explicit.

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:

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jun 24, 2021
@amotin amotin requested a review from behlendorf June 24, 2021 23:00
@amotin
Copy link
Member Author

amotin commented Jun 24, 2021

Or should I use "#ifdef ZFS_DEBUG" instead?

@behlendorf
Copy link
Contributor

Yes, it'd be better to use ZFS_DEBUG for this since that's how this is handled elsewhere in the code.

While abd_verify() does nothing when built without debug, compiler
can't optimize it out by itself due to calls to external list_*()
and abd_verify_scatter().  This commit makes it explicit.

Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
@amotin
Copy link
Member Author

amotin commented Jun 25, 2021

OK. Changed. I am curios why asserts still depend on NDEBUG, not ZFS_DEBUG as everything else.

@ikozhukhov
Copy link
Contributor

LGTM
i can't use github feature for 'approve' - i have no review button for my account

@behlendorf
Copy link
Contributor

I am curios why asserts still depend on NDEBUG, not ZFS_DEBUG as everything else.

I believe it's something we've just never gotten around to updating since it hasn't been a problem in practice. We probably should make it consistent at some point.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 25, 2021
@behlendorf behlendorf merged commit 5e2c833 into openzfs:master Jun 25, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 29, 2021
While abd_verify() does nothing when built without debug, compiler
can't optimize it out by itself due to calls to external list_*()
and abd_verify_scatter().  This commit makes it explicit.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Adam Moss <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12280
tle211212 pushed a commit to tle211212/zfs that referenced this pull request Jul 9, 2021
While abd_verify() does nothing when built without debug, compiler
can't optimize it out by itself due to calls to external list_*()
and abd_verify_scatter().  This commit makes it explicit.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Adam Moss <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12280
Conflicts:
	module/zfs/abd.c
@amotin amotin deleted the abd_verify_out branch August 24, 2021 20:17
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.

5 participants