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

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Feb 25, 2021 at 04:21:31PM +0000, Kwok Cheung Yeung wrote:
> Reversing the order reduces the hole to 3 bytes:
> 
>         size_t                     num_dependees;        /*    80     8 */
>         union {
>                 gomp_sem_t *       completion_sem;       /*    88     8 */
>                 struct gomp_team * detach_team;          /*    88     8 */
>         };                                               /*    88     8 */
>         _Bool                      deferred_p;           /*    96     1 */
> 
>         /* XXX 3 bytes hole, try to pack */
> 
>         int                        priority;             /*   100     4 */
> 
> If we were really determined to get rid of the hole, we could stash
> deferred_p in the least-significant bit of the pointer union, but I think

Sorry, indeed, I was thinking about how it would be packed after priority,
not before it but putting it in between priority and the related prio queues
is undesirable.  So union, deferred_p, priority is the right order.

> > I think you can move task->deferred_p into the if stmt.
> 
> That can be done (since the code for detach is currently the only thing
> using it), but I think it would be better to have deferred_p always have the
> right value, regardless of whether or not it is used? Otherwise that might
> lead to some confusion if it is later used by something else.

Ok either way.

> I will get this committed later if the regression tests finish with no surprises.

Two more nits below.

> @@ -86,7 +87,8 @@ gomp_init_task (struct gomp_task *task, struct gomp_task *parent_task,
>    task->dependers = NULL;
>    task->depend_hash = NULL;
>    task->depend_count = 0;
> -  task->detach = false;
> +  task->deferred_p = true;
> +  task->detach_team = NULL;
>  }

Please initialize deferred_p to false rather than true, because gomp_init_task
is called in many places and except for that one spot in GOMP_task (and one
in taskloop.c) the tasks are undeferred (e.g. the implicit tasks in parallel
or the initial one etc.).
And maybe also reorder the fields initialized in there to match the order of increasing
field offsets.

> @@ -414,16 +411,18 @@ GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
>        task.final_task = (thr->task && thr->task->final_task)
>  			|| (flags & GOMP_TASK_FLAG_FINAL);
>        task.priority = priority;
> +      task.deferred_p = false;

And then this shouldn't be here, gomp_init_task has already initialized it
that way.

	Jakub