[libomptarget] Fix a race condition in checkDeviceAndCtors

[lechenyu]

Summary

When multiple threads invoke checkDeviceAndCtors, both of them may read true from the shared variable Device.HasPendingGlobals, and then invoke initLibrary redundantly. Therefore only protecting the access to Device.HasPendingGlobals is not sufficient to guarantee that initLibrary is invoked just once.

To fix this race condition, we move the invocation of initLibrary into the critical section, and remove the same lock inside initLibrary,

[tianshilei1992]

This should work. On the other hand, is it possible to use init_once somehow? I think that'd be a better way.

[jhuber6]

In D136952#3892132, @tianshilei1992 wrote:

This should work. On the other hand, is it possible to use init_once somehow? I think that'd be a better way.

Could we use init_once? We need to do this once per device right after the images have been loaded.

[jhuber6]

I think this should be fine. I believe I hit a deadlock here before so it's good to fix.

[tianshilei1992]

In D136952#3892309, @jhuber6 wrote:

In D136952#3892132, @tianshilei1992 wrote:

This should work. On the other hand, is it possible to use init_once somehow? I think that'd be a better way.

Could we use init_once? We need to do this once per device right after the images have been loaded.

Once per device?

[jhuber6]

In D136952#3893155, @tianshilei1992 wrote:

Once per device?

It just means that we'd need an array of std::once_flags for each device. It's not a huge barrier but not sure if it's that much more convenient.

[tianshilei1992]

In D136952#3893157, @jhuber6 wrote:

In D136952#3893155, @tianshilei1992 wrote:

Once per device?

It just means that we'd need an array of std::once_flags for each device. It's not a huge barrier but not sure if it's that much more convenient.

Yeah, that's what I mean. I generally think std::once_flags is cleaner than this method. I'm fine using PendingGlobalsMtx here as we can't get rid of it anyway.

[lechenyu]

In D136952#3893183, @tianshilei1992 wrote:

In D136952#3893157, @jhuber6 wrote:

In D136952#3893155, @tianshilei1992 wrote:

Once per device?

It just means that we'd need an array of std::once_flags for each device. It's not a huge barrier but not sure if it's that much more convenient.

Yeah, that's what I mean. I generally think std::once_flags is cleaner than this method. I'm fine using PendingGlobalsMtx here as we can't get rid of it anyway.

Hi, Shilei,

Should I land this patch or replace the lock with std::once_flags?

[tianshilei1992]

Let’s just land it for now. Thanks.

openmp/libomptarget/src/omptarget.cpp

 static int initLibrary(DeviceTy &Device) {
   /*
    * Map global data
    */
   int32_t DeviceId = Device.DeviceID;
   int Rc = OFFLOAD_SUCCESS;
   bool SupportsEmptyImages = Device.RTL->supports_empty_images &&
                              Device.RTL->supports_empty_images() > 0;
-
-  std::lock_guard<decltype(Device.PendingGlobalsMtx)> LG(
-      Device.PendingGlobalsMtx);
   {
     std::lock_guard<decltype(PM->TrlTblMtx)> LG(PM->TrlTblMtx);
     for (auto *HostEntriesBegin : PM->HostEntriesBeginRegistrationOrder) {
       TranslationTable *TransTable =
           &PM->HostEntriesBeginToTransTable[HostEntriesBegin];
       if (TransTable->HostTable.EntriesBegin ==
               TransTable->HostTable.EntriesEnd &&
           !SupportsEmptyImages) {
if (!deviceIsReady(DeviceID)) {
     handleTargetOutcome(false, Loc);
     return true;
   }
 
   // Get device info.
   DeviceTy &Device = *PM->Devices[DeviceID];
 
   // Check whether global data has been mapped for this device
-  bool HasPendingGlobals;
   {
     std::lock_guard<decltype(Device.PendingGlobalsMtx)> LG(
         Device.PendingGlobalsMtx);
-    HasPendingGlobals = Device.HasPendingGlobals;
-  }
-  if (HasPendingGlobals && initLibrary(Device) != OFFLOAD_SUCCESS) {
+    if (Device.HasPendingGlobals && initLibrary(Device) != OFFLOAD_SUCCESS) {
       REPORT("Failed to init globals on device %" PRId64 "\n", DeviceID);
       handleTargetOutcome(false, Loc);
       return true;
     }
+  }
 
   return false;
 }
 
 static int32_t getParentIndex(int64_t Type) {
   return ((Type & OMP_TGT_MAPTYPE_MEMBER_OF) >> 48) - 1;
 }