Re: [PATCH] malloc.c, arena.c: Use ALIGN_UP, powerof2, and pagesize consistently.

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Tue, Dec 30, 2014 at 02:20:58PM -0500, Carlos O'Donell wrote:
> I was auditing malloc.c and arena.c for reetrancy issues and
> decided to do a cleanup as I read the code.
> 
> The malloc implementation has instance of the ALIGN_UP pattern
> that would be made clearer by using the so-named libc-internal.h
> macro.
> 
> Even more interesting is that the micro-optimization of
> precomputing `pagesize - 1` in an attempt to make subsequent
> ALIGN_UP operations fast results in larger code than just simply
> using the ALIGN_UP macro with the pagesize variable (gcc 4.8).
> 
> e.g.
> ---
> <new_heap>:
> ...
> +       48 01 fe                add    %rdi,%rsi
> +       48 81 fe ff 7f 00 00    cmp    $0x7fff,%rsi
>         55                      push   %rbp
> -                               lea    (%rdi,%rsi,1),%rbp
>         48 8b 40 18             mov    0x?(%rax),%rax
>         53                      push   %rbx
> -       48 83 e8 01             sub    $0x1,%rax
> -       48 81 fd ff 7f 00 00    cmp    $0x7fff,%rbp
> ...
>         48 8b 3d 00 00 00 00    mov    0x?(%rip),%rdi        #? <new_heap+0x?>
> 
> -       48 01 c5                add    %rax,%rbp
> -       48 f7 d0                not    %rax
> +                               lea    -0x?(%rsi,%rax,1),%rbp
> +       48 f7 d8                neg    %rax
>         48 21 c5                and    %rax,%rbp
>         48 85 ff                test   %rdi,%rdi
>                                 je     ? <new_heap+0x?>
> ...
>         5d                      pop    %rbp
>         41 5c                   pop    %r12
>         c3                      retq
> -       0f 1f 44 00 00          nopl   0x?(%rax,%rax,1)
> ...
> -       66 66 66 2e 0f 1f 84    data32 data32 nopw %cs:0x?(%rax,%rax,1)
> -       00 00 00 00 00 
> +       0f 1f 40 00             nopl   0x?(%rax)
> ---
> 
> In addition we cleanup the use of page_mask, pagemask, pagesz, and
> pagesize and settle on a readable and common variable "pagesize."
> In only one instance do we leave page_mask since it makes sense for
> that particular case (hooks.c:mem2chunk_check).
> 
> Lastly we use powerof2 where appropriate.
> 
> Tested on x86_64. Verified no regressions.
> 
> OK to checkin?

Looks OK to me.

Siddhesh

