This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Fix] Track all threads that may delete an entry
ClosedPublic

Authored by gValarini on Aug 25 2022, 10:36 AM.

Details

Summary

The entries inside a "target data end" is processed in three steps:

  1. Query internal data maps for the entries and dispatch any necessary device-side operations (i.e., data retrieval);
  2. Synchronize the such operations;
  3. Update the host-side pointers and remove any entry which reference counter reached zero.

Such steps may be executed by multiple threads which may even operate on
the same entries. The current implementation (D121058) tries to
synchronize these threads by tracking the "owner" for the deletion of
each entry using their thread ID. Unfortunately it may failed to do so
because of the following reasons:

  1. The owner is always assigned at the first step only if the reference count is 0 when the map is queried. This does not work when such owner thread is faster than a previous one that is also processing the same entry on another "target data end", leading to user-after-free problems.
  2. The entry is only added for post-processing (step 3) if its reference count was 0 at query time (step 1). This does not allow for threads to exchange responsibility for the deletion, leading again to user-after-free problems.
  3. An entry may appear multiple times in the arguments array of a "target data end", which may lead to deleting the entry prematurely, leading, again, to user-after-free problems.

This patch addresses these problems by tracking all the threads that are
using an entry at "target data end" region through a counter, ensuring
only the last one deletes it when needed. It also ensures that all
entries that are successfully found inside the data maps in step 1 are
also processed in step 3, regardless if their reference count was zeroed
or not at query time. This ensures the deletion ownership may be passed
to any thread that is using such entry.

Diff Detail

Event Timeline

gValarini created this revision.Aug 25 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 10:36 AM
gValarini requested review of this revision.Aug 25 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 10:36 AM
jdoerfert added inline comments.Sep 7 2022, 10:34 AM
openmp/libomptarget/include/device.h
76

Make a separate header, this is reusable.

openmp/libomptarget/src/omptarget.cpp
687

I think, this can leave an entry intact on the device:

T1: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry, HDTTMap is released.
T2: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry. HDTTMap is released.
Both exit this function, TPRs are destroyed, DeleterThreadCount is decremented twice and reaches 1, but nobody executing the code that would delete the entry.
ye-luo added inline comments.Sep 7 2022, 10:49 AM
openmp/libomptarget/src/omptarget.cpp
687

Can this be protected by not calling HDTTMap.destroy()?

ye-luo added inline comments.Sep 7 2022, 12:17 PM
openmp/libomptarget/src/omptarget.cpp
687

Second thought on this. If the mutex protection from HDTTMap is anyway needed, std::atomic seems likely not needed.
Probably need to revisit D123443, D123444 and D123446.

gValarini updated this revision to Diff 484646.EditedDec 21 2022, 12:32 PM

Update the reference counting scheme and which entries are included in the post-processing.

  • The counters are now explicitly managed. No more scope problems!
  • All entries are added for post-processing to allow any thread that references an entry at a target data end to become the deleter of such entry.
gValarini retitled this revision from [libomptarget] Avoid double-frees and user-after-frees of entries to [OpenMP][Fix] Track all threads that may delete an entry.Dec 21 2022, 12:33 PM
gValarini edited the summary of this revision. (Show Details)

I have minor questions but after reading it once it seems to make sense.

openmp/libomptarget/src/omptarget.cpp
737

IsNotLastUser, right?

751

What's this block about?

gValarini updated this revision to Diff 484725.Dec 21 2022, 5:41 PM
gValarini marked 5 inline comments as done.

Fixed a variable name

gValarini added inline comments.Dec 21 2022, 5:52 PM
openmp/libomptarget/src/omptarget.cpp
687

@ye-luo we have a new version of this solution, I think your previous comments are now fixed.

687

I believe this does not happen anymore.

737

Ops, IsNotLastUser is the correct one. Thanks!

751

To be completely honest, I cannot recall. Most of the checks that can affect DelEntry were transferred from targetDataEnd. Although I understand the other ones related to struct and ptr/obj mapping, I don´t get why the first element of a mapper-based entry should not have its reference count updated or should not be removed.

I know this was implemented in https://reviews.llvm.org/D100673, do you remember the purpose of this check? We could add a comment here explaining it better.

ye-luo accepted this revision.Dec 28 2022, 10:12 PM
ye-luo added inline comments.
openmp/libomptarget/src/device.cpp
364

getTgtPtrBegin arguments are very correlated. Better to be scrutinized in the the future.

openmp/libomptarget/src/omptarget.cpp
803

More readable with
/*FromDataEnd=*/true

This revision is now accepted and ready to land.Dec 28 2022, 10:12 PM
gValarini updated this revision to Diff 486297.Jan 4 2023, 8:06 AM
gValarini marked an inline comment as done.

Update documentation and variable names

gValarini marked an inline comment as done.Jan 4 2023, 8:06 AM
gValarini added inline comments.
openmp/libomptarget/src/device.cpp
364

I agree. Unfortunately, the comment below is a little misleading, since UpdateRefCount is not always set on targetDataEnd.

Decrement the reference counter if called from targetDataEnd.

I updated the doc comment for now and I'll add a small patch removing some of the return arguments that should be placed inside TargetPointerResultTy later.

gValarini marked an inline comment as done.Jan 4 2023, 8:06 AM

Ready to merge?

Ready to merge?

I believe it is.

There is just that one block of code you commented about that tries to manage the deallocation of nested custom mappers that I still cannot wrap my head around. But I believe we would need to revise how we manage the reference counting of those since they are probably being left allocated by the end of the program (https://github.com/llvm/llvm-project/issues/60106).

Do you think we can merge the patch and take care of that later?

Quite close to the release. Let's merge this and deal with the remaining issue in a later patch.

Quite close to the release. Let's merge this and deal with the remaining issue in a later patch.

Ok, I will land it. Thanks!

gValarini marked an inline comment as done.Jan 19 2023, 6:39 AM
gValarini edited the summary of this revision. (Show Details)Jan 19 2023, 6:48 AM
gValarini updated this revision to Diff 490499.Jan 19 2023, 6:49 AM

Update summary.