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.

Sign up for GitHub

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

Jump to bottom

Fix Ordering on WordLock (tsan detected) #292

Merged
merged 2 commits into from Aug 9, 2021
Merged

Fix Ordering on WordLock (tsan detected) #292

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e3c66e
Fix Ordering on WordLock (tsan detected)
thomcc Aug 7, 2021
4fd88be
Detect and work around TSan's lack of support for fences
thomcc Aug 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/src/word_lock.rs
View file
Open in desktop
Expand Up @@ -154,7 +154,7 @@ impl WordLock {
if let Err(x) = self.state.compare_exchange_weak(
state,
state.with_queue_head(thread_data),
Ordering::Release,
Ordering::AcqRel,
Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch! Thinking about it for a bit, an acquire is indeed needed here so that the call to park happens after the thread is inserted into the queue.

Ordering::Relaxed,
) {
return x;
Expand Down Expand Up @@ -189,7 +189,7 @@ impl WordLock {
match self.state.compare_exchange_weak(
state,
state | QUEUE_LOCKED_BIT,
Ordering::Acquire,
Ordering::AcqRel,
Copy link
Owner

Choose a reason for hiding this comment

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

This (and the changes below) doesn't make sense: there's nothing to release. The lock itself was already released in unlock with release ordering.

However I am noticing that the state load at the start of this function should be Acquire since it needs to happen before reading the state entries. Perhaps fixing this makes tsan happy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's nothing to release. The lock itself was already released in unlock with release ordering.

Ah, my bad, yeah I see, I didn't look that closely, and thought this was still trying to unlock not to wake up threads 😅

However I am noticing that the state load at the start of this function should be Acquire since it needs to happen before reading the state entries.

So, this doesn't make sense to me, since it enters a loop that can only be exited by either returning (in which case the data in behind the ptr inside state is not read) or by passing the cmpxchg, which gives the Acquire.

... And in fact, yeah, I was mistaken that this needed AcqRel — this one was actually fine as Acquire. (Sorry...)

Actually, I think unlock_slow might be entirely fine, and lock_slow is the only problem. It looks like the other two changes in that are probably caused by TSan not understanding how fences work in all cases (I had heard this was fixed, but it seems it is didn't get merged into LLVM yet, and has stalled. Although, in June there was some progress, , so maybe someday soon).

Regardless, if I apply the normal workaround for acquire fences under TSan (where you replace a fence with a discarded acquire load), then L252/L233 can both use Release.

(Just to make extra-sure, I verified that the change on L157 is needed with or without the tsan fence fix).

I'll have an updated patch shortly.

Ordering::Relaxed,
) {
Ok(_) => break,
Expand Down Expand Up @@ -230,7 +230,7 @@ impl WordLock {
match self.state.compare_exchange_weak(
state,
state & !QUEUE_LOCKED_BIT,
Ordering::Release,
Ordering::AcqRel,
Ordering::Relaxed,
) {
Ok(_) => return,
Expand All @@ -249,7 +249,7 @@ impl WordLock {
match self.state.compare_exchange_weak(
state,
state & LOCKED_BIT,
Ordering::Release,
Ordering::AcqRel,
Ordering::Relaxed,
) {
Ok(_) => break,
Expand Down