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

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Jan 29, 2021 at 7:53 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> 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).
>

task-detach-6.f90 should be disabled for now.  It has been blocking my testers
for weeks.

--
H.J.