From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16460 invoked by alias); 17 Dec 2018 17:46:47 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 16447 invoked by uid 89); 17 Dec 2018 17:46:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=sk:thomas, consolidated, U*thomas, H*Ad:U*julian X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Dec 2018 17:46:43 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=svr-ies-mbx-01.mgc.mentorg.com) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1gYwyV-0004w8-4r from Thomas_Schwinge@mentor.com ; Mon, 17 Dec 2018 09:46:39 -0800 Received: from hertz.schwinge.homeip.net (137.202.0.90) by svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Mon, 17 Dec 2018 17:46:35 +0000 From: Thomas Schwinge To: Chung-Lin Tang CC: , , Jakub Jelinek , Julian Brown Subject: Re: [PATCH 0/6, OpenACC, libgomp] Async re-work In-Reply-To: <091ccda8-a455-402e-0f10-7852c3e2606a@mentor.com> References: <432c2e58-7bf6-1f7e-457f-32813207b282@mentor.com> <20181206222246.1cceb504@squid.athome> <20181206222621.16ec00f0@squid.athome> <091ccda8-a455-402e-0f10-7852c3e2606a@mentor.com> User-Agent: Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/25.2.2 (x86_64-pc-linux-gnu) Date: Mon, 17 Dec 2018 17:46:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-SW-Source: 2018-12/txt/msg01235.txt.bz2 Hi Chung-Lin! On Fri, 14 Dec 2018 22:28:58 +0800, Chung-Lin Tang wrote: > On 2018/12/13 11:51 PM, Thomas Schwinge wrote: > > On Thu, 13 Dec 2018 23:28:49 +0800, Chung-Lin Tang wrote: > >> On 2018/12/7 6:26 AM, Julian Brown wrote: > >>> On Thu, 6 Dec 2018 22:22:46 +0000 > >>> Julian Brown wrote: > >>> > >>>> On Thu, 6 Dec 2018 21:42:14 +0100 > >>>> Thomas Schwinge wrote: > >>>> > >>>>> [...] > >>>>> ..., where the "Invalid read of size 8" happens, and which > >>>>> eventually would try to "free (tgt)" again, via > >>>>> libgomp/target.c:gomp_unmap_tgt: > >>>>> > >>>>> attribute_hidden void > >>>>> gomp_unmap_tgt (struct target_mem_desc *tgt) > >>>>> { > >>>>> /* Deallocate on target the tgt->tgt_start .. tgt->tgt_end > >>>>> region. */ if (tgt->tgt_end) > >>>>> gomp_free_device_memory (tgt->device_descr, tgt->to_free); > >>>>>=20=20=20=20=20=20=20 > >>>>> free (tgt->array); > >>>>> free (tgt); > >>>>> } > >>>>> > >>>>> Is the "free (tgt)" in libgomp/target.c:gomp_unmap_vars_async wrong, > >>>>> or something else? >=20 > I think I understand the problem now. In gomp_unmap_vars_async(), in the = case of > tgt->list_count =3D=3D 0 (i.e. no map arguments at all) the code should s= imply free the tgt > and return, while the code in goacc_async_copyout_unmap_vars() didn't han= dle this case > and always scheduled an asynchronous free of the tgt later, causing that = valgrind error > you see. >=20 > I am still testing the attached patch, but I think it is the right fix: I= reviewed what I > wrote and it seemed the way I organized things into a goacc_async_copyout= _unmap_vars() routine, > including the hackish refcount++, etc. is simply unneeded. I have deleted= those stuff > and consolidated things back into gomp_unmap_vars_async(). >=20 > I'll update the whole patches later after complete testing, the attached = is the patch atop > of the prior async patches. (the small program you gave above does pass v= algrind now) Thanks, confirmed. Gr=C3=BC=C3=9Fe Thomas > diff -ru trunk-orig/libgomp/oacc-async.c trunk-work/libgomp/oacc-async.c > --- trunk-orig/libgomp/oacc-async.c 2018-12-14 21:06:06.649794724 +0800 > +++ trunk-work/libgomp/oacc-async.c 2018-12-14 22:11:29.252251925 +0800 > @@ -238,31 +238,6 @@ > thr->default_async =3D async; > } >=20=20 > -static void > -goacc_async_unmap_tgt (void *ptr) > -{ > - struct target_mem_desc *tgt =3D (struct target_mem_desc *) ptr; > - > - if (tgt->refcount > 1) > - tgt->refcount--; > - else > - gomp_unmap_tgt (tgt); > -} > - > -attribute_hidden void > -goacc_async_copyout_unmap_vars (struct target_mem_desc *tgt, > - struct goacc_asyncqueue *aq) > -{ > - struct gomp_device_descr *devicep =3D tgt->device_descr; > - > - /* Increment reference to delay freeing of device memory until callback > - has triggered. */ > - tgt->refcount++; > - gomp_unmap_vars_async (tgt, true, aq); > - devicep->openacc.async.queue_callback_func (aq, goacc_async_unmap_tgt, > - (void *) tgt); > -} > - > attribute_hidden void > goacc_async_free (struct gomp_device_descr *devicep, > struct goacc_asyncqueue *aq, void *ptr) > diff -ru trunk-orig/libgomp/oacc-int.h trunk-work/libgomp/oacc-int.h > --- trunk-orig/libgomp/oacc-int.h 2018-12-14 21:06:06.649794724 +0800 > +++ trunk-work/libgomp/oacc-int.h 2018-12-14 22:11:43.379947915 +0800 > @@ -104,8 +104,6 @@ >=20=20 > void goacc_init_asyncqueues (struct gomp_device_descr *); > bool goacc_fini_asyncqueues (struct gomp_device_descr *); > -void goacc_async_copyout_unmap_vars (struct target_mem_desc *, > - struct goacc_asyncqueue *); > void goacc_async_free (struct gomp_device_descr *, struct goacc_asyncque= ue *, > void *); > struct goacc_asyncqueue *get_goacc_asyncqueue (int); > diff -ru trunk-orig/libgomp/oacc-mem.c trunk-work/libgomp/oacc-mem.c > --- trunk-orig/libgomp/oacc-mem.c 2018-12-14 21:06:06.649794724 +0800 > +++ trunk-work/libgomp/oacc-mem.c 2018-12-14 22:10:08.325998369 +0800 > @@ -911,7 +911,7 @@ > else > { > goacc_aq aq =3D get_goacc_asyncqueue (async); > - goacc_async_copyout_unmap_vars (t, aq); > + gomp_unmap_vars_async (t, true, aq); > } > } >=20=20 > diff -ru trunk-orig/libgomp/oacc-parallel.c trunk-work/libgomp/oacc-paral= lel.c > --- trunk-orig/libgomp/oacc-parallel.c 2018-12-14 21:06:06.649794724 +0800 > +++ trunk-work/libgomp/oacc-parallel.c 2018-12-14 22:09:51.918353575 +0800 > @@ -245,7 +245,7 @@ > { > acc_dev->openacc.async.exec_func (tgt_fn, mapnum, hostaddrs, devad= drs, > dims, tgt, aq); > - goacc_async_copyout_unmap_vars (tgt, aq); > + gomp_unmap_vars_async (tgt, true, aq); > } > } >=20=20 > diff -ru trunk-orig/libgomp/target.c trunk-work/libgomp/target.c > --- trunk-orig/libgomp/target.c 2018-12-14 21:06:06.653794622 +0800 > +++ trunk-work/libgomp/target.c 2018-12-14 20:42:03.629154346 +0800 > @@ -1072,6 +1072,17 @@ > return is_tgt_unmapped; > } >=20=20 > +static void > +gomp_unref_tgt (void *ptr) > +{ > + struct target_mem_desc *tgt =3D (struct target_mem_desc *) ptr; > + > + if (tgt->refcount > 1) > + tgt->refcount--; > + else > + gomp_unmap_tgt (tgt); > +} > + > /* Unmap variables described by TGT. If DO_COPYFROM is true, copy relev= ant > variables back from device to host: if it is false, it is assumed tha= t this > has been done already. */ > @@ -1130,10 +1141,11 @@ > gomp_remove_var (devicep, k); > } >=20=20 > - if (tgt->refcount > 1) > - tgt->refcount--; > + if (aq) > + devicep->openacc.async.queue_callback_func (aq, gomp_unref_tgt, > + (void *) tgt); > else > - gomp_unmap_tgt (tgt); > + gomp_unref_tgt ((void *) tgt); >=20=20 > gomp_mutex_unlock (&devicep->lock); > }