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

[Terry Wilmarth via Phabricator via Openmp-commits]

tlwilmar 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;
----------------
Hahnfeld wrote:
> 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!
> ```
You're right... but...  I think that we need to set to NOT SAFE whenever we come out of the spin loop in order to reset before both fork and join barriers.  However, that impacts how we free and reap the threads.  I'll have to tinker with this a bit.


Repository:
  rL LLVM

https://reviews.llvm.org/D28377