From patchwork Wed Jan 23 01:11:52 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Modra X-Patchwork-Id: 214706 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]) by ozlabs.org (Postfix) with SMTP id C5BC92C0040 for ; Wed, 23 Jan 2013 12:12:35 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1359508356; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:Date:From:To:Cc:Subject:Message-ID:Mail-Followup-To: References:MIME-Version:Content-Type:Content-Disposition: In-Reply-To:User-Agent:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=S8h1yFN+MqruH9oYMbp449gRtUE=; b=Gvl0cYbi83SULgq pBFFJdg8VSQSAaXdTLLysS6cz5l30HKK119astouM5w5Z9qTHYLp0MkqZ/ZDyBnJ YEWUK2mc0Z9J2WQeKI/txe2MjM0NiTgu9d/m60Z17sreJS8ItIdvb2Z7SUcbvubU cijvb+SMkvBQCu3D6tGX6jQs0yok= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Received:Received:Received:Date:From:To:Cc:Subject:Message-ID:Mail-Followup-To:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:User-Agent:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=gC3Ry8+rFJnDX1cC3aB1Q4nLXh+Zdh1wfWQBQM8EeIsCgtgTJMqMNBCiXR2ddI MHRcuJryHABJDL+HbmL09+OyDH4gZQ29nO0V28827ZSNvILyi383769hOimIt28L Y5O5lIVW85PlXEj8DUqBDGu7vqrR26AIAUcS1NiUjmChA=; Received: (qmail 23080 invoked by alias); 23 Jan 2013 01:12:09 -0000 Received: (qmail 23042 invoked by uid 22791); 23 Jan 2013 01:12:08 -0000 X-SWARE-Spam-Status: No, hits=-4.3 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, KHOP_RCVD_TRUST, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-pa0-f48.google.com (HELO mail-pa0-f48.google.com) (209.85.220.48) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 23 Jan 2013 01:12:00 +0000 Received: by mail-pa0-f48.google.com with SMTP id fa1so4420239pad.21 for ; Tue, 22 Jan 2013 17:12:00 -0800 (PST) X-Received: by 10.66.86.71 with SMTP id n7mr61240563paz.77.1358903519996; Tue, 22 Jan 2013 17:11:59 -0800 (PST) Received: from bubble.grove.modra.org ([101.166.26.37]) by mx.google.com with ESMTPS id uh9sm11681410pbc.65.2013.01.22.17.11.56 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 22 Jan 2013 17:11:58 -0800 (PST) Received: by bubble.grove.modra.org (Postfix, from userid 1000) id 51D73EA1F62; Wed, 23 Jan 2013 11:41:52 +1030 (CST) Date: Wed, 23 Jan 2013 11:41:52 +1030 From: Alan Modra To: Torvald Riegel Cc: gcc-patches@gcc.gnu.org, Jakub Jelinek , Richard Henderson Subject: Re: PR libgomp/56073: benchmark regression due to PR libgomp/51376 fix Message-ID: <20130123011152.GO3244@bubble.grove.modra.org> Mail-Followup-To: Torvald Riegel , gcc-patches@gcc.gnu.org, Jakub Jelinek , Richard Henderson References: <20130122110324.GL3244@bubble.grove.modra.org> <1358860823.3101.225.camel@triegel.csb> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1358860823.3101.225.camel@triegel.csb> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 On Tue, Jan 22, 2013 at 02:20:23PM +0100, Torvald Riegel wrote: > > @@ -116,11 +116,10 @@ GOMP_task (void (*fn) (void *), void *data, void ( > > } > > else > > fn (data); > > - if (team != NULL) > > + if (task.children != NULL) > > Why does this not need an acquire barrier? I explained that in my patch submission. Briefly, the only way this particular task.children can be set is if this thread's task work function creates children. So since the setter is *this* thread, we need no barriers. We can have task.children set by the current thread then changed by a child thread, but seeing a stale non-NULL value is not a problem here. Once past the task_lock acquisition, this thread will see the real value of task.children. > > { > > gomp_mutex_lock (&team->task_lock); > > - if (task.children != NULL) > > Are you sure that you can remove this check to task.children here, > especially if you don't use an acquire barrier previously? > > Can there be several threads trying to call gomp_clear_parent? Yes and yes. gomp_clear_parent handles a NULL arg, and as you can see, runs inside a task_lock mutex region here. > > - gomp_clear_parent (task.children); > > + gomp_clear_parent (task.children); > > gomp_mutex_unlock (&team->task_lock); > > Please add comments or reference the comments in the other pieces of > synchronizing code related to this. Please note that all of the above is a reversion! Adding a comment has the risk that the comment is *wrong*. That said, I appreciate the need to document threaded code well.. > > - if (task == NULL || team == NULL) > > + if (task == NULL > > + || __atomic_load_n (&task->children, MEMMODEL_ACQUIRE) == NULL) > > Please mention with which stores this acquire load synchronizes. How does this look? * task.c (GOMP_task, GOMP_taskwait): Comment. Index: libgomp/task.c =================================================================== --- libgomp/task.c (revision 195370) +++ libgomp/task.c (working copy) @@ -116,6 +116,15 @@ } else fn (data); + /* Access to "children" is normally done inside a task_lock + mutex region, but the only way this particular task.children + can be set is if this thread's task work function (fn) + creates children. So since the setter is *this* thread, we + need no barriers here when testing for non-NULL. We can have + task.children set by the current thread then changed by a + child thread, but seeing a stale non-NULL value is not a + problem. Once past the task_lock acquisition, this thread + will see the real value of task.children. */ if (task.children != NULL) { gomp_mutex_lock (&team->task_lock); @@ -296,6 +305,12 @@ struct gomp_task *child_task = NULL; struct gomp_task *to_free = NULL; + /* The acquire barrier on load of task->children here synchronizes + with the write of a NULL in gomp_barrier_handle_tasks. It is + not necessary that we synchronize with other non-NULL writes at + this point, but we must ensure that all writes to memory by a + child thread task work function are seen before we exit from + GOMP_taskwait. */ if (task == NULL || __atomic_load_n (&task->children, MEMMODEL_ACQUIRE) == NULL) return;