[Openmp-commits] [openmp] d23131a - [OpenMP] Fix race condition in the completion/freeing of detached tasks
Joachim Protze via Openmp-commits
openmp-commits at lists.llvm.org
Sun May 17 03:28:49 PDT 2020
Author: Joachim Protze
Date: 2020-05-17T12:28:38+02:00
New Revision: d23131a3c063830e3d8d4f7d43cbcf95d92db3d3
URL: https://github.com/llvm/llvm-project/commit/d23131a3c063830e3d8d4f7d43cbcf95d92db3d3
DIFF: https://github.com/llvm/llvm-project/commit/d23131a3c063830e3d8d4f7d43cbcf95d92db3d3.diff
LOG: [OpenMP] Fix race condition in the completion/freeing of detached tasks
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.
Reviewed By: AndreyChurbanov
Differential Revision: https://reviews.llvm.org/D79702
Added:
Modified:
openmp/runtime/src/kmp_tasking.cpp
Removed:
################################################################################
diff --git a/openmp/runtime/src/kmp_tasking.cpp b/openmp/runtime/src/kmp_tasking.cpp
index 1cfc66fdd666..fe8bcad0fa6f 100644
--- a/openmp/runtime/src/kmp_tasking.cpp
+++ b/openmp/runtime/src/kmp_tasking.cpp
@@ -861,7 +861,37 @@ static void __kmp_task_finish(kmp_int32 gtid, kmp_task_t *task,
}
}
+ // bookkeeping for resuming task:
+ // GEH - note tasking_ser => task_serial
+ KMP_DEBUG_ASSERT(
+ (taskdata->td_flags.tasking_ser || taskdata->td_flags.task_serial) ==
+ taskdata->td_flags.task_serial);
+ if (taskdata->td_flags.task_serial) {
+ if (resumed_task == NULL) {
+ resumed_task = taskdata->td_parent; // In a serialized task, the resumed
+ // task is the parent
+ }
+ } else {
+ KMP_DEBUG_ASSERT(resumed_task !=
+ NULL); // verify that resumed task is passed as argument
+ }
+
+ /* If the tasks' destructor thunk flag has been set, we need to invoke the
+ destructor thunk that has been generated by the compiler. The code is
+ placed here, since at this point other tasks might have been released
+ hence overlapping the destructor invocations with some other work in the
+ released tasks. The OpenMP spec is not specific on when the destructors
+ are invoked, so we should be free to choose. */
+ if (taskdata->td_flags.destructors_thunk) {
+ kmp_routine_entry_t destr_thunk = task->data1.destructors;
+ KMP_ASSERT(destr_thunk);
+ destr_thunk(gtid, task);
+ }
+
KMP_DEBUG_ASSERT(taskdata->td_flags.complete == 0);
+ KMP_DEBUG_ASSERT(taskdata->td_flags.started == 1);
+ KMP_DEBUG_ASSERT(taskdata->td_flags.freed == 0);
+
bool detach = false;
if (taskdata->td_flags.detachable == TASK_DETACHABLE) {
if (taskdata->td_allow_completion_event.type ==
@@ -870,14 +900,17 @@ static void __kmp_task_finish(kmp_int32 gtid, kmp_task_t *task,
__kmp_acquire_tas_lock(&taskdata->td_allow_completion_event.lock, gtid);
if (taskdata->td_allow_completion_event.type ==
KMP_EVENT_ALLOW_COMPLETION) {
+ // task finished execution
+ KMP_DEBUG_ASSERT(taskdata->td_flags.executing == 1);
+ taskdata->td_flags.executing = 0; // suspend the finishing task
+ // no access to taskdata after this point!
+ // __kmp_fulfill_event might free taskdata at any time from now
taskdata->td_flags.proxy = TASK_PROXY; // proxify!
detach = true;
}
__kmp_release_tas_lock(&taskdata->td_allow_completion_event.lock, gtid);
}
}
- KMP_DEBUG_ASSERT(taskdata->td_flags.started == 1);
- KMP_DEBUG_ASSERT(taskdata->td_flags.freed == 0);
if (!detach) {
taskdata->td_flags.complete = 1; // mark the task as completed
@@ -897,45 +930,19 @@ static void __kmp_task_finish(kmp_int32 gtid, kmp_task_t *task,
// with the proxy task as origin
__kmp_release_deps(gtid, taskdata);
}
+ // td_flags.executing must be marked as 0 after __kmp_release_deps has been
+ // called. Othertwise, if a task is executed immediately from the
+ // release_deps code, the flag will be reset to 1 again by this same
+ // function
+ KMP_DEBUG_ASSERT(taskdata->td_flags.executing == 1);
+ taskdata->td_flags.executing = 0; // suspend the finishing task
}
- // td_flags.executing must be marked as 0 after __kmp_release_deps has been
- // called. Othertwise, if a task is executed immediately from the release_deps
- // code, the flag will be reset to 1 again by this same function
- KMP_DEBUG_ASSERT(taskdata->td_flags.executing == 1);
- taskdata->td_flags.executing = 0; // suspend the finishing task
KA_TRACE(
20, ("__kmp_task_finish: T#%d finished task %p, %d incomplete children\n",
gtid, taskdata, children));
- /* If the tasks' destructor thunk flag has been set, we need to invoke the
- destructor thunk that has been generated by the compiler. The code is
- placed here, since at this point other tasks might have been released
- hence overlapping the destructor invocations with some other work in the
- released tasks. The OpenMP spec is not specific on when the destructors
- are invoked, so we should be free to choose. */
- if (taskdata->td_flags.destructors_thunk) {
- kmp_routine_entry_t destr_thunk = task->data1.destructors;
- KMP_ASSERT(destr_thunk);
- destr_thunk(gtid, task);
- }
-
- // bookkeeping for resuming task:
- // GEH - note tasking_ser => task_serial
- KMP_DEBUG_ASSERT(
- (taskdata->td_flags.tasking_ser || taskdata->td_flags.task_serial) ==
- taskdata->td_flags.task_serial);
- if (taskdata->td_flags.task_serial) {
- if (resumed_task == NULL) {
- resumed_task = taskdata->td_parent; // In a serialized task, the resumed
- // task is the parent
- }
- } else {
- KMP_DEBUG_ASSERT(resumed_task !=
- NULL); // verify that resumed task is passed as argument
- }
-
// Free this task and then ancestor tasks if they have no children.
// Restore th_current_task first as suggested by John:
// johnmc: if an asynchronous inquiry peers into the runtime system
@@ -3847,20 +3854,14 @@ void __kmp_fulfill_event(kmp_event_t *event) {
bool detached = false;
int gtid = __kmp_get_gtid();
- if (taskdata->td_flags.proxy == TASK_PROXY) {
- // The associated task code completed before this call and detached.
+ // The associated task might have completed or could be completing at this
+ // point.
+ // We need to take the lock to avoid races
+ __kmp_acquire_tas_lock(&event->lock, gtid);
+ if (taskdata->td_flags.proxy == TASK_PROXY)
detached = true;
- event->type = KMP_EVENT_UNINITIALIZED;
- } else {
- // The associated task has not completed but could be completing at this
- // point.
- // We need to take the lock to avoid races
- __kmp_acquire_tas_lock(&event->lock, gtid);
- if (taskdata->td_flags.proxy == TASK_PROXY)
- detached = true;
- event->type = KMP_EVENT_UNINITIALIZED;
- __kmp_release_tas_lock(&event->lock, gtid);
- }
+ event->type = KMP_EVENT_UNINITIALIZED;
+ __kmp_release_tas_lock(&event->lock, gtid);
if (detached) {
// If the task detached complete the proxy task
More information about the Openmp-commits
mailing list