[RFC] : Align large allocations to cacheline

  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?

ChangeLog:
2014-10-29  Wilco Dijkstra  <wdijkstr@arm.com>

	* malloc/malloc.c (__libc_malloc): Add support for aligning large blocks.
	* sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h: New file.
	New defines (MALLOC_LARGE_BLOCK_ALIGN), (MALLOC_LARGE_BLOCK_SIZE).
	* 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
  

> On Oct 29, 2014, at 6:43 AM, 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?
> 
> ChangeLog:
> 2014-10-29  Wilco Dijkstra  <wdijkstr@arm.com>
> 
>    * malloc/malloc.c (__libc_malloc): Add support for aligning large blocks.
>    * sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h: New file.
>    New defines (MALLOC_LARGE_BLOCK_ALIGN), (MALLOC_LARGE_BLOCK_SIZE).
>    * 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
> +
>   void *(*hook) (size_t, const void *)
>     = atomic_forced_read (__malloc_hook);
>   if (__builtin_expect (hook != NULL, 0))
> diff --git a/sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
> b/sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
> new file mode 100644
> index 0000000..3fe9f72
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
> @@ -0,0 +1,23 @@
> +/* System-specific malloc support functions.  AArch64 version.
> +   Copyright (C) 2012-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define MALLOC_LARGE_BLOCK_ALIGN (64)

Yes that is not the only cache line size on aarch64. Why don't you make it a global variable and read it from the system register like we do for memset?  ThunderX has a cache line size of 128. 

Thanks,
Andrew


> +#define MALLOC_LARGE_BLOCK_SIZE (16384)
> +
> +#include_next <malloc-sysdep.h>
> +
> diff --git a/sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
> b/sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
> new file mode 100644
> index 0000000..3fe9f72
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
> @@ -0,0 +1,23 @@
> +/* System-specific malloc support functions.  AArch64 version.
> +   Copyright (C) 2012-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define MALLOC_LARGE_BLOCK_ALIGN (64)
> +#define MALLOC_LARGE_BLOCK_SIZE (16384)
> +
> +#include_next <malloc-sysdep.h>
> +
> -- 
> 1.7.9.5
> 
>
  

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.

>    void *(*hook) (size_t, const void *)
>      = atomic_forced_read (__malloc_hook);
>    if (__builtin_expect (hook != NULL, 0))
> diff --git a/sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
> b/sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
> new file mode 100644
> index 0000000..3fe9f72
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aarch64/malloc-sysdep.h
> @@ -0,0 +1,23 @@
> +/* System-specific malloc support functions.  AArch64 version.
> +   Copyright (C) 2012-2014 Free Software Foundation, Inc.

These copyright lines are wrong, but the whole copy of the LGPL could
probably be dropped with the file being so simple.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define MALLOC_LARGE_BLOCK_ALIGN (64)
> +#define MALLOC_LARGE_BLOCK_SIZE (16384)
> +
> +#include_next <malloc-sysdep.h>
> +
> diff --git a/sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
> b/sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
> new file mode 100644
> index 0000000..3fe9f72
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/arm/malloc-sysdep.h
> @@ -0,0 +1,23 @@
> +/* System-specific malloc support functions.  AArch64 version.
> +   Copyright (C) 2012-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define MALLOC_LARGE_BLOCK_ALIGN (64)
> +#define MALLOC_LARGE_BLOCK_SIZE (16384)
> +
> +#include_next <malloc-sysdep.h>
> +
> --
> 1.7.9.5
>
>
  

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 ?


Ramana
  

> Andrew Pinski wrote:
>
> > +#define MALLOC_LARGE_BLOCK_ALIGN (64)
> 
> Yes that is not the only cache line size on aarch64. Why don't you make it a global variable
> and read it from the system register like we do for memset?  ThunderX has a cache line size of
> 128.

We could easily set it to 128 for AArch64 - the amount of wasted memory is 
minimal for large block sizes so overaligning doesn't hurt much.

However you raise an interesting point - several functions may want to know
the cacheline size and this would be useful across all targets. So we could
add generic infrastructure that sets the cacheline size at GLIBC startup
rather than having to add complex target specific code to compute it and
cache it on demand in every function that might query the line size.

Wilco
  

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 wrote:
> >> 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.

Right, it would be trivial to move the code so that the malloc hook 
is tried first. However if it isn't set then it will try the memalign
hook as well. I'm unclear what the idea of the hooks is, to hook the
original call only or to hook whatever low-level function you eventually
fall into or any combination... There is also an overhead in trying
several hooks rather than just one.

One possibility would be to split _mid_memalign, skip all the
unnecessary tests on alignment (which we know is correct) and directly go 
to the arena_get part, which is simpler and faster.

Wilco
  

On Wed, Oct 29, 2014 at 01:43:54PM -0000, Wilco Dijkstra 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?

It seems like this would pathologically increase fragmentation: when a
tiny block is split off the beginning to align a large allocation,
isn't it likely that this tiny chunk (rather than some other tiny
chunk already in the appropriate-sized free bin prior to the large
allocation) will be used to satisfy a tiny request, which may end up
being long-lived?

memalign-type operations are known to be a major problem for
fragmentation. See this bug report:

https://sourceware.org/bugzilla/show_bug.cgi?id=14581

A smarter approach might be to round up the _sizes_ of large
allocations so that the next available address after the allocated
block is nicely aligned. This will tend to make _future_ allocations
aligned, even if the first one isn't, and won't leave any free gaps
that could contribute to fragmentation.

Rich
  

On Wed, Oct 29, 2014 at 9:25 AM, Wilco Dijkstra <wdijkstr@arm.com> wrote:
>
> > Andrew Pinski wrote:
> >
> > > +#define MALLOC_LARGE_BLOCK_ALIGN (64)
> >
> > Yes that is not the only cache line size on aarch64. Why don't you make it a global variable
> > and read it from the system register like we do for memset?  ThunderX has a cache line size of
> > 128.
>
> We could easily set it to 128 for AArch64 - the amount of wasted memory is
> minimal for large block sizes so overaligning doesn't hurt much.
>
> However you raise an interesting point - several functions may want to know
> the cacheline size and this would be useful across all targets. So we could
> add generic infrastructure that sets the cacheline size at GLIBC startup
> rather than having to add complex target specific code to compute it and
> cache it on demand in every function that might query the line size.


This has already done for powerpc since the A2 processor can use
different cache-line sizes, so look there for precedent.

grep for __cache_line_size.
  

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
  

> Rich Felker wrote: 
> On Wed, Oct 29, 2014 at 01:43:54PM -0000, Wilco Dijkstra 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?
> 
> It seems like this would pathologically increase fragmentation: when a
> tiny block is split off the beginning to align a large allocation,
> isn't it likely that this tiny chunk (rather than some other tiny
> chunk already in the appropriate-sized free bin prior to the large
> allocation) will be used to satisfy a tiny request, which may end up
> being long-lived?

A good implementation of memalign would try to avoid that, for example
by increasing the previous allocated block or not adding tiny splitoff
chunks to the free list.

> memalign-type operations are known to be a major problem for
> fragmentation. See this bug report:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=14581

It seems that report is claiming that freeing a block doesn't immediately
merge with adjacent freed blocks. If that is the case then that is a very
serious bug indeed! The fact that memalign is not fully integrated into 
the allocation code doesn't help either - it never tries to reuse an
correctly aligned free block.

> A smarter approach might be to round up the _sizes_ of large
> allocations so that the next available address after the allocated
> block is nicely aligned. This will tend to make _future_ allocations
> aligned, even if the first one isn't, and won't leave any free gaps
> that could contribute to fragmentation.

That might be possible too, I'd have to give it a go. I was trying to avoid
reworking the guts of the malloc code - I think it would be better to create
a new modern allocator from scratch.

Wilco
  

On Wed, Oct 29, 2014 at 03:31:37PM -0000, Wilco Dijkstra wrote:
> > Rich Felker wrote: 
> > On Wed, Oct 29, 2014 at 01:43:54PM -0000, Wilco Dijkstra 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?
> > 
> > It seems like this would pathologically increase fragmentation: when a
> > tiny block is split off the beginning to align a large allocation,
> > isn't it likely that this tiny chunk (rather than some other tiny
> > chunk already in the appropriate-sized free bin prior to the large
> > allocation) will be used to satisfy a tiny request, which may end up
> > being long-lived?
> 
> A good implementation of memalign would try to avoid that, for example
> by increasing the previous allocated block or not adding tiny splitoff
> chunks to the free list.

I don't think modifying the previous allocated block is practical;
this almost certainly has data races unless you impose costly
synchronization.

If you do the split but don't add the split block to the free list, I
don't see how you'd track it to free it later.

You could refrain from splitting the tiny block at all and instead
keep it as part of the the allocated block with a special flag bit on
the header to indicate that the allocator needs to look back to find
the actual beginning of the block when freeing or reallocing it, etc.
This is essentially a more complex, but slightly more optimal, version
of my idea for rounding up the size.

> > memalign-type operations are known to be a major problem for
> > fragmentation. See this bug report:
> > 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=14581
> 
> It seems that report is claiming that freeing a block doesn't immediately
> merge with adjacent freed blocks. If that is the case then that is a very
> serious bug indeed! The fact that memalign is not fully integrated into 
> the allocation code doesn't help either - it never tries to reuse an
> correctly aligned free block.

Yes, there are probably improvements that could be made to memalign to
reduce the problem, but they have tradeoffs. I don't think it's
practical to make memalign reuse correctly aligned free blocks unless
you segregate the free lists not just by size but also by alignment.
Otherwise you'd have to do a non-constant-time search to determine
whether a free block of the desired alignment exists and select it.

> > A smarter approach might be to round up the _sizes_ of large
> > allocations so that the next available address after the allocated
> > block is nicely aligned. This will tend to make _future_ allocations
> > aligned, even if the first one isn't, and won't leave any free gaps
> > that could contribute to fragmentation.
> 
> That might be possible too, I'd have to give it a go. I was trying to avoid
> reworking the guts of the malloc code - I think it would be better to create
> a new modern allocator from scratch.

That's quite possible. All the basic algorithms involved are
understood and well documented, so it's certainly practical to redo
without all the cruft. However there is quite a big testing burden.
Even if you know what you're doing, it's very easy to make stupid
mistakes.

Rich
  

On 10/29/2014 09:43 AM, Wilco Dijkstra wrote:
> +#define MALLOC_LARGE_BLOCK_ALIGN (64)
> +#define MALLOC_LARGE_BLOCK_SIZE (16384)

Typo-prone interface. It will generate -Wundef errors.

See: https://sourceware.org/glibc/wiki/Wundef

It will become an error soon to leave these undefined
for other arches.

Cheers,
Carlos.