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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added inline comments.


================
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;
----------------
tlwilmar wrote:
> 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.
`__kmp_initialize_info` is not called if a hot team is reused with either the same or a lower number of threads.

```lang=C
int i;
for (i = 0; i < 2; i++) {
	#pragma omp parallel num_threads(2)
	{
		#pragma omp single nowait
		#pragma omp task
		{ printf("Executed by thread #%d!\n", omp_get_thread_num()); }
	}
}
```

with `$ KMP_F_DEBUG=10 ./crash2 3>&1 1>&2 2>&3 | grep -E __kmp_initialize_info1` (sorry for the pipes!)
```
__kmp_initialize_info1: T#0:0 this_thread=0x60abc0 curtask=(nil)
__kmp_initialize_info1: T#1:1 this_thread=0x617f00 curtask=(nil)
__kmp_initialize_info1: T#0:0 this_thread=0x60abc0 curtask=0x607980
__kmp_initialize_info1: T#1:1 this_thread=0x617f00 curtask=0x610480
Executed by thread #1!
Executed by thread #1!
Finished parallel regions!
```


================
Comment at: runtime/src/kmp_runtime.cpp:4348
 static void
 __kmp_reinitialize_team( kmp_team_t *team, kmp_internal_control_t *new_icvs, ident_t *loc ) {
     KF_TRACE( 10, ( "__kmp_reinitialize_team: enter this_thread=%p team=%p\n",
----------------
This function is called when a team is reused, maybe we have to add it here?


================
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;
----------------
tlwilmar wrote:
> 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".
Ah, all right, I forgot that returning from `__kmp_execute_tasks_template` does not mean that all tasks are finished!


Repository:
  rL LLVM

https://reviews.llvm.org/D28377