-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix limiter_node decrementer #647
Conversation
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
Signed-off-by: Mishin, Ilya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think two questions need to be resolved before this patch gets merged:
- Align whitespaces so that they are similar to, let's say, whitespace surroundings.
- Binary compatibility questions that we have talked offline.
Otherwise, the patch looks good to me.
Signed-off-by: Mishin, Ilya <[email protected]>
75f1373
to
f51fb90
Compare
This PR also fix issues: #342 and #489 Workaround is not needed: oneTBB/test/tbb/test_limiter_node.cpp Lines 128 to 138 in 2915b9b
|
All tests after replace workaround to: template< typename Sender, typename Receiver >
void make_edge_impl(Sender& sender, Receiver& receiver){
//#if __GNUC__ < 12 && !TBB_USE_DEBUG
// // Seemingly, GNU compiler generates incorrect code for the call of limiter.register_successor in release (-03)
// // The function pointer to make_edge workarounds the issue for unknown reason
// auto make_edge_ptr = tbb::flow::make_edge<int>;
// make_edge_ptr(sender, receiver);
//#else
tbb::flow::make_edge(sender, receiver);
//#endif
} Compilers:
Without this PR - ~100% of launches freezes. |
Signed-off-by: Serov, Vladimir <[email protected]>
Signed-off-by: Serov, Vladimir <[email protected]>
Commit: f620936 GCC versions:
CMake (3.17.0):
11 runs of each test set of each compiler. |
Do you have any updates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me!
Consider implementing the suggestion, though.
spin_mutex::scoped_lock lock(my_mutex); | ||
++my_count; | ||
if ( my_future_decrement ) { | ||
if ( my_count > my_future_decrement ) { | ||
my_count -= my_future_decrement; | ||
my_future_decrement = 0; | ||
} | ||
else { | ||
my_future_decrement -= my_count; | ||
my_count = 0; | ||
} | ||
} | ||
--my_tries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the code is a complete (both semantic and syntactic) duplication of lines 1951-1963 in the newer version of the same file. I would recommend moving them into separate limiter_node-specific helper method, which will be named correspondingly. E.g., account_successful_forward
.
There is also similar (slightly incomplete) code duplication about forwarding failure: see lines 2094-2101 and 1983-1994 in the newer version of the file. Please consider moving them as well.
Of course, it is mainly for better code readability and maintainability in the future, and not a showstopper for now. If there is no time or desire implementing this "small" improvement, consider leaving a TODO comment, something like: "// TODO: consider moving duplicated logic about processing of successful and unsuccessful task forward into separate functions."
* Fix limiter_node decrementer Signed-off-by: Mishin, Ilya <[email protected]> * Fix align Signed-off-by: Mishin, Ilya <[email protected]> * add new variable for check unused decrements Signed-off-by: Mishin, Ilya <[email protected]> * remove iostream Signed-off-by: Mishin, Ilya <[email protected]> * remove unnesessary cast Signed-off-by: Mishin, Ilya <[email protected]> * align whitespaces Signed-off-by: Mishin, Ilya <[email protected]> * Fix whitespace alignment Signed-off-by: Serov, Vladimir <[email protected]> * Revert workaround for GCC Signed-off-by: Serov, Vladimir <[email protected]> Co-authored-by: Serov, Vladimir <[email protected]>
Description
We have logical race in case where in one time first node try_put to limiter_node and second node try_put to limiter_node decrementer. So, we need to count the attempts to decrease my_count that were still in the my_tries stage.
Fixes # - #634
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@
to send notificationsOther information