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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/src/device.cpp:112
+      if (Event)
+        destroyEvent(Event);
       return OFFLOAD_SUCCESS;
----------------
I feel it is better to have a wrapper wraps the raw Event and handles destroyEvent at the destructor.

Once HostDataToTargetMap destructor got called, all the Events are leaked.


================
Comment at: openmp/libomptarget/src/device.cpp:319
+      if (OldEvent)
+        destroyEvent(OldEvent);
+    } else {
----------------
Missing return error check


================
Comment at: openmp/libomptarget/src/device.cpp:327
+        void *Event = Entry->Event;
+        Entry->unlock();
+
----------------
after this point. If another thread changes Entry->Event and destroyed the old event.
The current copied Event becomes invalid.


================
Comment at: openmp/libomptarget/src/device.cpp:295
+      Entry->unlock();
+      // If there is an event attached, destroy it.
+      if (OldEvent)
----------------
tianshilei1992 wrote:
> ye-luo wrote:
> > The current event never got destroyed unitl the next transfer.
> > 1. event doesn't got properly destroyed.
> > 2. almost like one event per map.
> For your first comment, yes, and now it is fixed by an ugly way. A nicer solution would be related to the API design in D108528where which we can discuss. The question is whether we want to pass async info to the plugin for event creation, synchronization, and destroy.
> 
> For the second point, it's a compatibility consideration, that is if there is a platform that doesn't allow event to be reused after it is fulfilled. I don't know.
> For the second point, it's a compatibility consideration, that is if there is a platform that doesn't allow event to be reused after it is fulfilled. I don't know.

We don't have such a platform to worry about. If there is one, we just mark the platform as not supporting events that we expect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418