[Openmp-commits] [PATCH] D76780: [OpenMP] Added memory barrier to solve data race

[Jonas Hahnfeld via Phabricator via Openmp-commits]

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
 #ifdef DEBUG_QUEUING_LOCKS
       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);
 #endif
+      // 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;
 #ifdef DEBUG_QUEUING_LOCKS
       TRACE_LOCK_T(gtid + 1, "rel nw=0 for t=", head);
 #endif
 
       KMP_MB();
       /* 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76780/new/

https://reviews.llvm.org/D76780