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

[Ramana Radhakrishnan <ramana dot gcc at googlemail dot com>]

On Wed, Oct 29, 2014 at 2:27 PM, Will Newton <will.newton@linaro.org> wrote:
> 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.


Indeed - sorry for the noise.

Ramana
>
> --
> Will Newton
> Toolchain Working Group, Linaro