[gomp4.1] fix more scheduling inconsistencies and add verification routines
diff mbox

Message ID 5617ED90.9050808@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Oct. 9, 2015, 4:38 p.m. UTC
As per our IRC discussion.

I am conditionally compiling the verification code because you mentioned 
that the GPGPUs may not having a working printf.

Also, I removed the code caching the workgroup since it may contain the 
incorrect workgroup as I had suggested.  Now instead we look in 
child_task->taskgroup which will have the correct workgroup always.

Tested on x86-64 Linux with make check-target-libgomp for a variety of 
different OMP_NUM_THREADS and with _ENABLE_LIBGOMP_CHECKING_ set to 1.

OK for branch?

p.s. As a thought, maybe we can set _ENABLE_LIBGOMP_CHECKING_ to 1 until 
release?
commit 6d27e5511720d2e77d037720dd32f86cc57d42f0
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Sun Oct 4 09:52:49 2015 -0700

    	* libgomp.h (_LIBGOMP_CHECKING_): New macro.
    	* task.c (verify_children_queue): New.
    	(verify_taskgroup_queue): New.
    	(verify_task_queue): New.
    	(gomp_task_run_pre): Call verify_*_queue functions.
    	If an upcoming tied task is about to leave the sibling or
    	taskgroup queues in an invalid state, adjust appropriately.
    	(GOMP_taskgroup_end): Do not pass taskgroup to gomp_task_run_pre().

Comments

Jakub Jelinek Oct. 9, 2015, 4:47 p.m. UTC | #1
On Fri, Oct 09, 2015 at 09:38:40AM -0700, Aldy Hernandez wrote:
> As per our IRC discussion.
> 
> I am conditionally compiling the verification code because you mentioned
> that the GPGPUs may not having a working printf.

Both that and code size being important there.

> Also, I removed the code caching the workgroup since it may contain the
> incorrect workgroup as I had suggested.  Now instead we look in
> child_task->taskgroup which will have the correct workgroup always.
> 
> Tested on x86-64 Linux with make check-target-libgomp for a variety of
> different OMP_NUM_THREADS and with _ENABLE_LIBGOMP_CHECKING_ set to 1.
> 
> OK for branch?

Yes, with a small change (just compile test with
-D_ENABLE_LIBGOMP_CHECKING_=1, no need to retest otherwise):

> p.s. As a thought, maybe we can set _ENABLE_LIBGOMP_CHECKING_ to 1 until
> release?

I'd prefer not to, it will affect the timing and slow down already slow
testing of libgomp for everybody.

> --- a/libgomp/task.c
> +++ b/libgomp/task.c
> @@ -27,6 +27,7 @@
>     creation and termination.  */
>  
>  #include "libgomp.h"
> +#include <stdio.h>

Please nuke the above.

> +  if (task->parent != parent)
> +    {
> +      fprintf (stderr, "verify_children_queue: incompatible parents\n");
> +      abort ();
> +    }

and just use
  if (task->parent != parent)
    gomp_fatal ("verify_children_queue: incompatible parents");
instead.  Note no \n at the end.
Ditto for all other fprintf + abort pairs.
gomp_fatal accepts printf style formatting string, so you can even
handle it in:

> +      fprintf (stderr, "verify_taskgroup_queue: incompatible taskgroups\n");
> +      fprintf (stderr, "%p %p\n", task->taskgroup, taskgroup);
> +      abort ();

this case, just use a single fmt string, and just use space instead of \n in
the middle.

	Jakub

Patch
diff mbox

diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index d798321..19b3dab 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -36,6 +36,11 @@ 
 #ifndef LIBGOMP_H 
 #define LIBGOMP_H 1
 
+#ifndef _LIBGOMP_CHECKING_
+/* Define to 1 to perform internal sanity checks.  */
+#define _LIBGOMP_CHECKING_ 0
+#endif
+
 #include "config.h"
 #include "gstdint.h"
 #include "libgomp-plugin.h"
diff --git a/libgomp/task.c b/libgomp/task.c
index 7a1373a..306fdd6 100644
--- a/libgomp/task.c
+++ b/libgomp/task.c
@@ -27,6 +27,7 @@ 
    creation and termination.  */
 
 #include "libgomp.h"
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include "gomp-constants.h"
@@ -567,10 +568,153 @@  gomp_create_target_task (struct gomp_device_descr *devicep,
     gomp_team_barrier_wake (&team->barrier, 1);
 }
 
