diff mbox

PR libgomp/51376 fix

Message ID 20111202094011.GJ28621@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Dec. 2, 2011, 9:40 a.m. UTC 
On Thu, Dec 01, 2011 at 12:36:08PM +0100, Jakub Jelinek wrote:
> On Thu, Dec 01, 2011 at 09:58:08PM +1030, Alan Modra wrote:
> > The GOMP_task change fixes a similar potential problem.  Bootstrapped
> > and regression tested powerpc-linux.  OK to apply?
> > 
> > 	PR libgomp/51376
> > 	* task.c (GOMP_taskwait): Don't access task->children outside of
> > 	task_lock mutex region.
> > 	(GOMP_task): Likewise.
> 
> Can't this be solved just by adding a barrier?  The access to the var
> outside of the lock has been quite intentional, to avoid locking in the
> common case where there are no children.

No, I tried that and the task-6.C testcase still failed although not
quite as readily.  I was using

if (task == NULL
    || __atomic_load_n (&task->children, MEMMODEL_ACQUIRE) == 0)

You need a release in the child as well as the acquire to ensure
proper synchronisation, and there's a window for failure between the
child clearing task->children and performing a release as part of the
mutex unlock.

Oops, on looking at this email I see I attached an old patch..  This
one avoids a segfault on trying to lock team->task_lock when there
is no team.  This one really has been bootstrapped and regression
tested successfully.

	PR libgomp/51376
	* task.c (GOMP_taskwait): Don't access task->children outside of
	task_lock mutex region.
	(GOMP_task): Likewise.

Comments

Alan Modra Dec. 3, 2011, 1:22 p.m. UTC | #1 
On Fri, Dec 02, 2011 at 08:10:11PM +1030, Alan Modra wrote:
> On Thu, Dec 01, 2011 at 12:36:08PM +0100, Jakub Jelinek wrote:
> > On Thu, Dec 01, 2011 at 09:58:08PM +1030, Alan Modra wrote:
> > > The GOMP_task change fixes a similar potential problem.  Bootstrapped
> > > and regression tested powerpc-linux.  OK to apply?
> > > 
> > > 	PR libgomp/51376
> > > 	* task.c (GOMP_taskwait): Don't access task->children outside of
> > > 	task_lock mutex region.
> > > 	(GOMP_task): Likewise.
> > 
> > Can't this be solved just by adding a barrier?  The access to the var
> > outside of the lock has been quite intentional, to avoid locking in the
> > common case where there are no children.
> 
> No, I tried that and the task-6.C testcase still failed although not
> quite as readily.  I was using
> 
> if (task == NULL
>     || __atomic_load_n (&task->children, MEMMODEL_ACQUIRE) == 0)
> 
> You need a release in the child as well as the acquire to ensure
> proper synchronisation, and there's a window for failure between the
> child clearing task->children and performing a release as part of the
> mutex unlock.

Perhaps I misunderstood your question.  I suppose you could solve the
problem by adding a barrier, if by that you meant the acquire barrier
in GOMP_taskwait as I tried before I fully understood the problem,
plus the corresponding release barrier in gomp_barrier_handle_tasks.
ie. replace
		      parent->children = NULL;
with
		      __atomic_write_n (&parent->children, NULL,
					MEMMODEL_RELEASE);

That should work, but of course will slow down what I imagine is a
common multi-thread case where child threads don't complete before the
parent gets around to a taskwait.  Do you really want to do that?
Alan Modra Dec. 9, 2011, 2:23 a.m. UTC | #2 
On Fri, Dec 02, 2011 at 08:10:11PM +1030, Alan Modra wrote:
> 	PR libgomp/51376
> 	* task.c (GOMP_taskwait): Don't access task->children outside of
> 	task_lock mutex region.
> 	(GOMP_task): Likewise.

Committed revision 182151 on rth's irc OK.
diff mbox

Patch

Index: libgomp/task.c
===================================================================
--- libgomp/task.c	(revision 181902)
+++ libgomp/task.c	(working copy)
@@ -116,10 +116,11 @@  GOMP_task (void (*fn) (void *), void *da
 	}
       else
 	fn (data);
-      if (task.children)
+      if (team != NULL)
 	{
 	  gomp_mutex_lock (&team->task_lock);
-	  gomp_clear_parent (task.children);
+	  if (task.children != NULL)
+	    gomp_clear_parent (task.children);
 	  gomp_mutex_unlock (&team->task_lock);
 	}
       gomp_end_task ();
@@ -290,8 +291,9 @@  GOMP_taskwait (void)
   struct gomp_task *child_task = NULL;
   struct gomp_task *to_free = NULL;
 
-  if (task == NULL || task->children == NULL)
+  if (task == NULL || team == NULL)
     return;
+
   gomp_mutex_lock (&team->task_lock);
   while (1)
     {