[Openmp-commits] [PATCH] D28377: Fix a race in shutdown when tasking is used

[Terry Wilmarth via Phabricator via Openmp-commits]

tlwilmar added a comment.

Hi Jonas, 
There are probably numerous ways of doing this.  I answered your comments with why I did it this way.  
Thanks!
Terry



================
Comment at: runtime/src/kmp_runtime.cpp:4007-4012
+    if (__kmp_tasking_mode != tskm_immediate_exec)
+        // When tasking is possible, threads are not safe to reap until they are
+        // done tasking; this will be set when tasking code is exited in wait
+        this_thr->th.th_reap_state = KMP_NOT_SAFE_TO_REAP;
+    else  // no tasking --> always safe to reap
+        this_thr->th.th_reap_state = KMP_SAFE_TO_REAP;
----------------
Hahnfeld wrote:
> Do we need this here or would it be enough to have the flag completely handled in `__kmp_execute_tasks_template`?
> 
> I don't know whether that would create a race on `th_reap_state`. If `__kmp_execute_tasks_template` is not guaranteed to be called at least once before a barrier finishes, why aren't there problems with multiple parallel regions? Each thread will have `th.th_reap_state = KMP_SAFE_TO_REAP` at the end of the first parallel region...
__kmp_execute_tasks_template may not be called by all threads, and it may be called multiple times by individual threads, so it's often premature to set the flag to SAFE inside. __kmp_initialize_info will be called to reset th_reap_state for each thread.


================
Comment at: runtime/src/kmp_wait_release.h:215-224
+                    else
+                        this_thr->th.th_reap_state = KMP_SAFE_TO_REAP;
                 }
                 else {
                     KMP_DEBUG_ASSERT(!KMP_MASTER_TID(this_thr->th.th_info.ds.ds_tid));
                     this_thr->th.th_task_team = NULL;
+                    this_thr->th.th_reap_state = KMP_SAFE_TO_REAP;
----------------
Hahnfeld wrote:
> Resetting to `th.th_reap_state = KMP_SAFE_TO_REAP` could then be done at the end of `__kmp_execute_tasks_template`
As mentioned above, we want to avoid prematurely setting the thread as safe to reap.  Note that the cases in which we set the thread as safe to reap are when 1) no tasks have been encountered by any threads; 2) the task team is no longer active; 3) the current thread's task team is NULL.  The case inside of __kmp_execute_tasks_template only amounts to "this thread couldn't find any more tasks after randomly searching for some".


Repository:
  rL LLVM

https://reviews.llvm.org/D28377