Page MenuHomePhabricator

[OpenMP] Fix race condition in the completion/freeing of detached tasks
ClosedPublic

Authored by protze.joachim on May 11 2020, 2:19 AM.

Details

Summary

Spurious assertion failures are symptoms of a race condition for the handling of detached tasks:
Assertion failure at kmp_tasking.cpp(3744): taskdata->td_flags.complete == 1.
Assertion failure at kmp_tasking.cpp(710): taskdata->td_flags.executing == 0.

in the case of detach=true, all accesses to taskdata in __kmp_task_finish need to happen before (~line 873):

taskdata->td_flags.proxy = TASK_PROXY;

This assignment signals to __kmp_fulfill_event, that the task will need to be freed there. So, conceptionally the ownership of taskdata is moved.

Is it safe to move the call to the destructors up in the code?
Do we need to acquire the lock in __kmp_fulfill_event to make sure the lock is released in __kmp_task_finish, before taskdata is freed?

Diff Detail

Event Timeline

protze.joachim created this revision.May 11 2020, 2:19 AM
protze.joachim updated this revision to Diff 263172.EditedMay 11 2020, 7:40 AM

Applied clang-format

Take the lock in __kmp_fulfill_event to avoid the race between unlocking in __kmp_task_finish and freeing taskdata (which contains the lock).

protze.joachim edited the summary of this revision. (Show Details)May 11 2020, 7:41 AM
AndreyChurbanov accepted this revision.May 12 2020, 7:00 AM

LGTM

Joachim, thanks for taking care of this bug.

This revision is now accepted and ready to land.May 12 2020, 7:00 AM
openmp/runtime/src/kmp_tasking.cpp
900

completion_lock looks a better name than competion_lock (typo?).

protze.joachim marked an inline comment as done.

Fixed the typo by reverting this local change

protze.joachim marked an inline comment as done.May 13 2020, 8:12 AM
protze.joachim added inline comments.
openmp/runtime/src/kmp_tasking.cpp
900

I revert this change to use a temporal variable (not sure whether use of auto is accepted in general). With the locking in kmp_fulfill_event this is not needed.

This revision was automatically updated to reflect the committed changes.
protze.joachim marked an inline comment as done.