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

[Guilherme Valarini via Phabricator via Openmp-commits]

gValarini added inline comments.


================
Comment at: openmp/libomptarget/src/device.cpp:201
+
+  std::lock_guard<std::mutex> LG(DataMapMtx);
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104555