work around a TSAN report during multiprocess encryption #7143
Conversation
Pull Request Test Coverage Report for Build james.stone_421
💛 - Coveralls |
ironage
force-pushed
the
js/tsan-fpositives
branch
from
ece139e to
a94e154
Compare
| // Copying an entire page may overwrite arrays which is being read concurrently by | ||
| // another thread. This is benign because it overwrites data from old version with | ||
| // the same value as is already there, so the reader sees the correct value. This | ||
| // is all by design. However, when doing a memcpy of the entire page, TSAN sees |
There was a problem hiding this comment.
So, do you essentially think that this is just false positive? Which part is "all by design"? The obvious question from my side is why it tries to overwrite with exactly the same data :D
There was a problem hiding this comment.
@kiburtse only some of the data on the page is expected to change when advancing versions. The parts of the page that are populated with array data for readers of older versions will not actually change, but the parts of the page that were "free space" may now be allocated and filled with new data by an external writer. TSAN says that the memcpy triggers a "write" on the old version data, even if the values didn't change. I was attempting to fix that by only copying the changed bytes, but this strategy doesn't appear to work because we still have to read the entire page to do this, which triggers another separate TSAN race (also possibly a false positive). I'm not sure this strategy is worth pursuing anymore, but I am hesitant to just add a tsan suppression because it may mask real problems.
| // this as a race even though we are replacing concurrently read data with the same | ||
| // values. | ||
| for (size_t i = 0; i < num_bytes; ++i) { | ||
| if (src[i] != dst[i]) { |
There was a problem hiding this comment.
Why check here if it should be the same data? Isn't this a contradiction to what is written in the comment?
|
Still the same tsan warnings from ubuntu2004-encryption-tsan builder |
Potential fix for #6474
I think this is a better approach than adding more suppressions to TSAN because that may hide legitimate issues in the future. The cost of this approach is a performance hit when running with TSAN enabled.