[Openmp-commits] [PATCH] D104418: [OpenMP][Offloading] Fixed data race in libomptarget caused by async data movement

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked 4 inline comments as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/device.cpp:279
+
+    void *Event = Entry->getEvent();
+    bool NeedNewEvent = Event == nullptr;
----------------
ye-luo wrote:
> One more question. Should the event only protect IsNew or both IsNew and HasFlagAlways? I feel in the case of HasFlagAlways not IsNew, it is user's responsible to handle the ordering of multiple transfers.
It should protect all because the race we are trying to fix in this patch is user transparent. Even two target tasks reading same object/variable/memory can trigger the race.


================
Comment at: openmp/libomptarget/src/device.cpp:95
+      void *Event = search->getEvent();
+      if (Event)
+        destroyEvent(Event);
----------------
JonChesterfield wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > Use unique_ptr with Deleter for Event?
> > That sounds reasonable but actually not practical. Same as what I said before, we can't have automatic destroy of the event because when `libomptarget` starts to destroy, we don't know whether the plugin is already dead. Calling to plugin may cause segment fault. Since we already have event pool, all those events will be destroyed properly when a plugin is destroyed. For those resources shared between `libomptarget` and plugins, `libomptarget` can only hold it, but not own it. That being said, we can only have *explicit* destruction.
> This would be solvable by calling dlclose on the plugins that have been opened by dlopen before libomptarget is destructed. That seems like a good idea independent of this patch - the plugin lifetime can be strictly nested within the lifetime of libomptarget.so
IIRC, I did try that, but that didn't solve the problem. We want to have a destructor function, which contains the `dlclose`, called at the end of the life cycle of `libomptarget`, even after all globals are destructed. That is not an easy task.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418