From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id 6C7483858D1E for ; Tue, 14 Feb 2023 15:11:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6C7483858D1E 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";a="96690250" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa3.mentor.iphmx.com with ESMTP; 14 Feb 2023 07:11:21 -0800 IronPort-SDR: PVOGd2bRmznAxPB6e3n3+tTpuKGzSQpsqgO1Y67gMymq4UGocC4uwCv/IAz1EyHoCec9fiCsao toljm2QNDTlhMC2hG8Ar8fI6coiXwC4jZB1Ve5Fl7266wqrvHSVVIV7PBoYWbZo+60FeyjQPaX Si8vaYD9iFpcdHl7FKAiA1iyNjszSu5JC1DGEocncU5JL7bqBo1luwKZkg/lB2glRqGRVhhUW1 yyt0VLkPb+Vx2yFPfEEhokjy0bZnGpnnht9NwHMyV/8LJdIc5qQuJ+KQQe0aGNllQd6wIGLaNc MLk= Message-ID: <82361586-1626-1243-337c-12509188ed9c@codesourcery.com> Date: Tue, 14 Feb 2023 15:11:14 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator) Content-Language: en-GB To: Thomas Schwinge , CC: Jakub Jelinek , Tobias Burnus , Tom de Vries 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> From: Andrew Stubbs In-Reply-To: <87h6voz9tl.fsf@euler.schwinge.homeip.net> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-15.mgc.mentorg.com (139.181.222.15) To svr-ies-mbx-11.mgc.mentorg.com (139.181.222.11) X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,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: On 14/02/2023 12:54, Thomas Schwinge wrote: > Hi Andrew! > > 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 = 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