[Openmp-commits] [PATCH] D76780: [OpenMP] Added memory barrier to solve data race
Jonas Hahnfeld via Phabricator via Openmp-commits
openmp-commits at lists.llvm.org
Wed Mar 25 11:21:14 PDT 2020
Hahnfeld accepted this revision.
Hahnfeld added a comment.
This revision is now accepted and ready to land.
Yes, this looks related to memory consistency. As far as I understand the threads synchronize on `th_spin_here`, so this is guaranteed to be updated. Any other write before this in `__kmp_release_queuing_lock` is not guaranteed to be synchronized by a weak memory model. This includes `th_next_waiting` (which triggers the assertion), but also writes by the user application. That's particularly bad because this should be taken care of by the runtime!
So long story short: LGTM (provided we can move the `KMP_MB` a bit as mentioned inline), and this might actually fix also fix real user code.
Comment at: openmp/runtime/src/kmp_lock.cpp:1241
// ToDo: Use __kmp_wait_sleep or similar when blocktime != inf
KMP_WAIT(spin_here_p, FALSE, KMP_EQ, lck);
This thread waits for `th_spin_here = FALSE` (pointed to by `spin_here_p`).
Comment at: openmp/runtime/src/kmp_lock.cpp:1243-1251
TRACE_LOCK(gtid + 1, "acq spin");
if (this_thr->th.th_next_waiting != 0)
__kmp_dump_queuing_lock(this_thr, gtid, lck, *head_id_p, *tail_id_p);
+ // Make sure all updates to thread structures are complete before
Please move the `KMP_MB` before the debug output / right after `KMP_WAIT`, so `__kmp_dump_queuing_lock` isn't called for the wrong reasons. Also I would reword the comment, the barrier should also take care of writes from the user code.
Comment at: openmp/runtime/src/kmp_lock.cpp:1476-1483
head_thr->th.th_next_waiting = 0;
TRACE_LOCK_T(gtid + 1, "rel nw=0 for t=", head);
/* reset spin value */
Here's the release code: First set `th_next_waiting = 0`, then `KMP_MB` to sync this write to memory and finally `th_spin_here = FALSE` to release the locked thread.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the Openmp-commits