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

[Dev Jain]

On 20/11/25 10:51 pm, Adhemerval Zanella Netto wrote:
>
> On 17/11/25 02:40, Dev Jain wrote:
>> On 14/11/25 9:57 pm, Adhemerval Zanella Netto wrote:
>>> On 09/11/25 04:29, 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.
>>> I am not sure if the calculation of
>>>
>>>>    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;
>>>> +
>>>>          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));
>>>> -      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);
>>> I am not sure if this calculation is fully correct, with a simple testcase:
>>>
>>> $ cat t.c
>>> #include <stdlib.h>
>>> #include <pthread.h>
>>>
>>> static void *tf (void* arg)
>>> {
>>>     for (int i = 0; i < 1024; i++)
>>>       malloc (4096);
>>>     return NULL;
>>> }
>>>
>>> int main ()
>>> {
>>>     for (int i = 0; i < 1024; i++)
>>>       malloc (4096);
>>>
>>>     pthread_t t;
>>>     pthread_create (&t, NULL, tf, NULL);
>>>     pthread_join (t, NULL);
>>> }
>>> $ strace -e madvise -f -E GLIBC_TUNABLES=glibc.malloc.hugetlb=1 elf/ld.so --library-path . ./t
>>> madvise(0xaaaade400000, 3354624, MADV_HUGEPAGE) = -1 ENOMEM (Cannot allocate memory)
>>> madvise(0xf7fa28ed0000, 65536, 0x66 /* MADV_??? */) = 0
>>> strace: Process 313335 attached
>>> [pid 313335] madvise(0xf7fa24000000, 2097152, MADV_HUGEPAGE) = 0
>>> [pid 313335] madvise(0xf7fa28ed0000, 8314880, MADV_DONTNEED) = 0
>>> [pid 313335] +++ exited with 0 +++
>>> +++ exited with 0 +++
>>>
>>> Where without this patch:
>>>
>>> $ strace -e madvise -f -E GLIBC_TUNABLES=glibc.malloc.hugetlb=1 ./t
>>> madvise(0xc3da42fb9000, 2097152, MADV_HUGEPAGE) = 0
>>> madvise(0xc3da43200000, 2097152, MADV_HUGEPAGE) = 0
>>> strace: Process 313347 attached
>>> [pid 313347] madvise(0xfa19510a0000, 8314880, MADV_DONTNEED) = 0
>>> [pid 313347] +++ exited with 0 +++
>>> +++ exited with 0 +++
>> Thanks for testing! Although without the patch, I get this:
>>
>> madvise(0xfffff8000000, 2097152, MADV_HUGEPAGE) = 0
>> madvise(0xfffff8000000, 4194304, MADV_HUGEPAGE) = 0
>> madvise(0xfffff8000000, 6291456, MADV_HUGEPAGE) = 0
>> madvise(0xfffff7400000, 65536, 0x66 /* MADV_??? */) = -1 EINVAL (Invalid argument)
>> strace: Process 3162240 attached
>> [pid 3162240] madvise(0xfffff0000000, 2097152, MADV_HUGEPAGE) = 0
>> [pid 3162240] madvise(0xfffff7400000, 8314880, MADV_DONTNEED) = 0
>> [pid 3162240] +++ exited with 0 +++
>> +++ exited with 0 +++
>>
>> I don't know where the EINVAL is coming from, that shouldn't be happening, because we
>> have hardcoded MADV_HUGEPAGE everywhere. I played around and I think that when madvise() is not invoked
>> ever, then strace shows it returning EINVAL. I am not familiar with how strace actually
>> works and how to interpret the output.
> The EINVAL most likely come from the size not being multiple of the large
> page size, but I am not sure.  I could not understand why the flags are being
> shown as invalid (0x66), since I step on every madvise syscall trap and
> I did not see this valug.

For MADV_HUGEPAGE, the size does not matter, if you just do a madvise(brk, 1, MADV_HUGEPAGE)
it will work.

>
>> But, there is an obvious error in this patch. I shouldn't be making the madvise() call
>> on brk pointer, but on init_sbrk_base. Calling it on brk will cross the heap VMA, and
>> the target range may include a hole. I checked the kernel source and it returns -ENOMEM
>> if it encounters a gap.
>>
>> But I have a question. In madvise_thp, we align the pointer down if it is not aligned to
>> pagesize. If before that pointer, there is a gap, then madvise() will return -ENOMEM,
>> although, the madvise() operation will continue on the VMA which we have specified.
>> So, the operation will do what we want it to do, but return -ENOMEM. Do you think
>> this is something which needs to be fixed? The fix is simple: Align the pointer up
>> instead of down, and subtract from size instead of add. Then we can check again
>> whether size >= thp_pagesize, and continue with the syscall.
> Are you referring to the case where the initial heap is not largepage aligned, then
> the align down to a possible invalid address? If kernel returns ENOMEM in such case
> (I have not checked yet), your suggestion might work although I think it will not
> cover the initial page.

I am referring to this:

/* Linux requires the input address to be page-aligned, and unaligned
inputs happens only for initial data segment. */
if(__glibc_unlikely(!PTR_IS_ALIGNED(p, GLRO(dl_pagesize))))
{
void*q = PTR_ALIGN_DOWN(p, GLRO(dl_pagesize));
size += PTR_DIFF(p, q);
p = q;
}

In this case we should be aligning the pointer up and subtracting from size.
I'll send a patch to fix this - although it does not really matter, but I think
we should do this cleanly and not suppress the -ENOMEM even when the MADV_HUGEPAGE
operation will work.

>
>>>
>>> The second madvise call 'madvise(0xf7fa28ed0000, 65536, 0x66 /* MADV_??? */)'
>>> is indeed even bogus.  With gdb the value passed on the syscall are indeed:
>>>
>>> (gdb) catch syscall madvise
>>> (gdb) r
>>> [...]
>>> (gdb) i r
>>> x0             0xfffffffffffffff4  -12
>>> x1             0x355000            3493888
>>> x2             0xe                 14
>>> [...]
>>>
>>>> +      }
>>>>              LIBC_PROBE (memory_sbrk_more, 2, brk, size);
>>>>            }
>>>>    
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20251122/75bf3202/attachment-0001.htm>