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

Set the number of threads up front in db_stress #9466

Closed
wants to merge 1 commit into from

Conversation

ltamasi
Copy link
Contributor

@ltamasi ltamasi commented Jan 28, 2022

Summary:
With the code on main, RunStressTest increments the number of threads
one by one as the threads are created and started. This results in a
data race with NonBatchedOpsStressTest::VerifyDb, which reads this
value without synchronization, and is also not correct in the sense
that VerifyDb assumes that the number of threads already has its final
value set (e.g. it's checking whether the current thread is the last
one). The patch fixes this by setting the number of threads before
creating/starting any threads. This also eliminates the need for locking
the mutex during thread startup.

Test Plan:
Ran the blackbox crash test under TSAN for a while.

Summary:
With the code on main, `RunStressTest` increments the number of threads
one by one as the threads are created and started. This results in a
data race with `NonBatchedOpsStressTest::VerifyDb`, which reads this
value without synchronization, and is also not correct in the sense
that `VerifyDb` assumes that the number of threads already has its final
value set (e.g. it's checking whether the current thread is the last
one). The patch fixes this by setting the number of threads before
creating/starting any threads. This also eliminates the need for locking
the mutex during thread startup.

Test Plan:
Ran the blackbox crash test under TSAN for a while.
@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

which reads this value without synchronization, and is also not correct in the sense
that VerifyDb assumes that the number of threads already has its final value set

The latter problem seems a consequence of the former, because if it had synchronized it would only be able to read the final value (because lock is held until all threads are created). Anyways this fix LGTM

@ltamasi
Copy link
Contributor Author

ltamasi commented Jan 29, 2022

The latter problem seems a consequence of the former, because if it had synchronized it would only be able to read the final value (because lock is held until all threads are created). Anyways this fix LGTM

Thanks for the review! I mentioned the second issue separately because there are other ways of making the code race-free (namely, making the counters atomic) but that still wouldn't be correct for the purposes of computing the key range in VerifyDb. But yeah, locking the mutex there as well would also solve both problems

ltamasi added a commit to ltamasi/rocksdb that referenced this pull request Feb 1, 2022
Summary:
Fixes a typo introduced in
facebook#9466.

Fixes facebook#9482

Test Plan:
```
COMPILE_WITH_TSAN=1 make db_stress -j24
./db_stress --ops_per_thread=1000 --reopen=5
```
facebook-github-bot pushed a commit that referenced this pull request Feb 1, 2022
Summary:
Fixes a typo introduced in #9466.

Fixes #9482

Pull Request resolved: #9485

Test Plan:
```
COMPILE_WITH_TSAN=1 make db_stress -j24
./db_stress --ops_per_thread=1000 --reopen=5
```

Reviewed By: ajkr

Differential Revision: D33928601

Pulled By: ltamasi

fbshipit-source-id: 3e01a0ca5fffb56c268c811cbe045413b225059a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants