From: Joachim Protze Date: Sun, 17 May 2020 10:25:02 +0000 (+0200) Subject: [OpenMP] Fix race condition in the completion/freeing of detached tasks X-Git-Tag: llvmorg-12-init~5850 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d23131a3c063830e3d8d4f7d43cbcf95d92db3d3;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/openmp/runtime/src/kmp_tasking.cpp b/openmp/runtime/src/kmp_tasking.cpp index 1cfc66f..fe8bcad 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