[og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)
Message ID | 87h6voz9tl.fsf@euler.schwinge.homeip.net |
---|---|
State | Deferred |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 52ED23858C5E for <patchwork@sourceware.org>; Tue, 14 Feb 2023 12:54:58 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 38EB53858D1E for <gcc-patches@gcc.gnu.org>; Tue, 14 Feb 2023 12:54:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 38EB53858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.97,296,1669104000"; d="scan'208,223";a="100705477" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 14 Feb 2023 04:54:40 -0800 IronPort-SDR: fhDHZgCA3cQlSr743XV1Q8G5yuNF46MQeJragT84kAHJB6skYCMKu0XqlF8IjTeoDJcHcod659 8nqom898V9YvEzu8azqUm24khOM4tps/TRNR2aRlFKOWxG11i/7oAsbcA8Tos3PZEgQNh83wvD k0L8JUrjGAlZsDOh0F9FDueA62NPGnn5wFXtKneA3RMFmp5HVAM1zTYn4+xVQJXcotXf/1KFVO M9O4GzJt1j467vI8AccpLfJSzU1FV0o0UvNmQcVcYeVAa4PadHxzB8xTkVijqN3jsRJGOR9/UL 2Mc= From: Thomas Schwinge <thomas@codesourcery.com> To: Andrew Stubbs <ams@codesourcery.com>, <gcc-patches@gcc.gnu.org> CC: Jakub Jelinek <jakub@redhat.com>, Tobias Burnus <tobias@codesourcery.com>, Tom de Vries <tdevries@suse.de> Subject: [og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator) In-Reply-To: <93ae0df7-6cb9-40de-81e6-768029ca49fc@codesourcery.com> References: <25ad524d-f0d6-1970-b8e9-9b11b6cde68b@codesourcery.com> <42c70624-2b10-340c-8945-601203768d48@suse.de> <664653d3-cf64-b800-6ffb-c27e50dc15bf@suse.de> <bfe2c7c8-6374-91ea-2587-805c57497b14@codesourcery.com> <5e75a64c-a8d3-2d2a-162a-a3ea79358b48@suse.de> <3e39c39d-2753-f08c-5fb7-85051cafab85@suse.de> <985755ee-f7bd-7d47-1ea0-1ca980117f6d@codesourcery.com> <93ae0df7-6cb9-40de-81e6-768029ca49fc@codesourcery.com> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/28.2 (x86_64-pc-linux-gnu) Date: Tue, 14 Feb 2023 13:54:30 +0100 Message-ID: <87h6voz9tl.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-11.mgc.mentorg.com (139.181.222.11) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)
|
|
Commit Message
Thomas Schwinge
Feb. 14, 2023, 12:54 p.m. UTC
Hi Andrew! On 2022-01-13T11:13:51+0000, Andrew Stubbs <ams@codesourcery.com> wrote: > Updated patch: this version fixes some missed cases of malloc in the > realloc implementation. Right, and as it seems I've run into another issue: a stray 'free'. > --- a/libgomp/allocator.c > +++ b/libgomp/allocator.c Re 'omp_realloc': > @@ -660,9 +709,10 @@ retry: > gomp_mutex_unlock (&allocator_data->lock); > #endif > if (prev_size) > - new_ptr = realloc (data->ptr, new_size); > + new_ptr = MEMSPACE_REALLOC (allocator_data->memspace, data->ptr, > + data->size, new_size); > else > - new_ptr = malloc (new_size); > + new_ptr = MEMSPACE_ALLOC (allocator_data->memspace, new_size); > if (new_ptr == NULL) > { > #ifdef HAVE_SYNC_BUILTINS > @@ -690,7 +740,11 @@ retry: > && (free_allocator_data == NULL > || free_allocator_data->pool_size == ~(uintptr_t) 0)) > { > - new_ptr = realloc (data->ptr, new_size); > + omp_memspace_handle_t memspace __attribute__((unused)) > + = (allocator_data > + ? allocator_data->memspace > + : predefined_alloc_mapping[allocator]); > + new_ptr = MEMSPACE_REALLOC (memspace, data->ptr, data->size, new_size); > if (new_ptr == NULL) > goto fail; > ret = (char *) new_ptr + sizeof (struct omp_mem_header); > @@ -701,7 +755,11 @@ retry: > } > else > { > - new_ptr = malloc (new_size); > + omp_memspace_handle_t memspace __attribute__((unused)) > + = (allocator_data > + ? allocator_data->memspace > + : predefined_alloc_mapping[allocator]); > + new_ptr = MEMSPACE_ALLOC (memspace, new_size); > if (new_ptr == NULL) > goto fail; > } > @@ -735,32 +793,35 @@ retry: | free (data->ptr); > return ret; I run into a SIGSEGV if a non-'malloc'-based allocation is 'free'd here. The attached "In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE'" appears to resolve my issue, but not yet regression-tested. Does that look correct to you? Or, instead of invoking 'MEMSPACE_FREE', should we scrap the 'used_pool_size' bookkeeping here, and just invoke 'omp_free' instead? --- libgomp/allocator.c +++ libgomp/allocator.c @@ -842,19 +842,7 @@ retry: if (old_size - old_alignment < size) size = old_size - old_alignment; memcpy (ret, ptr, size); - if (__builtin_expect (free_allocator_data - && free_allocator_data->pool_size < ~(uintptr_t) 0, 0)) - { -#ifdef HAVE_SYNC_BUILTINS - __atomic_add_fetch (&free_allocator_data->used_pool_size, -data->size, - MEMMODEL_RELAXED); -#else - gomp_mutex_lock (&free_allocator_data->lock); - free_allocator_data->used_pool_size -= data->size; - gomp_mutex_unlock (&free_allocator_data->lock); -#endif - } - free (data->ptr); + ialias_call (omp_free) (ptr, free_allocator); return ret; (I've not yet analyzed whether that's completely equivalent.) Note that this likewise applies to the current upstream submission: <https://inbox.sourceware.org/gcc-patches/400092d8ce44340cece0e2e38f88edbad6400b03.1657188329.git.ams@codesourcery.com> "libgomp, nvptx: low-latency memory allocator". Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Comments
On 14/02/2023 12:54, Thomas Schwinge wrote: > Hi Andrew! > > On 2022-01-13T11:13:51+0000, Andrew Stubbs <ams@codesourcery.com> wrote: >> Updated patch: this version fixes some missed cases of malloc in the >> realloc implementation. > > Right, and as it seems I've run into another issue: a stray 'free'. > >> --- a/libgomp/allocator.c >> +++ b/libgomp/allocator.c > > Re 'omp_realloc': > >> @@ -660,9 +709,10 @@ retry: >> gomp_mutex_unlock (&allocator_data->lock); >> #endif >> if (prev_size) >> - new_ptr = realloc (data->ptr, new_size); >> + new_ptr = MEMSPACE_REALLOC (allocator_data->memspace, data->ptr, >> + data->size, new_size); >> else >> - new_ptr = malloc (new_size); >> + new_ptr = MEMSPACE_ALLOC (allocator_data->memspace, new_size); >> if (new_ptr == NULL) >> { >> #ifdef HAVE_SYNC_BUILTINS >> @@ -690,7 +740,11 @@ retry: >> && (free_allocator_data == NULL >> || free_allocator_data->pool_size == ~(uintptr_t) 0)) >> { >> - new_ptr = realloc (data->ptr, new_size); >> + omp_memspace_handle_t memspace __attribute__((unused)) >> + = (allocator_data >> + ? allocator_data->memspace >> + : predefined_alloc_mapping[allocator]); >> + new_ptr = MEMSPACE_REALLOC (memspace, data->ptr, data->size, new_size); >> if (new_ptr == NULL) >> goto fail; >> ret = (char *) new_ptr + sizeof (struct omp_mem_header); >> @@ -701,7 +755,11 @@ retry: >> } >> else >> { >> - new_ptr = malloc (new_size); >> + omp_memspace_handle_t memspace __attribute__((unused)) >> + = (allocator_data >> + ? allocator_data->memspace >> + : predefined_alloc_mapping[allocator]); >> + new_ptr = MEMSPACE_ALLOC (memspace, new_size); >> if (new_ptr == NULL) >> goto fail; >> } >> @@ -735,32 +793,35 @@ retry: > | free (data->ptr); >> return ret; > > I run into a SIGSEGV if a non-'malloc'-based allocation is 'free'd here. > > The attached > "In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE'" > appears to resolve my issue, but not yet regression-tested. Does that > look correct to you? That looks correct. The only remaining use of "free" should be the one referring to the allocator object itself (i.e. the destructor). > Or, instead of invoking 'MEMSPACE_FREE', should we scrap the > 'used_pool_size' bookkeeping here, and just invoke 'omp_free' instead? > > --- libgomp/allocator.c > +++ libgomp/allocator.c > @@ -842,19 +842,7 @@ retry: > if (old_size - old_alignment < size) > size = old_size - old_alignment; > memcpy (ret, ptr, size); > - if (__builtin_expect (free_allocator_data > - && free_allocator_data->pool_size < ~(uintptr_t) 0, 0)) > - { > -#ifdef HAVE_SYNC_BUILTINS > - __atomic_add_fetch (&free_allocator_data->used_pool_size, -data->size, > - MEMMODEL_RELAXED); > -#else > - gomp_mutex_lock (&free_allocator_data->lock); > - free_allocator_data->used_pool_size -= data->size; > - gomp_mutex_unlock (&free_allocator_data->lock); > -#endif > - } > - free (data->ptr); > + ialias_call (omp_free) (ptr, free_allocator); > return ret; > > (I've not yet analyzed whether that's completely equivalent.) The used_pool_size code comes from upstream, so if you want to go beyond the mechanical substitution of "free" then you're adding a new patch (rather than tweaking an old one). I'll leave that for others to comment on. Andrew
Hi! On 2023-02-14T15:11:14+0000, Andrew Stubbs <ams@codesourcery.com> wrote: > On 14/02/2023 12:54, Thomas Schwinge wrote: >> On 2022-01-13T11:13:51+0000, Andrew Stubbs <ams@codesourcery.com> wrote: >>> Updated patch: this version fixes some missed cases of malloc in the >>> realloc implementation. >> >> Right, and as it seems I've run into another issue: a stray 'free'. >> >>> --- a/libgomp/allocator.c >>> +++ b/libgomp/allocator.c >> >> Re 'omp_realloc': >> >>> @@ -660,9 +709,10 @@ retry: >>> gomp_mutex_unlock (&allocator_data->lock); >>> #endif >>> if (prev_size) >>> - new_ptr = realloc (data->ptr, new_size); >>> + new_ptr = MEMSPACE_REALLOC (allocator_data->memspace, data->ptr, >>> + data->size, new_size); >>> else >>> - new_ptr = malloc (new_size); >>> + new_ptr = MEMSPACE_ALLOC (allocator_data->memspace, new_size); >>> if (new_ptr == NULL) >>> { >>> #ifdef HAVE_SYNC_BUILTINS >>> @@ -690,7 +740,11 @@ retry: >>> && (free_allocator_data == NULL >>> || free_allocator_data->pool_size == ~(uintptr_t) 0)) >>> { >>> - new_ptr = realloc (data->ptr, new_size); >>> + omp_memspace_handle_t memspace __attribute__((unused)) >>> + = (allocator_data >>> + ? allocator_data->memspace >>> + : predefined_alloc_mapping[allocator]); >>> + new_ptr = MEMSPACE_REALLOC (memspace, data->ptr, data->size, new_size); >>> if (new_ptr == NULL) >>> goto fail; >>> ret = (char *) new_ptr + sizeof (struct omp_mem_header); >>> @@ -701,7 +755,11 @@ retry: >>> } >>> else >>> { >>> - new_ptr = malloc (new_size); >>> + omp_memspace_handle_t memspace __attribute__((unused)) >>> + = (allocator_data >>> + ? allocator_data->memspace >>> + : predefined_alloc_mapping[allocator]); >>> + new_ptr = MEMSPACE_ALLOC (memspace, new_size); >>> if (new_ptr == NULL) >>> goto fail; >>> } >>> @@ -735,32 +793,35 @@ retry: >> | free (data->ptr); >>> return ret; >> >> I run into a SIGSEGV if a non-'malloc'-based allocation is 'free'd here. >> >> The attached >> "In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE'" >> appears to resolve my issue, but not yet regression-tested. No issues in testing. >> Does that >> look correct to you? > > That looks correct. Thanks. I've pushed to devel/omp/gcc-12 branch commit 3a2c07395b0a565955a7b86f0eba866937e15989 "In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE'", see attached. > The only remaining use of "free" should be the one > referring to the allocator object itself (i.e. the destructor). ACK. >> Or, instead of invoking 'MEMSPACE_FREE', should we scrap the >> 'used_pool_size' bookkeeping here, and just invoke 'omp_free' instead? >> >> --- libgomp/allocator.c >> +++ libgomp/allocator.c >> @@ -842,19 +842,7 @@ retry: >> if (old_size - old_alignment < size) >> size = old_size - old_alignment; >> memcpy (ret, ptr, size); >> - if (__builtin_expect (free_allocator_data >> - && free_allocator_data->pool_size < ~(uintptr_t) 0, 0)) >> - { >> -#ifdef HAVE_SYNC_BUILTINS >> - __atomic_add_fetch (&free_allocator_data->used_pool_size, -data->size, >> - MEMMODEL_RELAXED); >> -#else >> - gomp_mutex_lock (&free_allocator_data->lock); >> - free_allocator_data->used_pool_size -= data->size; >> - gomp_mutex_unlock (&free_allocator_data->lock); >> -#endif >> - } >> - free (data->ptr); >> + ialias_call (omp_free) (ptr, free_allocator); >> return ret; >> >> (I've not yet analyzed whether that's completely equivalent.) > > The used_pool_size code comes from upstream, so if you want to go beyond > the mechanical substitution of "free" then you're adding a new patch > (rather than tweaking an old one). I'll leave that for others to comment on. And I'll leave that for another day, and/or another person. ;-) Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
From d49d0b9dc4f96c496afb2d5caac4addb382fdf39 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <thomas@codesourcery.com> Date: Tue, 14 Feb 2023 13:35:03 +0100 Subject: [PATCH] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' ... to not run into a SIGSEGV if a non-'malloc'-based allocation is 'free'd here. Fix-up for og12 commit c5d1d7651297a273321154a5fe1b01eba9dcf604 "libgomp, nvptx: low-latency memory allocator". libgomp/ * allocator.c (omp_realloc): Route 'free' through 'MEMSPACE_FREE'. --- libgomp/allocator.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/libgomp/allocator.c b/libgomp/allocator.c index 05b323d458e2..ba9a4e17cc20 100644 --- a/libgomp/allocator.c +++ b/libgomp/allocator.c @@ -854,7 +854,17 @@ retry: gomp_mutex_unlock (&free_allocator_data->lock); #endif } - free (data->ptr); + { + omp_memspace_handle_t was_memspace __attribute__((unused)) + = (free_allocator_data + ? free_allocator_data->memspace + : predefined_alloc_mapping[free_allocator]); + int was_pinned __attribute__((unused)) + = (free_allocator_data + ? free_allocator_data->pinned + : free_allocator == ompx_pinned_mem_alloc); + MEMSPACE_FREE (was_memspace, data->ptr, data->size, was_pinned); + } return ret; fail: -- 2.39.1