[Openmp-commits] [PATCH] D104555: [OpenMP][Offloading] Guard data movement from host to device with mapping table lock

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/src/device.cpp:201
+
+  std::lock_guard<std::mutex> LG(DataMapMtx);
+
----------------
tianshilei1992 wrote:
> gValarini wrote:
> > As far as I can understand, this lock is protecting both the queries/updates to the data map tables and the data transfer issue, which indeed fixes the problem described in the patch.
> > 
> > But I think this can reduce the possibility of concurrent data transfers to the same device when there is no overlapping between the data regions (as discussed in [49940](https://bugs.llvm.org/show_bug.cgi?id=49940#c9)), especially when the device only implements the synchronous API. What do you think about implementing two locking regions?
> > 
> > 1. A `DataMapMtx` (the one already present) would be locked only when querying/updating the data map. This is mandatory since the mapping tables are global to each device and must be synchronized among the many runtime threads;
> > 2. A `DataMapEntry` (one per mapped entry, stored at the `HostDataToTargetTy`) would be locked during the data transfer issue stage, avoiding the data corruption problem.
> > 
> > This could allow multiple threads to issue (or even execute, on the sync. case) multiple data transfer while still being correctly synchronized.
> > 
> > Note: the second locking region must start right before `DataMapMtx` is unlocked, so the same thread that first allocated the data is the one that issues the data transfer. This is important because other threads that use the same data won't issue such transfer (unless the `always` modifier is used).
> > Note: the second locking region must start right before DataMapMtx is unlocked, so the same thread that first allocated the data is the one that issues the data transfer. This is important because other threads that use the same data won't issue such transfer (unless the always modifier is used).
> 
> I'm not sure I understand your idea. If the second lock is inside the first lock, what's the point of it? For synchronous data movement, there is no issue stage. When the data movement is called, it just starts to copy and return when it's done.
I think @gValarini means the second lock overlaps with the first lock not inside.
T1 : lock table -> lock entry A -> unlock table  > issue transfer A -> unlock entry A
T2:  lock table (only need to wait for the unlock of table from  T1 instead of the unlock of entry A) -> lock entry B -> unlock table  > issue transfer B -> unlock entry B.
In this way, transfer A and B can happen concurrently.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104555