[PATCH] malloc: Optimize the madvise behaviour on the main heap

[Dev Jain]

On 09/11/25 12:59 pm, Dev Jain wrote:
> Linux handles virtual memory in Virtual Memory Areas (VMAs). The
> madvise(MADV_HUGEPAGE) call works on a VMA granularity, which sets the
> VM_HUGEPAGE flag on the VMA. Therefore, if we can guarantee that a VMA
> has been marked with VM_HUGEPAGE already, then we do not need to call
> madvise() on that VMA again.
>
> For mp_.thp_pagesize != 0, currently we align the new brk to the thp size.
> This means that after the first extension, all such brk extensions are
> guaranteed to produce an extension size >= thp size: madvise_thp() will
> invoke the madvise() syscall only if size >= thp size, and the other
> condition is related to the sysctl setting, wherein mp_.thp_mode will be
> same throughout the lifetime of the process. Therefore, currently we invoke
> the madvise() syscall on the heap on each extension, which is unnecessary.
>
> First, pass the total heap size, instead of the extension size, to
> madvise_thp: Linux does not care about the size passed, in case the
> madvise() syscall is invoked with MADV_HUGEPAGE flag, because the flag
> will be set on the entire VMA, no matter for what portion of the VMA
> the syscall is invoked.
>
> This enables us to do the following: if the old heap size >= thp size,
> we can guarantee that madvise() was invoked on one of the previous
> extensions of the heap. So, avoid making the syscall in this case.
>
> The tricky part is computing the size of the heap, i.e the current program
> break minus the initial program break. In case the first ever attempt at
> extending the break fails, mp_.sbrk_base will be set to an mmapped address.
> Therefore, we need some other way of remembering the initial location
> of the program break. We can reuse some code for this: MORECORE (0), when
> invoked for the first time ever, will give us the initial program break.
> ---
> The patch applies on 259adb087dd9. Built on Aarch64, all malloc tests pass.
>
>   malloc/malloc.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 0b21bdf1bd..277fb9e9ec 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1938,6 +1938,12 @@ struct malloc_par
>     /* First address handed out by MORECORE/sbrk.  */
>     char *sbrk_base;
>   
> +  /* The initial location of program break. This will most likely be equal
> +     to sbrk_base; in case the first ever extension attempt of brk fails,
> +     sbrk_base will point to an mmapped address (see sysmalloc_mmap_fallback),
> +     in which case these two values will not be equal.  */
> +  char *init_sbrk_base;
> +
>   #if USE_TCACHE
>     /* Maximum number of small buckets to use.  */
>     size_t tcache_small_bins;
> @@ -2667,6 +2673,9 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>         if (__glibc_unlikely (mp_.thp_pagesize != 0))
>   	{
>   	  uintptr_t lastbrk = (uintptr_t) MORECORE (0);
> +	  if (mp_.init_sbrk_base == NULL)
> +	    mp_.init_sbrk_base = (char *) lastbrk;

After my patch to enable THP by default on Aarch64, this code will break
because I will be initializing mp_.thp_pagesize later in madvise_thp. See
below...

> +
>   	  uintptr_t top = ALIGN_UP (lastbrk + size, mp_.thp_pagesize);
>   	  size = top - lastbrk;
>   	}
> @@ -2682,8 +2691,18 @@ sysmalloc (INTERNAL_SIZE_T nb, mstate av)
>         if ((ssize_t) size > 0)
>           {
>             brk = (char *) (MORECORE ((long) size));

I can instead initialize init_sbrk_base here, since MORECORE will return the
program break when invoked for the first time.

> -	  if (brk != (char *) (MORECORE_FAILURE))
> -	    madvise_thp (brk, size);
> +	  if (brk != (char *) (MORECORE_FAILURE)) {
> +	    size_t old_size = (size_t) (brk - mp_.init_sbrk_base);
> +
> +	    /*
> +	      If heap already marked with MADV_HUGEPAGE, skip madvise(). Note
> +	      that, we don't need to check mp_.init_sbrk_base != NULL; if it
> +	      is NULL, it implies that mp_.thp_pagesize == 0, in which case
> +	      madvise_thp() will not invoke madvise().
> +	     */
> +	    if (old_size < mp_.thp_pagesize)
> +	    madvise_thp (brk, old_size + size);
> +	  }
>             LIBC_PROBE (memory_sbrk_more, 2, brk, size);
>           }
>   

After fixing that, there is some error in this patch, I got a regression on SPEC with THP=madvise
and glibc.malloc.hugetlb=1. I suspect it has something to do with the madvise_thp (snd_brk, correction)
down in the code. As in, suppose the first extension of the heap is less than thp_pagesize, correction
is also less than thp_pagesize, but the sum of them is >= thp_pagesize. This will result in VM_HUGEPAGE
never being set in either of the madvise_thp() calls, but the resultant heap size will be >= thp_pagesize,
and my patch will avoid making the syscall, so VM_HUGEPAGE never gets set on the main heap.

But what I am trying to understand right now is, how is it possible to reach the if branch here:

else
{
front_misalign = 0;
end_misalign = 0;
correction = 0;
aligned_brk = brk;
/* handle contiguous cases */
if(contiguous(av))
{

Suppose that we reach here - This means that av is contiguous. This must imply that

snd_brk == MORECORE_FAILURE ... (i)

for if not, we have executed the following three lines:

set_noncontiguous(av);
/* We do not need, and cannot use, another sbrk call to find end */
brk = mbrk;
snd_brk = brk + size;

giving us a noncontiguous av.

Now, since we are in the else branch, the if branch in this if-else-if-else code:

brk == old_end && snd_brk == (char *) (MORECORE_FAILURE) is false. Combining this
fact with (i), there exists a case where brk != old_end. So now we have got a condition
in which brk != old_end and the heap is contiguous. How is this possible?

I can confirm that we do hit this if (contiguous (av)) branch by putting an assert (1 == 0)
there and running malloc tests and hitting the assertion.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20251123/ce8f97ec/attachment.htm>