[Openmp-commits] [openmp] 9584c6f - [OpenMP][Offloading] Fixed data race in libomptarget caused by async data movement
Shilei Tian via Openmp-commits
openmp-commits at lists.llvm.org
Wed Jan 5 17:20:11 PST 2022
Author: Shilei Tian
Date: 2022-01-05T20:20:04-05:00
New Revision: 9584c6fa2fe216522b86ee5422147b511c73cb4a
URL: https://github.com/llvm/llvm-project/commit/9584c6fa2fe216522b86ee5422147b511c73cb4a
DIFF: https://github.com/llvm/llvm-project/commit/9584c6fa2fe216522b86ee5422147b511c73cb4a.diff
LOG: [OpenMP][Offloading] Fixed data race in libomptarget caused by async data movement
The async data movement can cause data race if the target supports it.
Details can be found in [1]. This patch tries to fix this problem by attaching
an event to the entry of data mapping table. Here are the details.
For each issued data movement, a new event is generated and returned to `libomptarget`
by calling `createEvent`. The event will be attached to the corresponding mapping table
entry.
For each data mapping lookup, if there is no need for a data movement, the
attached event has to be inserted into the queue to gaurantee that all following
operations in the queue can only be executed if the event is fulfilled.
This design is to avoid synchronization on host side.
Note that we are using CUDA terminolofy here. Similar mechanism is assumped to
be supported by another targets. Even if the target doesn't support it, it can
be easily implemented in the following fall back way:
- `Event` can be any kind of flag that has at least two status, 0 and 1.
- `waitEvent` can directly busy loop if `Event` is still 0.
My local test shows that `bug49334.cpp` can pass.
Reference:
[1] https://bugs.llvm.org/show_bug.cgi?id=49940
Reviewed By: grokos, JonChesterfield, ye-luo
Differential Revision: https://reviews.llvm.org/D104418
Added:
Modified:
openmp/libomptarget/include/device.h
openmp/libomptarget/src/device.cpp
openmp/libomptarget/src/omptarget.cpp
Removed:
################################################################################
diff --git a/openmp/libomptarget/include/device.h b/openmp/libomptarget/include/device.h
index 75dde85a8806d..dbaa1bd0b460a 100644
--- a/openmp/libomptarget/include/device.h
+++ b/openmp/libomptarget/include/device.h
@@ -82,6 +82,14 @@ struct HostDataToTargetTy {
/// movement has been issued. This mutex *must* be locked right before
/// releasing the mapping table lock.
std::mutex UpdateMtx;
+ /// Pointer to the event corresponding to the data update of this map.
+ /// Note: At present this event is created when the first data transfer from
+ /// host to device is issued, and only being used for H2D. It is not used
+ /// for data transfer in another direction (device to host). It is still
+ /// unclear whether we need it for D2H. If in the future we need similar
+ /// mechanism for D2H, and if the event cannot be shared between them, Event
+ /// should be written as <tt>void *Event[2]</tt>.
+ void *Event = nullptr;
};
// When HostDataToTargetTy is used by std::set, std::set::iterator is const
// use unique_ptr to make States mutable.
@@ -115,6 +123,12 @@ struct HostDataToTargetTy {
/// Get the hold reference count.
uint64_t getHoldRefCount() const { return States->HoldRefCount; }
+ /// Get the event bound to this data map.
+ void *getEvent() const { return States->Event; }
+
+ /// Set the event bound to this data map.
+ void setEvent(void *Event) const { States->Event = Event; }
+
/// Reset the specified reference count unless it's infinity. Reset to 1
/// (even if currently 0) so it can be followed by a decrement.
void resetRefCount(bool UseHoldRefCount) const {
diff --git a/openmp/libomptarget/src/device.cpp b/openmp/libomptarget/src/device.cpp
index 90d9947068209..75935b30520c2 100644
--- a/openmp/libomptarget/src/device.cpp
+++ b/openmp/libomptarget/src/device.cpp
@@ -91,6 +91,9 @@ int DeviceTy::disassociatePtr(void *HstPtrBegin) {
"count\n");
} else if (search->isDynRefCountInf()) {
DP("Association found, removing it\n");
+ void *Event = search->getEvent();
+ if (Event)
+ destroyEvent(Event);
HostDataToTargetMap.erase(search);
DataMapMtx.unlock();
return OFFLOAD_SUCCESS;
@@ -264,20 +267,62 @@ DeviceTy::getTargetPointer(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
DPxPTR(HstPtrBegin), DPxPTR(TargetPointer));
int Ret = submitData(TargetPointer, HstPtrBegin, Size, AsyncInfo);
-
- // Unlock the entry immediately after the data movement is issued.
- Entry->unlock();
-
if (Ret != OFFLOAD_SUCCESS) {
+ Entry->unlock();
REPORT("Copying data to device failed.\n");
// We will also return nullptr if the data movement fails because that
// pointer points to a corrupted memory region so it doesn't make any
// sense to continue to use it.
TargetPointer = nullptr;
}
+
+ void *Event = Entry->getEvent();
+ bool NeedNewEvent = Event == nullptr;
+ if (NeedNewEvent && createEvent(&Event) != OFFLOAD_SUCCESS) {
+ Entry->unlock();
+ REPORT("Failed to create event\n");
+ return {{false /* IsNewEntry */, false /* IsHostPointer */},
+ {} /* MapTableEntry */,
+ nullptr /* TargetPointer */};
+ }
+ // We cannot assume the event should not be nullptr because we don't
+ // know if the target support event. But if a target doesn't,
+ // recordEvent should always return success.
+ Ret = recordEvent(Event, AsyncInfo);
+ if (Ret != OFFLOAD_SUCCESS) {
+ Entry->unlock();
+ REPORT("Failed to set dependence on event " DPxMOD "\n", DPxPTR(Event));
+ return {{false /* IsNewEntry */, false /* IsHostPointer */},
+ {} /* MapTableEntry */,
+ nullptr /* TargetPointer */};
+ }
+ if (NeedNewEvent)
+ Entry->setEvent(Event);
+ // We're done with the entry. Release the entry.
+ Entry->unlock();
} else {
// Release the mapping table lock directly.
DataMapMtx.unlock();
+ // If not a host pointer and no present modifier, we need to wait for the
+ // event if it exists.
+ if (!IsHostPtr && !HasPresentModifier) {
+ Entry->lock();
+ void *Event = Entry->getEvent();
+ if (Event) {
+ int Ret = waitEvent(Event, AsyncInfo);
+ Entry->unlock();
+ if (Ret != OFFLOAD_SUCCESS) {
+ // If it fails to wait for the event, we need to return nullptr in
+ // case of any data race.
+ REPORT("Failed to wait for event " DPxMOD ".\n", DPxPTR(Event));
+ return {{false /* IsNewEntry */, false /* IsHostPointer */},
+ {} /* MapTableEntry */,
+ nullptr /* TargetPointer */};
+ }
+ } else {
+ Entry->unlock();
+ }
+ }
}
return {{IsNew, IsHostPtr}, Entry, TargetPointer};
@@ -365,7 +410,7 @@ void *DeviceTy::getTgtPtrBegin(void *HstPtrBegin, int64_t Size) {
int DeviceTy::deallocTgtPtr(void *HstPtrBegin, int64_t Size,
bool HasHoldModifier) {
// Check if the pointer is contained in any sub-nodes.
- int rc;
+ int Ret = OFFLOAD_SUCCESS;
DataMapMtx.lock();
LookupResult lr = lookupMapping(HstPtrBegin, Size);
if (lr.Flags.IsContained || lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) {
@@ -380,18 +425,22 @@ int DeviceTy::deallocTgtPtr(void *HstPtrBegin, int64_t Size,
DPxPTR(HT.HstPtrBegin), DPxPTR(HT.TgtPtrBegin), Size,
(HT.HstPtrName) ? getNameFromMapping(HT.HstPtrName).c_str()
: "unknown");
+ void *Event = lr.Entry->getEvent();
HostDataToTargetMap.erase(lr.Entry);
+ if (Event && destroyEvent(Event) != OFFLOAD_SUCCESS) {
+ REPORT("Failed to destroy event " DPxMOD "\n", DPxPTR(Event));
+ Ret = OFFLOAD_FAIL;
+ }
}
- rc = OFFLOAD_SUCCESS;
} else {
REPORT("Section to delete (hst addr " DPxMOD ") does not exist in the"
" allocated memory\n",
DPxPTR(HstPtrBegin));
- rc = OFFLOAD_FAIL;
+ Ret = OFFLOAD_FAIL;
}
DataMapMtx.unlock();
- return rc;
+ return Ret;
}
/// Init device, should not be called directly.
diff --git a/openmp/libomptarget/src/omptarget.cpp b/openmp/libomptarget/src/omptarget.cpp
index 3e9f6427b4724..dd3f97e12f724 100644
--- a/openmp/libomptarget/src/omptarget.cpp
+++ b/openmp/libomptarget/src/omptarget.cpp
@@ -581,14 +581,33 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
void *&TgtPtrBase = AsyncInfo.getVoidPtrLocation();
TgtPtrBase = ExpectedTgtPtrBase;
- int rt = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
- sizeof(void *), AsyncInfo);
- Pointer_TPR.MapTableEntry->unlock();
-
- if (rt != OFFLOAD_SUCCESS) {
+ int Ret = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
+ sizeof(void *), AsyncInfo);
+ if (Ret != OFFLOAD_SUCCESS) {
+ Pointer_TPR.MapTableEntry->unlock();
REPORT("Copying data to device failed.\n");
return OFFLOAD_FAIL;
}
+ void *Event = Pointer_TPR.MapTableEntry->getEvent();
+ bool NeedNewEvent = Event == nullptr;
+ if (NeedNewEvent && Device.createEvent(&Event) != OFFLOAD_SUCCESS) {
+ Pointer_TPR.MapTableEntry->unlock();
+ REPORT("Failed to create event.\n");
+ return OFFLOAD_FAIL;
+ }
+ // We cannot assume the event should not be nullptr because we don't
+ // know if the target support event. But if a target doesn't,
+ // recordEvent should always return success.
+ Ret = Device.recordEvent(Event, AsyncInfo);
+ if (Ret != OFFLOAD_SUCCESS) {
+ Pointer_TPR.MapTableEntry->unlock();
+ REPORT("Failed to set dependence on event " DPxMOD "\n",
+ DPxPTR(Event));
+ return OFFLOAD_FAIL;
+ }
+ if (NeedNewEvent)
+ Pointer_TPR.MapTableEntry->setEvent(Event);
+ Pointer_TPR.MapTableEntry->unlock();
} else
Device.ShadowMtx.unlock();
}
More information about the Openmp-commits
mailing list