+#if _LIBGOMP_CHECKING
+/* Sanity check TASK to make sure it is in its parent's children
+   queue, and that the tasks therein are in the right order.
+
+   The expected order is:
+	parent_depends_on WAITING tasks
+	!parent_depends_on WAITING tasks
+	TIED tasks
+
+   PARENT is the alleged parent of TASK.  */
+
+static void
+verify_children_queue (struct gomp_task *task, struct gomp_task *parent)
+{
+  if (task->parent != parent)
+    {
+      fprintf (stderr, "verify_children_queue: incompatible parents\n");
+      abort ();
+    }
+  /* It's OK, Annie was an orphan and she turned out all right.  */
+  if (!parent)
+    return;
+
+  bool seen_tied = false;
+  bool seen_plain_waiting = false;
+  bool found = false;
+  struct gomp_task *t = parent->children;
+  while (1)
+    {
+      if (t == task)
+	found = true;
+      if (seen_tied && t->kind == GOMP_TASK_WAITING)
+	{
+	  fprintf (stderr,
+		   "verify_children_queue: WAITING task after TIED.");
+	  abort ();
+	}
+      if (t->kind == GOMP_TASK_TIED)
+	seen_tied = true;
+      else if (t->kind == GOMP_TASK_WAITING)
+	{
+	  if (t->parent_depends_on)
+	    {
+	      if (seen_plain_waiting)
+		{
+		  fprintf (stderr,
+			   "verify_children_queue: parent_depends_on after "
+			   "!parent_depends_on\n");
+		  abort ();
+		}
+	    }
+	  else
+	    seen_plain_waiting = true;
+	}
+      t = t->next_child;
+      if (t == parent->children)
+	break;
+    }
+  if (!found)
+    {
+      fprintf (stderr,
+	       "verify_children_queue: child not found in parent queue\n");
+      abort ();
+    }
+}
+
+/* Sanity check TASK to make sure it is in its taskgroup queue (if
+   applicable), and that the tasks therein are in the right order.
+
+   The expected order is that GOMP_TASK_WAITING tasks must come before
+   GOMP_TASK_TIED tasks.
+
+   TASK is the task.  TASKGROUP is the alleged taskgroup that contains
+   TASK.  */
+
+static void
+verify_taskgroup_queue (struct gomp_task *task,
+			struct gomp_taskgroup *taskgroup)
+{
+  if (taskgroup != task->taskgroup)
+    {
+      fprintf (stderr, "verify_taskgroup_queue: incompatible taskgroups\n");
+      fprintf (stderr, "%p %p\n", task->taskgroup, taskgroup);
+      abort ();
+    }
+  if (!taskgroup)
+    return;
+
+  bool seen_tied = false;
+  bool found = false;
+  struct gomp_task *t = taskgroup->children;
+  while (1)
+    {
+      if (t == task)
+	found = true;
+      if (t->kind == GOMP_TASK_WAITING && seen_tied)
+	{
+	  fprintf (stderr,
+		   "verify_taskgroup_queue: WAITING task after TIED.\n");
+	  abort ();
+	}
+      if (t->kind == GOMP_TASK_TIED)
+	seen_tied = true;
+      t = t->next_taskgroup;
+      if (t == taskgroup->children)
+	break;
+    }
+  if (!found)
+    {
+      fprintf (stderr,
+	       "verify_taskgroup_queue: child not found in parent queue\n");
+      abort ();
+    }
+}
+
+/* Verify that TASK is in the team's task queue.  */
+
+static void
+verify_task_queue (struct gomp_task *task, struct gomp_team *team)
+{
+  struct gomp_task *t = team->task_queue;
+  if (team)
+    while (1)
+      {
+	if (t == task)
+	  return;
+	t = t->next_queue;
+	if (t == team->task_queue)
+	  break;
+      }
+  fprintf (stderr, "verify_team_queue: child not in team\n");
+  abort ();
+}
+#endif
+
 static inline bool
 gomp_task_run_pre (struct gomp_task *child_task, struct gomp_task *parent,
-		   struct gomp_taskgroup *taskgroup, struct gomp_team *team)
+		   struct gomp_team *team)
 {
+  struct gomp_taskgroup *taskgroup = child_task->taskgroup;
+#if _LIBGOMP_CHECKING
+  verify_children_queue (child_task, parent);
+  verify_taskgroup_queue (child_task,
+			  taskgroup ? taskgroup : child_task->taskgroup);
+  verify_task_queue (child_task, team);
+#endif
+
   if (parent)
     {
       /* Adjust children such that it will point to a next child,
@@ -583,6 +727,21 @@  gomp_task_run_pre (struct gomp_task *child_task, struct gomp_task *parent,
 	 by GOMP_taskwait).  */
       if (parent->children == child_task)
 	parent->children = child_task->next_child;
+      /* TIED tasks cannot come before WAITING tasks.  If we're about
+	 to make this task TIED, rewire things appropriately.
+	 However, a TIED task at the end is perfectly fine.  */
+      else if (child_task->next_child->kind == GOMP_TASK_WAITING
+	       && child_task->next_child != parent->children)
+	{
+	  /* Remove from the list.  */
+	  child_task->prev_child->next_child = child_task->next_child;
+	  child_task->next_child->prev_child = child_task->prev_child;
+	  /* Rewire at the end of its siblings.  */
+	  child_task->next_child = parent->children;
+	  child_task->prev_child = parent->children->prev_child;
+	  parent->children->prev_child->next_child = child_task;
+	  parent->children->prev_child = child_task;
+	}
 
       /* If the current task (child_task) is at the top of the
 	 parent's last_parent_depends_on, it's about to be removed
@@ -610,8 +769,28 @@  gomp_task_run_pre (struct gomp_task *child_task, struct gomp_task *parent,
   /* Adjust taskgroup to point to the next taskgroup.  See note above
      regarding adjustment of children as to why the child_task is not
      removed entirely from the circular list.  */
-  if (taskgroup && taskgroup->children == child_task)
-    taskgroup->children = child_task->next_taskgroup;
+  if (taskgroup)
+    {
+      if (taskgroup->children == child_task)
+	taskgroup->children = child_task->next_taskgroup;
+      /* TIED tasks cannot come before WAITING tasks.  If we're about
+	 to make this task TIED, rewire things appropriately.
+	 However, a TIED task at the end is perfectly fine.  */
+      else if (child_task->next_taskgroup->kind == GOMP_TASK_WAITING
+	       && child_task->next_taskgroup != taskgroup->children)
+	{
+	  /* Remove from the list.  */
+	  child_task->prev_taskgroup->next_taskgroup
+	    = child_task->next_taskgroup;
+	  child_task->next_taskgroup->prev_taskgroup
+	    = child_task->prev_taskgroup;
+	  /* Rewire at the end of its taskgroup.  */
+	  child_task->next_taskgroup = taskgroup->children;
+	  child_task->prev_taskgroup = taskgroup->children->prev_taskgroup;
+	  taskgroup->children->prev_taskgroup->next_taskgroup = child_task;
+	  taskgroup->children->prev_taskgroup = child_task;
+	}
+    }
 
   /* Remove child_task from the task_queue.  */
   child_task->prev_queue->next_queue = child_task->next_queue;
@@ -907,7 +1086,7 @@  gomp_barrier_handle_tasks (gomp_barrier_state_t state)
 	{
 	  child_task = team->task_queue;
 	  cancelled = gomp_task_run_pre (child_task, child_task->parent,
-					 child_task->taskgroup, team);
+					 team);
 	  if (__builtin_expect (cancelled, 0))
 	    {
 	      if (to_free)
@@ -1020,8 +1199,7 @@  GOMP_taskwait (void)
 	{
 	  child_task = task->children;
 	  cancelled
-	    = gomp_task_run_pre (child_task, task, child_task->taskgroup,
-				 team);
+	    = gomp_task_run_pre (child_task, task, team);
 	  if (__builtin_expect (cancelled, 0))
 	    {
 	      if (to_free)
@@ -1222,8 +1400,7 @@  gomp_task_maybe_wait_for_dependencies (void **depend)
 	{
 	  child_task = task->children;
 	  cancelled
-	    = gomp_task_run_pre (child_task, task, child_task->taskgroup,
-				 team);
+	    = gomp_task_run_pre (child_task, task, team);
 	  if (__builtin_expect (cancelled, 0))
 	    {
 	      if (to_free)
@@ -1379,8 +1556,7 @@  GOMP_taskgroup_end (void)
       if (child_task->kind == GOMP_TASK_WAITING)
 	{
 	  cancelled
-	    = gomp_task_run_pre (child_task, child_task->parent, taskgroup,
-				 team);
+	    = gomp_task_run_pre (child_task, child_task->parent, team);
 	  if (__builtin_expect (cancelled, 0))
 	    {
 	      if (to_free)