fix data race in BM_ThreadYieldSpinLockThrashing #1099
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1099 +/- ##
=======================================
Coverage 93.39% 93.39%
=======================================
Files 165 165
Lines 6229 6229
=======================================
Hits 5817 5817
Misses 412 412 |
|
@esigo thanks for looking into it and the fix. Could you please share a little information on how the race condition happens in the original implementation? |
| } | ||
| std::this_thread::yield(); | ||
| }, | ||
| [](std::atomic<bool> &l) { l.store(false, std::memory_order_release); }); | ||
| [](std::atomic_flag &l) { l.clear(std::memory_order_release); }); |
There was a problem hiding this comment.
Thanks for the fix. would you help understand this change (probably enrich PR description) how it is fixing the race condition.
There was a problem hiding this comment.
I updated the PR description. Please let me know if it needs more information is needed.
There was a problem hiding this comment.
Thanks for the update. Should we either
- Change the test comment ( // SpinLock thrashing with thread::yield() after N spins ) as there is no more yield happening.
- Add the yield after every few tries in the tight loop to be in sync with the benchmark comment.
There was a problem hiding this comment.
Good point, thanks. I updated the comment and added yield.
@ThomsonTan, does this answer the question? |
|
Thanks @esigo for the explanation. |
Fixes #1098 (issue)
Changes
Please provide a brief description of the changes here.
The lock function, should return only and only if it actually acquired the lock. This was not the case if the locking was not successful after a limit amount of tries.
see here for more info the issue.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes