[OpenMP] Fix a few issues with hidden helper task

[hbae]

Summary

This patch includes the following changes to address a few issues when using hidden helper task.

• Assertion is triggered when there are inadvertent calls to hidden helper functions on non-Linux OS
• Added deinit code in __kmp_internal_end_library function to fix random shutdown crashes
• Moved task data access into the lock-guarded region in __kmp_push_task

[tianshilei1992]

LG. Thanks!

openmp/runtime/src/kmp_tasking.cpp

439

Why this needs to be done in lock? After the task is created, we never change this flag, don't we?

[tianshilei1992]

openmp/runtime/src/kmp_runtime.cpp

6190

Just out of curiosity, why the deinitz in __kmp_internal_end_thread is not enough?

[hbae]

openmp/runtime/src/kmp_runtime.cpp

6190

When the runtime shutdown is processed by __kmp_internal_end_library setting __kmp_global.g.g_done, __kmp_internal_end_thread does not have a chance to finalize hidden helper.

openmp/runtime/src/kmp_tasking.cpp

439

Observed some weird situations that the flag value changes after releasing the lock whereas there is nothing wrong in the lock implementation and the lock variable passed to it. Also verified that the flag is correct before invoking the actual task function. I don't have an exact root cause of the problem, so this seems to be the best we can do now.

[tianshilei1992]

openmp/runtime/src/kmp_runtime.cpp

6190

Okay, thanks for explanation.

[protze.joachim]

openmp/runtime/src/kmp_tasking.cpp

439

There is a chance, that another thread steals, executes, and finishes the task and releases the taskdata after the lock is released. So, what you observed might be a read after release.

[tianshilei1992]

openmp/runtime/src/kmp_tasking.cpp

439

Oh, yeah, it could be because the task has already been pushed.

[isuruf]

Should this be backported to release/12.x branch? Otherwise openmp on macos is broken in 12.x series.

[tianshilei1992]

In D105308#2907035, @isuruf wrote:

Should this be backported to release/12.x branch? Otherwise openmp on macos is broken in 12.x series.

Probably not given the fact the LLVM 13 is a few days ahead, but it's up to @jdoerfert

[jdoerfert]

In D105308#2908638, @tianshilei1992 wrote:

In D105308#2907035, @isuruf wrote:

Should this be backported to release/12.x branch? Otherwise openmp on macos is broken in 12.x series.

Probably not given the fact the LLVM 13 is a few days ahead, but it's up to @jdoerfert

@isuruf Feel free to file a bug, attach this commit as fix and ask Tom for backporting. This has been successful in tree for a while now, the only problem porting it back could be that there are other "fixes" missing in 12. That said, it's not up to me but I'm supportive. Make sure the patch cleanly applies to the 12 branch or create a new one to attach to the bug report.

openmp/runtime/src/kmp_runtime.cpp

#endif
   KMP_MB(); /* Flush all pending memory write invalidates.  */
 
   __kmp_cleanup();
 #if OMPT_SUPPORT
   ompt_fini();
 #endif
 }
 
 void __kmp_internal_end_library(int gtid_req) {
   /* if we have already cleaned up, don't try again, it wouldn't be pretty */
   /* this shouldn't be a race condition because __kmp_internal_end() is the
      only place to clear __kmp_serial_init */
   /* we'll check this later too, after we get the lock */
   // 2009-09-06: We do not set g_abort without setting g_done. This check looks
   // redundant, because the next check will work in any case.
   if (__kmp_global.g.g_abort) {
     KA_TRACE(11, ("__kmp_internal_end_library: abort, exiting\n"));
     /* TODO abort? */
     return;
   }
   if (TCR_4(__kmp_global.g.g_done) || !__kmp_init_serial) {
     KA_TRACE(10, ("__kmp_internal_end_library: already finished\n"));
     return;
   }
 
+  // If hidden helper team has been initialized, we need to deinit it
+  if (TCR_4(__kmp_init_hidden_helper) &&
+      !TCR_4(__kmp_hidden_helper_team_done)) {
+    TCW_SYNC_4(__kmp_hidden_helper_team_done, TRUE);
+    // First release the main thread to let it continue its work
+    __kmp_hidden_helper_main_thread_release();
+    // Wait until the hidden helper team has been destroyed
+    __kmp_hidden_helper_threads_deinitz_wait();
+  }
+
   KMP_MB(); /* Flush all pending memory write invalidates.  */
   /* find out who we are and what we should do */
   {
     int gtid = (gtid_req >= 0) ? gtid_req : __kmp_gtid_get_specific();
     KA_TRACE(
         10, ("__kmp_internal_end_library: enter T#%d  (%d)\n", gtid, gtid_req));
     if (gtid == KMP_GTID_SHUTDOWN) {
       KA_TRACE(10, ("__kmp_internal_end_library: !__kmp_init_runtime, system "
if (__kmp_global.g.g_abort) {
     return;
   }
   if (TCR_4(__kmp_global.g.g_done) || !__kmp_init_serial) {
     KA_TRACE(10, ("__kmp_internal_end_thread: already finished\n"));
     return;
   }
 
   // If hidden helper team has been initialized, we need to deinit it
-  if (TCR_4(__kmp_init_hidden_helper)) {
+  if (TCR_4(__kmp_init_hidden_helper) &&
+      !TCR_4(__kmp_hidden_helper_team_done)) {
     TCW_SYNC_4(__kmp_hidden_helper_team_done, TRUE);
     // First release the main thread to let it continue its work
     __kmp_hidden_helper_main_thread_release();
     // Wait until the hidden helper team has been destroyed
     __kmp_hidden_helper_threads_deinitz_wait();
   }
 
   KMP_MB(); /* Flush all pending memory write invalidates.  */
if (__kmp_init_serial == 0)
     __kmp_do_serial_initialize();
   __kmp_display_env_impl(!verbose, verbose);
   __kmp_release_bootstrap_lock(&__kmp_initz_lock);
 }
 
 // Globals and functions for hidden helper task
 kmp_info_t **__kmp_hidden_helper_threads;
 kmp_info_t *__kmp_hidden_helper_main_thread;
-kmp_int32 __kmp_hidden_helper_threads_num = 8;
 std::atomic<kmp_int32> __kmp_unexecuted_hidden_helper_tasks;
 #if KMP_OS_LINUX
+kmp_int32 __kmp_hidden_helper_threads_num = 8;
 kmp_int32 __kmp_enable_hidden_helper = TRUE;
 #else
+kmp_int32 __kmp_hidden_helper_threads_num = 0;
 kmp_int32 __kmp_enable_hidden_helper = FALSE;
 #endif
 
 namespace {
 std::atomic<kmp_int32> __kmp_hit_hidden_helper_threads_num;
 
 void __kmp_hidden_helper_wrapper_fn(int *gtid, int *, ...) {
   // This is an explicit synchronization on all hidden helper threads in case

openmp/runtime/src/kmp_tasking.cpp

TCW_4(thread_data->td.td_deque_ntasks,
         TCR_4(thread_data->td.td_deque_ntasks) + 1); // Adjust task count
   KMP_FSYNC_RELEASING(thread->th.th_current_task); // releasing self
   KMP_FSYNC_RELEASING(taskdata); // releasing child
   KA_TRACE(20, ("__kmp_push_task: T#%d returning TASK_SUCCESSFULLY_PUSHED: "
                 "task=%p ntasks=%d head=%u tail=%u\n",
                 gtid, taskdata, thread_data->td.td_deque_ntasks,
                 thread_data->td.td_deque_head, thread_data->td.td_deque_tail));
 
+  auto hidden_helper = taskdata->td_flags.hidden_helper;
+
   __kmp_release_bootstrap_lock(&thread_data->td.td_deque_lock);
 
   // Signal one worker thread to execute the task
-  if (taskdata->td_flags.hidden_helper) {
+  if (UNLIKELY(hidden_helper)) {
     // Wake hidden helper threads up if they're sleeping
     __kmp_hidden_helper_worker_thread_signal();
   }
 
   return TASK_SUCCESSFULLY_PUSHED;
 }
 
 // __kmp_pop_current_task_from_thread: set up current task from called thread

openmp/runtime/src/z_Linux_util.cpp

 #include "kmp_str.h"
 #include "kmp_wait_release.h"
 #include "kmp_wrapper_getpid.h"
 
 #if !KMP_OS_DRAGONFLY && !KMP_OS_FREEBSD && !KMP_OS_NETBSD && !KMP_OS_OPENBSD
 #include <alloca.h>
 #endif
 #include <math.h> // HUGE_VAL.
+#if KMP_OS_LINUX
 #include <semaphore.h>
+#endif // KMP_OS_LINUX
 #include <sys/resource.h>
 #include <sys/syscall.h>
 #include <sys/time.h>
 #include <sys/times.h>
 #include <unistd.h>
 
 #if KMP_OS_LINUX
 #include <sys/sysinfo.h>
case 15:
     break;
   }
 
   return 1;
 }
 
 #endif
 
+#if KMP_OS_LINUX
 // Functions for hidden helper task
 namespace {
 // Condition variable for initializing hidden helper team
 pthread_cond_t hidden_helper_threads_initz_cond_var;
 pthread_mutex_t hidden_helper_threads_initz_lock;
 volatile int hidden_helper_initz_signaled = FALSE;
 
 // Condition variable for deinitializing hidden helper team
void __kmp_hidden_helper_threads_deinitz_release() {
   status = pthread_cond_signal(&hidden_helper_threads_deinitz_cond_var);
   KMP_CHECK_SYSFAIL("pthread_cond_wait", status);
 
   TCW_SYNC_4(hidden_helper_deinitz_signaled, TRUE);
 
   status = pthread_mutex_unlock(&hidden_helper_threads_deinitz_lock);
   KMP_CHECK_SYSFAIL("pthread_mutex_unlock", status);
 }
+#else // KMP_OS_LINUX
+void __kmp_hidden_helper_worker_thread_wait() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_do_initialize_hidden_helper_threads() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_threads_initz_wait() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_initz_release() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_main_thread_wait() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_main_thread_release() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_worker_thread_signal() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_threads_deinitz_wait() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+
+void __kmp_hidden_helper_threads_deinitz_release() {
+  KMP_ASSERT(0 && "Hidden helper task is not supported on this OS");
+}
+#endif // KMP_OS_LINUX
 
 // end of file //