Re: [og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)

2023-02-16 Thread Thomas Schwinge 
Hi!

On 2023-02-14T15:11:14+, Andrew Stubbs  wrote:
> On 14/02/2023 12:54, Thomas Schwinge wrote:
>> On 2022-01-13T11:13:51+, 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.

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 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

Re: [og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)

2023-02-14 Thread Andrew Stubbs 

On 14/02/2023 12:54, Thomas Schwinge wrote:

Hi Andrew!

On 2022-01-13T11:13:51+, 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

[og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)

2023-02-14 Thread Thomas Schwinge 
Hi Andrew!

On 2022-01-13T11:13:51+, 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?

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:

"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
>From d49d0b9dc4f96c496afb2d5caac4addb382fdf39 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/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