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

  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
  

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