[og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)
Checks
Commit Message
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(-)
@@ -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