Page MenuHomePhabricator

[OpenMP][FIX] Simplify the entry deletion scheme
AcceptedPublic

Authored by jdoerfert on Apr 8 2022, 10:14 PM.

Details

Summary

The scheme introduced with D121058 did not work properly. There are
three reasons, two are fixed here:

  1. The thread ID scheme is not needed and can result in dangling pointers if the thread in charge is faster than another one interested in deletion. We now keep the entry until the last thread that wants to delete the entry makes it to the deletion point.
  2. The PostProcessingPtrs set can contain an entry multiple times. The deletion can happen before we visit the entry again in a subsequent iteration. That resulted again in a dangling pointer. We track the deleted entries now to avoid this.

Diff Detail

Event Timeline

jdoerfert created this revision.Apr 8 2022, 10:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2022, 10:14 PM
jdoerfert requested review of this revision.Apr 8 2022, 10:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2022, 10:14 PM
Herald added a subscriber: sstefan1. · View Herald Transcript
tianshilei1992 added inline comments.Apr 13 2022, 9:52 AM
openmp/libomptarget/src/omptarget.cpp
910

In what cases the same entry is in PostProcessingPtrs more than once?

jdoerfert added inline comments.Apr 13 2022, 10:02 AM
openmp/libomptarget/src/omptarget.cpp
910

We have a test case where this happens. I forgot which one. The entry shows up 3 times. The first and last time we do not want to delete it, the middle one is a deletion.

This revision is now accepted and ready to land.Apr 13 2022, 5:12 PM
ye-luo requested changes to this revision.Apr 13 2022, 10:10 PM
ye-luo added inline comments.
openmp/libomptarget/include/device.h
102–108

Each target task instead of thread.

224

Where does it get called?

This revision now requires changes to proceed.Apr 13 2022, 10:10 PM
jdoerfert added inline comments.Apr 14 2022, 1:04 PM
openmp/libomptarget/include/device.h
224
ye-luo added inline comments.Apr 14 2022, 1:45 PM
openmp/libomptarget/src/omptarget.cpp
869

In another thread which is working on increase the refcount may have a race with the current thread deleting the entry because the other thread released HDTTMapAccessorTy and only have the entry locked.

jdoerfert updated this revision to Diff 423308.Apr 17 2022, 1:01 PM
jdoerfert marked 2 inline comments as done.

Fix rebase issues

jdoerfert added inline comments.Apr 17 2022, 1:03 PM
openmp/libomptarget/src/omptarget.cpp
869

No. HDTTMap is still locked here. In the next patch we introduce the consistent locking for entries. So HDTTMap is locked, and then we lock entry, and then we do the lookup. No other thread can modify the entry at that point and the ref count and delete count will be valid.

ye-luo accepted this revision.Apr 17 2022, 1:57 PM
This revision is now accepted and ready to land.Apr 17 2022, 1:57 PM