[Mesa-dev] [PATCH 1/7] [RFC] util: Fix ralloc to use malloc instead of calloc

[Juha-Pekka Heikkila]

On 07.06.2016 20:26, Ian Romanick wrote:
> On 06/07/2016 07:26 AM, Juha-Pekka Heikkila wrote:
>> ralloc originally had had idea of using malloc but somehow
>> had slipped in calloc. Without these changes rzalloc did double
>> job of zeroing the memory, first calloc and then memset.
>> Now change ralloc to use malloc, leave rzalloc to use calloc and
>> make needed changes in ralloc functions to get things working.
>
> There are a lot of places where we expect zero-initialized memory.  How
> have you tested this?  Did you run before / after tests using valgrind?
> I'm especially nervous about changing the implementation of new.  In the
> past we intentionally removed zero initialization of fields from
> constructors because we new would do it for us, and maintaining long
> lists of foo=0 in the constructor is very error prone.

I made this set by switching ralloc to use malloc and then run piglit 
and go after all changed tests with valgrind. Then there was old 
glbenchmark with valgrind as a test of 'real' application. All this is 
just on IVB.

Comment about new operator I did not understand. There was zeroing 
allocator in new operator and my patch switches there allocator which 
again does zeroing for new operator. Without zeroing memory inside new 
there were lot of things broken.

>
> Have you been able to measure any performance benefit to this?
>

I have not done any attempt to measure performance differences with 
this. So far target was just to make ralloc work in way Kenneth probably 
originally meant it (guessing from the function names)

>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>>   src/util/ralloc.c | 31 +++++++++++++++++++++++--------
>>   src/util/ralloc.h |  2 +-
>>   2 files changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/util/ralloc.c b/src/util/ralloc.c
>> index 9526011..e27d835 100644
>> --- a/src/util/ralloc.c
>> +++ b/src/util/ralloc.c
>> @@ -104,19 +104,34 @@ add_child(ralloc_header *parent, ralloc_header *info)
>>   void *
>>   ralloc_context(const void *ctx)
>>   {
>> -   return ralloc_size(ctx, 0);
>> +   return rzalloc_size(ctx, 0);
>>   }
>>
>>   void *
>>   ralloc_size(const void *ctx, size_t size)
>>   {
>> -   /* ralloc_size was originally implemented using calloc, which meant some
>> -    * code accidentally relied on its zero filling behavior.
>> -    *
>> -    * TODO: Make ralloc_size not zero fill memory, and cleanup any code that
>> -    * should instead be using rzalloc.
>> -    */
>> -   return rzalloc_size(ctx, size);
>> +   void *block = malloc(size + sizeof(ralloc_header));
>
> This (and the similar code in rzalloc_size) should check for integer
> overflow.
>

ack, agree.

>> +   ralloc_header *info;
>> +   ralloc_header *parent;
>> +
>> +   if (unlikely(block == NULL))
>> +      return NULL;
>> +   info = (ralloc_header *) block;
>> +   parent = ctx != NULL ? get_header(ctx) : NULL;
>> +
>> +   info->child = NULL;
>> +   info->parent = NULL;
>> +   info->prev = NULL;
>> +   info->next = NULL;
>> +   info->destructor = NULL;
>> +
>> +   add_child(parent, info);
>> +
>> +#ifdef DEBUG
>> +   info->canary = CANARY;
>> +#endif
>> +
>> +   return PTR_FROM_HEADER(info);
>
> The common bits of this and rzalloc_size should be refactored into its
> own function...  static void *ralloc_header_init(const void *ctx, void
> *allocated_block) or something.
>

ack, agree.

>>   }
>>
>>   void *
>> diff --git a/src/util/ralloc.h b/src/util/ralloc.h
>> index 7587e11..462db7d 100644
>> --- a/src/util/ralloc.h
>> +++ b/src/util/ralloc.h
>> @@ -430,7 +430,7 @@ private:                                                                 \
>>   public:                                                                  \
>>      static void* operator new(size_t size, void *mem_ctx)                 \
>>      {                                                                     \
>> -      void *p = ralloc_size(mem_ctx, size);                              \
>> +      void *p = rzalloc_size(mem_ctx, size);                             \
>>         assert(p != NULL);                                                 \
>>         if (!HAS_TRIVIAL_DESTRUCTOR(TYPE))                                 \
>>            ralloc_set_destructor(p, _ralloc_destructor);                   \
>>
>