> 
> 2014-12-30  Carlos O'Donell  <carlos@redhat.com>
> 
> 	* arena.c (new_heap): Use pagesize. Call ALIGN_UP.
> 	(grow_heap): Likewise.
> 	* malloc.c: Include libc-internal.h.
> 	(do_check_malloc): Call powerof2.
> 	(sysmalloc): Use pagesize. Call ALIGN_UP.
> 	(systrim): Use pagesize.
> 	(mremap_chunk): Use pagesize. Call ALIGN_UP.
> 	(__libc_valloc): Use pagesize.
> 	(__libc_pvalloc): Use pagesize. Call ALIGN_UP.	
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 60ae9a4..f0879f2 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -510,7 +510,7 @@ static heap_info *
>  internal_function
>  new_heap (size_t size, size_t top_pad)
>  {
> -  size_t page_mask = GLRO (dl_pagesize) - 1;
> +  size_t pagesize = GLRO (dl_pagesize);
>    char *p1, *p2;
>    unsigned long ul;
>    heap_info *h;
> @@ -523,7 +523,7 @@ new_heap (size_t size, size_t top_pad)
>      return 0;
>    else
>      size = HEAP_MAX_SIZE;
> -  size = (size + page_mask) & ~page_mask;
> +  size = ALIGN_UP (size, pagesize);
>  
>    /* A memory region aligned to a multiple of HEAP_MAX_SIZE is needed.
>       No swap space needs to be reserved for the following large
> @@ -588,10 +588,10 @@ new_heap (size_t size, size_t top_pad)
>  static int
>  grow_heap (heap_info *h, long diff)
>  {
> -  size_t page_mask = GLRO (dl_pagesize) - 1;
> +  size_t pagesize = GLRO (dl_pagesize);
>    long new_size;
>  
> -  diff = (diff + page_mask) & ~page_mask;
> +  diff = ALIGN_UP (diff, pagesize);
>    new_size = (long) h->size + diff;
>    if ((unsigned long) new_size > (unsigned long) HEAP_MAX_SIZE)
>      return -1;
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 875fe2e..bc8f5da 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -241,6 +241,9 @@
>  /* For MIN, MAX, powerof2.  */
>  #include <sys/param.h>
>  
> +/* For ALIGN_UP.  */
> +#include <libc-internal.h>
> +
>  
>  /*
>    Debugging:
> @@ -2111,7 +2114,7 @@ do_check_malloc_state (mstate av)
>      return;
>  
>    /* pagesize is a power of 2 */
> -  assert ((GLRO (dl_pagesize) & (GLRO (dl_pagesize) - 1)) == 0);
> +  assert (powerof2(GLRO (dl_pagesize)));
>  
>    /* A contiguous main_arena is consistent with sbrk_base.  */
>    if (av == &main_arena && contiguous (av))
> @@ -2266,7 +2269,7 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>    unsigned long remainder_size;   /* its size */
>  
>  
> -  size_t pagemask = GLRO (dl_pagesize) - 1;
> +  size_t pagesize = GLRO (dl_pagesize);
>    bool tried_mmap = false;
>  
>  
> @@ -2292,9 +2295,9 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>           need for further alignments unless we have have high alignment.
>         */
>        if (MALLOC_ALIGNMENT == 2 * SIZE_SZ)
> -        size = (nb + SIZE_SZ + pagemask) & ~pagemask;
> +        size = ALIGN_UP (nb + SIZE_SZ, pagesize);
>        else
> -        size = (nb + SIZE_SZ + MALLOC_ALIGN_MASK + pagemask) & ~pagemask;
> +        size = ALIGN_UP (nb + SIZE_SZ + MALLOC_ALIGN_MASK, pagesize);
>        tried_mmap = true;
>  
>        /* Don't try if size wraps around 0 */
> @@ -2367,7 +2370,7 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>    assert ((old_top == initial_top (av) && old_size == 0) ||
>            ((unsigned long) (old_size) >= MINSIZE &&
>             prev_inuse (old_top) &&
> -           ((unsigned long) old_end & pagemask) == 0));
> +           ((unsigned long) old_end & (pagesize - 1)) == 0));
>  
>    /* Precondition: not enough current space to satisfy nb request */
>    assert ((unsigned long) (old_size) < (unsigned long) (nb + MINSIZE));
> @@ -2447,7 +2450,7 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>           previous calls. Otherwise, we correct to page-align below.
>         */
>  
> -      size = (size + pagemask) & ~pagemask;
> +      size = ALIGN_UP (size, pagesize);
>  
>        /*
>           Don't try to call MORECORE if argument is so big as to appear
> @@ -2481,7 +2484,7 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>  
>            /* Cannot merge with old top, so add its size back in */
>            if (contiguous (av))
> -            size = (size + old_size + pagemask) & ~pagemask;
> +            size = ALIGN_UP (size + old_size, pagesize);
>  
>            /* If we are relying on mmap as backup, then use larger units */
>            if ((unsigned long) (size) < (unsigned long) (MMAP_AS_MORECORE_SIZE))
> @@ -2587,7 +2590,7 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>  
>                    /* Extend the end address to hit a page boundary */
>                    end_misalign = (INTERNAL_SIZE_T) (brk + size + correction);
> -                  correction += ((end_misalign + pagemask) & ~pagemask) - end_misalign;
> +                  correction += (ALIGN_UP (end_misalign, pagesize)) - end_misalign;
>  
>                    assert (correction >= 0);
>                    snd_brk = (char *) (MORECORE (correction));
> @@ -2738,10 +2741,10 @@ systrim (size_t pad, mstate av)
>    long released;         /* Amount actually released */
>    char *current_brk;     /* address returned by pre-check sbrk call */
>    char *new_brk;         /* address returned by post-check sbrk call */
> -  size_t pagesz;
> +  size_t pagesize;
>    long top_area;
>  
> -  pagesz = GLRO (dl_pagesize);
> +  pagesize = GLRO (dl_pagesize);
>    top_size = chunksize (av->top);
>  
>    top_area = top_size - MINSIZE - 1;
> @@ -2749,7 +2752,7 @@ systrim (size_t pad, mstate av)
>      return 0;
>  
>    /* Release in pagesize units, keeping at least one page */
> -  extra = (top_area - pad) & ~(pagesz - 1);
> +  extra = (top_area - pad) & ~(pagesize - 1);
>  
>    if (extra == 0)
>      return 0;
> @@ -2834,7 +2837,7 @@ static mchunkptr
>  internal_function
>  mremap_chunk (mchunkptr p, size_t new_size)
>  {
> -  size_t page_mask = GLRO (dl_pagesize) - 1;
> +  size_t pagesize = GLRO (dl_pagesize);
>    INTERNAL_SIZE_T offset = p->prev_size;
>    INTERNAL_SIZE_T size = chunksize (p);
>    char *cp;
> @@ -2843,7 +2846,7 @@ mremap_chunk (mchunkptr p, size_t new_size)
>    assert (((size + offset) & (GLRO (dl_pagesize) - 1)) == 0);
>  
>    /* Note the extra SIZE_SZ overhead as in mmap_chunk(). */
> -  new_size = (new_size + offset + SIZE_SZ + page_mask) & ~page_mask;
> +  new_size = ALIGN_UP (new_size + offset + SIZE_SZ, pagesize);
>  
>    /* No need to remap if the number of pages does not change.  */
>    if (size + offset == new_size)
> @@ -3122,8 +3125,8 @@ __libc_valloc (size_t bytes)
>      ptmalloc_init ();
>  
>    void *address = RETURN_ADDRESS (0);
> -  size_t pagesz = GLRO (dl_pagesize);
> -  return _mid_memalign (pagesz, bytes, address);
> +  size_t pagesize = GLRO (dl_pagesize);
> +  return _mid_memalign (pagesize, bytes, address);
>  }
>  
>  void *
> @@ -3133,18 +3136,17 @@ __libc_pvalloc (size_t bytes)
>      ptmalloc_init ();
>  
>    void *address = RETURN_ADDRESS (0);
> -  size_t pagesz = GLRO (dl_pagesize);
> -  size_t page_mask = GLRO (dl_pagesize) - 1;
> -  size_t rounded_bytes = (bytes + page_mask) & ~(page_mask);
> +  size_t pagesize = GLRO (dl_pagesize);
> +  size_t rounded_bytes = ALIGN_UP (bytes, pagesize);
>  
>    /* Check for overflow.  */
> -  if (bytes > SIZE_MAX - 2 * pagesz - MINSIZE)
> +  if (bytes > SIZE_MAX - 2 * pagesize - MINSIZE)
>      {
>        __set_errno (ENOMEM);
>        return 0;
>      }
>  
> -  return _mid_memalign (pagesz, rounded_bytes, address);
> +  return _mid_memalign (pagesize, rounded_bytes, address);
>  }
>  
>  void *
> ---
> 
> Cheers,
> Carlos.

Attachment: pgpC0tC889Ou1.pgp
Description: PGP signature