Re: [RFC PATCH]: Align large allocations to cacheline

[Will Newton <will dot newton at linaro dot org>]

On 29 October 2014 14:16, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
> On Wed, Oct 29, 2014 at 2:05 PM, Will Newton <will.newton@linaro.org> wrote:
>> On 29 October 2014 13:43, Wilco Dijkstra <wdijkstr@arm.com> wrote:
>>> This patch aligns allocations of large blocks to a cacheline on ARM and AArch64. The main goal is to
>>> reduce performance variations due to random alignment choices, however it improves performance on
>>> several benchmarks as well. SPECFP2000 improves by ~1.5%.
>>>
>>> Any comments?
>>
>> This bug may be relevant to changing malloc alignments:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=6527
>>
>> I don't know precisely what the issue is with malloc_get_state so I am
>> not sure if it applies here.
>>
>>> ChangeLog:
>>> 2014-10-29  Wilco Dijkstra  <wdijkstr@arm.com>
>>>
>>>         * malloc/malloc.c (__libc_malloc): Add support for aligning large blocks.
>>
>> I think this should be:
>>
>> malloc/malloc.c (__libc_malloc) [MALLOC_LARGE_BLOCK_ALIGN]: Add
>> support for aligning large blocks.
>>
>> Although I am not a GNU ChangeLog expert...
>>
>>>         * sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h: New file.
>>>         New defines (MALLOC_LARGE_BLOCK_ALIGN), (MALLOC_LARGE_BLOCK_SIZE).
>>
>> "New file." should be sufficient.
>>
>>>         * sysdeps/unix/sysv/linux/arm/malloc-sysdep.h: Likewise.
>>>
>>> ---
>>>  malloc/malloc.c                                 |    8 ++++++++
>>>  sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h |   23 +++++++++++++++++++++++
>>>  sysdeps/unix/sysv/linux/arm/malloc-sysdep.h     |   23 +++++++++++++++++++++++
>>>  3 files changed, 54 insertions(+)
>>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
>>>  create mode 100644 sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
>>>
>>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>>> index 6cbe9f3..0b0466e 100644
>>> --- a/malloc/malloc.c
>>> +++ b/malloc/malloc.c
>>> @@ -2878,6 +2878,14 @@ __libc_malloc (size_t bytes)
>>>    mstate ar_ptr;
>>>    void *victim;
>>>
>>> +#ifdef MALLOC_LARGE_BLOCK_ALIGN
>>> +  if (bytes > MALLOC_LARGE_BLOCK_SIZE)
>>> +    {
>>> +      void *address = RETURN_ADDRESS (0);
>>> +      return _mid_memalign (MALLOC_LARGE_BLOCK_ALIGN, bytes, address);
>>> +    }
>>> +#endif
>>> +
>>
>> This will change the behaviour of malloc hooks i.e. the malloc hook
>> will not get triggered but memalign will.
>
> Yeah that was my first reaction too - but read _mid_memalign - that
> appears to call the malloc hooks too ?

But _mid_memalign calls the memalign hook rather than the malloc hook
which is rather surprising given we called malloc. It should simply be
a case of moving the #ifdef block to below the call of the malloc hook
in __libc_malloc to retain the existing behaviour.

-- 
Will Newton
Toolchain Working Group, Linaro