From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id 6FB283858C78 for ; Thu, 16 Feb 2023 21:45:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6FB283858C78 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,302,1669104000"; d="scan'208,223";a="97280739" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa4.mentor.iphmx.com with ESMTP; 16 Feb 2023 13:45:12 -0800 IronPort-SDR: jbx4umrjeB6ItEyP3pB505nhChICLfMlmJtJcVAKp/+W2l4hCPsLHtj7rscK/8Ho4Pni4hGOdw U8atlo6/P1ak6kjlMr8BCuCN8r/EmoTJDV7ppHDpZk9Wb9sgk7intUzTbxFiCH7ovRE1RrbQYz fk0GH6WddOKRKcgSCfTE3zDCVfrF3lC0qDjVcXB+EG2JxqRpm7TtY5NkVeD1bC7/wsSRLueD/4 dfHvTSa0pZP7BLateb9Xkgq8aJEsevmzDjgcS6mSvbsnB7qNaIBO1xRTL8sHmmvX8dez0r9Ta2 SAI= From: Thomas Schwinge To: Andrew Stubbs , CC: Jakub Jelinek , Tobias Burnus , Tom de Vries Subject: Re: [og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator) In-Reply-To: <82361586-1626-1243-337c-12509188ed9c@codesourcery.com> References: <25ad524d-f0d6-1970-b8e9-9b11b6cde68b@codesourcery.com> <42c70624-2b10-340c-8945-601203768d48@suse.de> <664653d3-cf64-b800-6ffb-c27e50dc15bf@suse.de> <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> <87h6voz9tl.fsf@euler.schwinge.homeip.net> <82361586-1626-1243-337c-12509188ed9c@codesourcery.com> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/28.2 (x86_64-pc-linux-gnu) Date: Thu, 16 Feb 2023 22:45:04 +0100 Message-ID: <87r0upxp27.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-12.mgc.mentorg.com (139.181.222.12) 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 List-Id: --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi! On 2023-02-14T15:11:14+0000, Andrew Stubbs wrote: > On 14/02/2023 12:54, Thomas Schwinge wrote: >> On 2022-01-13T11:13:51+0000, Andrew Stubbs 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 =3D realloc (data->ptr, new_size); >>> + new_ptr =3D MEMSPACE_REALLOC (allocator_data->memspace, data->ptr= , >>> + data->size, new_size); >>> else >>> - new_ptr =3D malloc (new_size); >>> + new_ptr =3D MEMSPACE_ALLOC (allocator_data->memspace, new_size); >>> if (new_ptr =3D=3D NULL) >>> { >>> #ifdef HAVE_SYNC_BUILTINS >>> @@ -690,7 +740,11 @@ retry: >>> && (free_allocator_data =3D=3D NULL >>> || free_allocator_data->pool_size =3D=3D ~(uintptr_t) 0)) >>> { >>> - new_ptr =3D realloc (data->ptr, new_size); >>> + omp_memspace_handle_t memspace __attribute__((unused)) >>> + =3D (allocator_data >>> + ? allocator_data->memspace >>> + : predefined_alloc_mapping[allocator]); >>> + new_ptr =3D MEMSPACE_REALLOC (memspace, data->ptr, data->size, n= ew_size); >>> if (new_ptr =3D=3D NULL) >>> goto fail; >>> ret =3D (char *) new_ptr + sizeof (struct omp_mem_header); >>> @@ -701,7 +755,11 @@ retry: >>> } >>> else >>> { >>> - new_ptr =3D malloc (new_size); >>> + omp_memspace_handle_t memspace __attribute__((unused)) >>> + =3D (allocator_data >>> + ? allocator_data->memspace >>> + : predefined_alloc_mapping[allocator]); >>> + new_ptr =3D MEMSPACE_ALLOC (memspace, new_size); >>> if (new_ptr =3D=3D 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_FR= EE'" >> 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 =3D 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, -d= ata->size, >> - MEMMODEL_RELAXED); >> -#else >> - gomp_mutex_lock (&free_allocator_data->lock); >> - free_allocator_data->used_pool_size -=3D 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=C3=BC=C3=9Fe Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-In-libgomp-allocator.c-omp_realloc-route-free-throug.patch" >From 3a2c07395b0a565955a7b86f0eba866937e15989 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge 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/ChangeLog.omp | 2 ++ libgomp/allocator.c | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libgomp/ChangeLog.omp b/libgomp/ChangeLog.omp index 530f5c6acf6..819a5333907 100644 --- a/libgomp/ChangeLog.omp +++ b/libgomp/ChangeLog.omp @@ -1,5 +1,7 @@ 2023-02-16 Thomas Schwinge + * allocator.c (omp_realloc): Route 'free' through 'MEMSPACE_FREE'. + * config/linux/allocator.c (linux_memspace_alloc) (linux_memspace_calloc): Clarify zero-initialization for pinned memory. diff --git a/libgomp/allocator.c b/libgomp/allocator.c index 05b323d458e..ba9a4e17cc2 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.25.1 --=-=-=--