[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



More information about the Openmp-commits mailing list