From patchwork Fri Oct 9 16:38:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 528311 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 8F598140549 for ; Sat, 10 Oct 2015 03:38:58 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=P+6WQDr0; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=FUVpOYPcrJsXgm3x+ rk5+GO4HXE11RrcrRQvl/DlKSqhbmPHKh3s0fpg03KJA2ZNzJpIja1kSVj3mop7F uknp548kgYH1rgxdNJpvxX63fgR+Puwh2O+yKFSbEPtCKTC0bS1J43H3iVcCCzuJ I4AX06lpTGsAuAHxApWXh+MEpA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=QpMCSyrcDSgf3MU+4gkghsd TVH8=; b=P+6WQDr040cU9KSmXEj0ThdpDE+DbGZ15Zfb03QzprAS5xgs6mrLHtW wOaH5iJ7HSLo20jCqaziAh3qaUYrQa2U2jjotjea+jJOc8Hj5xEDN0A3txE5HW5x Fq4H91h/d3En3Fjr2AxAMpjfTy9hdyTUwOAUgNNBfXyiwgQA3OJ8= Received: (qmail 92578 invoked by alias); 9 Oct 2015 16:38:50 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 92044 invoked by uid 89); 9 Oct 2015 16:38:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.0 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 09 Oct 2015 16:38:48 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 23489300AAA for ; Fri, 9 Oct 2015 16:38:47 +0000 (UTC) Received: from reynosa.quesejoda.com (unused [10.10.51.20] (may be forged)) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t99Gcjr7008425; Fri, 9 Oct 2015 12:38:46 -0400 Subject: Re: [gomp4.1] fix more scheduling inconsistencies and add verification routines To: Jakub Jelinek References: <56116160.70307@redhat.com> Cc: gcc-patches From: Aldy Hernandez Message-ID: <5617ED90.9050808@redhat.com> Date: Fri, 9 Oct 2015 09:38:40 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <56116160.70307@redhat.com> 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 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(). 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 #include #include #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)