[Openmp-commits] [PATCH] D81054: [OpenMP] Introduce target memory manager

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a comment.

Since I spent hours to hunt down several race conditions in libomp in the last months, please fix races immediately, when they are pointed out. There is no such thing as a //benign// race!



================
Comment at: openmp/libomptarget/src/MemoryManager.cpp:149
+      FreeListTy &List = FreeLists[I];
+      if (List.empty())
+        continue;
----------------
JonChesterfield wrote:
> tianshilei1992 wrote:
> > JonChesterfield wrote:
> > > tianshilei1992 wrote:
> > > > ye-luo wrote:
> > > > > There can be race when you test List.empty().
> > > > It is on purpose, and the "race" is not a problem. Think about it. Even we wrap it into a lock and now it is empty, there is still chance that when we move to the next list, one or more nodes are returned to this list. No difference.
> > > That seems to assume list.empty() is an atomic operation. It isn't - calling list.empty() from one thread while another can be inserting into the list is a data race.
> > > 
> > > We could do something involving a relaxed read followed by a lock followed by another read, in the double checked locking fashion. Uncontended locks are cheap though so it's probably not worthwhile.
> > Double check does not work either. If the `empty` function might crash because of the data race, that is a problem. Otherwise, it is not a problem. Like I said, another thread could still insert node into the list after we check empty using a lock.
> Double check requires an atomic read, though relaxed is fine. Or probably some use of barriers. Calling empty() while another thread modifies the list is the race.  Because empty() is not atomic qualified, the race is UB.
> 
> Empty probably resolves to loading two pointers and comparing them for equality, so I sympathise with the argument that the race is benign, but it's still prudent to remove the data races we know about. Less UB, and means data race detectors will have a better chance of helping find bugs.
> Double check does not work either. If the `empty` function might crash because of the data race, that is a problem. Otherwise, it is not a problem. Like I said, another thread could still insert node into the list after we check empty using a lock.

double check is used to solve race condition not data race. data race is UB and must be avoided. race condition is not UB and might be accepted (benign), but can also break the code - especially reference counting.
To avoid the data race, as Jon said, you should use atomics. You might want to add an atomic counter to avoid the use of non-atomic List.empty().

When using double-checking, you need to perform all changes under lock (inserting to the list must be done under the same lock). All related double-checks occur under the same lock. In this case, the issue you tried to make can not occur.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054