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

Make test_thread_monitor more reliable. #177

Closed
wants to merge 1 commit into from
Closed

Make test_thread_monitor more reliable. #177

wants to merge 1 commit into from

Conversation

jamesjer
Copy link
Contributor

@jamesjer jamesjer commented Aug 27, 2019

This is a fix for issue 160. Multiple problems are addressed.

First, the volatile keyword is not sufficient on architectures with weak memory ordering, such as ppc64le. The request, ack, and clock fields of ThreadState should be atomic so that accesses to them by multiple threads are done with the necessary memory barriers. The stamp field does not need to be either volatile or atomic, as it is accessed by the main thread only.

The main thread can decide that a child thread is waiting in the monitor before the child thread even starts; hence the change to line 92.

There is no need to fetch request from memory twice in a row on lines 62 and 63; just fetch it once and use it twice.

Finally, the increment of clock was done too often. Two threads, the main thread and a child thread, can get into an endless cycle of changing the clock value and trying to read the same clock value twice in a row. The tick of the clock value is only needed to signal that a child thread is going to sleep in the monitor ... so only increment if the child thread really is about to go to sleep in the monitor.

With these changes, the test completes reliably on ppc64le.

@hjmjohnson
Copy link

@jamesjer Thanks for the contribution.

@anton-potapov
Copy link
Contributor

closing as not applicable to oneTBB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants