Re: [PATCH] openmp: Fix intermittent hanging of task-detach-6 libgomp tests [PR98738]

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Jan 21, 2021 at 07:33:34PM +0000, Kwok Cheung Yeung wrote:
> The detach support clearly needs more work, but is this particular patch
> okay for trunk?

Sorry for the delay.

I'm afraid it is far from being ready.

> @@ -2402,17 +2437,41 @@ ialias (omp_in_final)
>  void
>  omp_fulfill_event (omp_event_handle_t event)
>  {
> -  gomp_sem_t *sem = (gomp_sem_t *) event;
> +  struct gomp_task *task = (struct gomp_task *) event;
> +  struct gomp_task *parent = task->parent;
>    struct gomp_thread *thr = gomp_thread ();
>    struct gomp_team *team = thr ? thr->ts.team : NULL;
>  
> -  if (gomp_sem_getcount (sem) > 0)
> -    gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", sem);
> +  if (gomp_sem_getcount (&task->completion_sem) > 0)
> +    gomp_fatal ("omp_fulfill_event: %p event already fulfilled!\n", task);

As written earlier, the intent of omp_fulfill_event is that it should be
callable from anywhere, not necessarily one of the threads in the team.
The application could have other threads (often called unshackeled threads)
from which it would call it, or just some other parallel or whatever else,
as long as it is not racy to pass in the omp_event_handle_t to there.
So,
   struct gomp_thread *thr = gomp_thread ();
   struct gomp_team *team = thr ? thr->ts.team : NULL;
is incorrect, it will give you the team of the current thread, rather than
the team of the task to be fulfilled.

It can also crash if team is NULL, which will happen any time
this is called outside of a parallel.  Just try (should go into testsuite
too):
#include <omp.h>

int
main ()
{
  omp_event_handle_t ev;
  #pragma omp task detach (ev)
  omp_fulfill_event (ev);
  return 0;
}

Additionally, there is an important difference between fulfill for
included tasks and for non-included tasks, for the former there is no team
or anything to care about, for the latter there is a team and one needs to
take the task_lock, but at that point it can do pretty much everything in
omp_fulfill_event rather than handling it elsewhere.

So, what I'm suggesting is:

Replace
  bool detach;
  gomp_sem_t completion_sem;
with
  struct gomp_task_detach *detach;
and add struct gomp_task_detach that would contain everything that will be
needed (indirect so that we don't waste space for it in every task, but only
for those that have detach clause).
We need:
1) some way to tell if it is an included task or not
2) for included tasks the gomp_sem_t completion_sem
(and nothing but 1) and 2) for those),
3) struct gomp_team * for non-included tasks
4) some way to find out if the task has finished and is just waiting for
fulfill event (perhaps your GOMP_TASK_DETACHED is ok for that)
5) some way to find out if the task has been fulfilled already
(gomp_sem_t for that seems an overkill though)

1) could be done through the struct gomp_team *team; member,
set it to NULL in included tasks (no matter if they are in some team or not)
and to non-NULL team of the task (non-included tasks must have a team).

And I don't see the point of task_detach_queue if we can handle the
dependers etc. all in omp_fulfill_event, which I think we can if we take the
task_lock.

So, I think omp_fulfill_event should look at the task->detach it got,
if task->detach->team is NULL, it is included task, GOMP_task should have
initialized task->detach->completion_sem and omp_fulfill_event should just
gomp_sem_post it and that is all, GOMP_task for included task needs to
gomp_sem_wait after it finishes before it returns.

Otherwise, take the team's task_lock, and look at whether the task is still
running, in that case just set the bool that it has been fulfilled (or
whatever way of signalling 5), perhaps it can be say clearing task->detach
pointer).  When creating non-included tasks in GOMP_task with detach clause
through gomp_malloc, it would add the size needed for struct
gomp_task_detach.
But if the task is already in GOMP_TASK_DETACHED state, instead we need
while holding the task_lock do everything that would have been done normally
on task finish, but we've skipped it because it hasn't been fulfilled.
Including the waking/sem_posts when something could be waiting on that task.

Do you agree with this, or see some reason why this can't work?

And testsuite should include also cases where we wait for the tasks with
detach clause to be fulfilled at the end of taskgroup (i.e. need to cover
all of taskwait, taskgroup end and barrier).

	Jakub