Fix assertions in crypto reference helpers #13880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
ryao
force-pushed
the
crypto-helpers
branch
4 times, most recently
from
28dfd4e to
b303722
Compare
mjguzik
reviewed
Sep 12, 2022
ryao
force-pushed
the
crypto-helpers
branch
from
b303722 to
1ea0700
Compare
ryao
commented
Sep 14, 2022
The assertions are racy and the use of `membar_exit()` did nothing to fix that. The helpers use atomic functions, so we cleverly get values from the atomics that we can use to ensure that the assertions operate on the correct values. We also use `membar_producer()` prior to decrementing reference counts so that operations that happened prior to a decrement to 0 will be guaranteed to happen before the decrement on architectures that reorder atomics. This also slightly improves performance by eliminating unnecessary reads, although I doubt it would be measurable in any benchmark. Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
ryao
force-pushed
the
crypto-helpers
branch
from
1ea0700 to
3e46f96
Compare
mjguzik
approved these changes
Sep 14, 2022
behlendorf
added
Status: Accepted
andrewc12
pushed a commit
to andrewc12/openzfs
that referenced
this pull request
Sep 27, 2022
The assertions are racy and the use of `membar_exit()` did nothing to fix that. The helpers use atomic functions, so we cleverly get values from the atomics that we can use to ensure that the assertions operate on the correct values. We also use `membar_producer()` prior to decrementing reference counts so that operations that happened prior to a decrement to 0 will be guaranteed to happen before the decrement on architectures that reorder atomics. This also slightly improves performance by eliminating unnecessary reads, although I doubt it would be measurable in any benchmark. Reviewed-by: Mateusz Guzik <mjguzik@gmail.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13880
andrewc12
pushed a commit
to andrewc12/openzfs
that referenced
this pull request
Oct 1, 2022
The assertions are racy and the use of `membar_exit()` did nothing to fix that. The helpers use atomic functions, so we cleverly get values from the atomics that we can use to ensure that the assertions operate on the correct values. We also use `membar_producer()` prior to decrementing reference counts so that operations that happened prior to a decrement to 0 will be guaranteed to happen before the decrement on architectures that reorder atomics. This also slightly improves performance by eliminating unnecessary reads, although I doubt it would be measurable in any benchmark. Reviewed-by: Mateusz Guzik <mjguzik@gmail.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13880
andrewc12
pushed a commit
to andrewc12/openzfs
that referenced
this pull request
Oct 1, 2022
The assertions are racy and the use of `membar_exit()` did nothing to fix that. The helpers use atomic functions, so we cleverly get values from the atomics that we can use to ensure that the assertions operate on the correct values. We also use `membar_producer()` prior to decrementing reference counts so that operations that happened prior to a decrement to 0 will be guaranteed to happen before the decrement on architectures that reorder atomics. This also slightly improves performance by eliminating unnecessary reads, although I doubt it would be measurable in any benchmark. Reviewed-by: Mateusz Guzik <mjguzik@gmail.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13880
andrewc12
pushed a commit
to andrewc12/openzfs
that referenced
this pull request
Oct 1, 2022
The assertions are racy and the use of `membar_exit()` did nothing to fix that. The helpers use atomic functions, so we cleverly get values from the atomics that we can use to ensure that the assertions operate on the correct values. We also use `membar_producer()` prior to decrementing reference counts so that operations that happened prior to a decrement to 0 will be guaranteed to happen before the decrement on architectures that reorder atomics. This also slightly improves performance by eliminating unnecessary reads, although I doubt it would be measurable in any benchmark. Reviewed-by: Mateusz Guzik <mjguzik@gmail.com> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes openzfs#13880
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
These helpers were mentioned in the discussion of #13843. However, the solution here is to use atomics in a better way, rather than implement
membar_exit(), so it should get its own pull request.Description
The assertions are racy and the use of
membar_exit()did nothing to fix that.The helpers use atomic functions, so we cleverly get values from the atomics that we can use to ensure that the assertions operate on the correct values.
This also slightly improves performance by eliminating unnecessary reads, although I doubt it would be measurable in any benchmark.
How Has This Been Tested?
The buildbot can test.
Types of changes
Checklist:
Signed-off-by.