[PATCH] Revert "malloc: Do not call madvise if heap's oldsize >= THP size"

[Adhemerval Zanella Netto]

On 13/03/26 02:48, Dev Jain wrote:
> This reverts commit 6e8f32d39a57aa1f31bf15375810aab79a0f5f4b.
> 
> First off, apologies for my misunderstanding on how madvise(MADV_HUGEPAGE)
> works. I had the misconception that doing madvise(p, 1, MADV_HUGEPAGE) will set
> VM_HUGEPAGE on the entire VMA - it does not, it will align the size to
> PAGE_SIZE (4k) and then *split* the VMA. Only the first page-length of the
> virtual space will VM_HUGEPAGE'd, the rest of it will stay the same.

No worries, do you know if this is properly documented somewhere in kernel
docs or in manual? It could be useful to raise this to kernel devs to avoid
potential pitfall in other projects.

> 
> The above is the semantics for all madvise() calls - which makes sense from a
> UABI perspective. madvise() should do the proposed thing to only the length
> (page-aligned) which it was asked to do, doing any more than that is not
> something the user is expecting.
> 
> Commit 6e8f32d39a57 tries to optimize around the madvise() call by determining
> whether the VMA got madvise'd before. This will work for most cases except
> the following: if check_may_shrink_heap() is true, shrink_heap() re-maps the
> shrunk portion, giving us a new VMA altogether. That VMA won't have the
> VM_HUGEPAGE flag.
> 
> Reverting this commit, we will again mark the new VMA with VM_HUGEPAGE, and
> the kernel will merge the two into a single VMA marked with VM_HUGEPAGE.
> 
> This may be the only case where we lose VM_HUGEPAGE, and we could micro-optimize
> by extending the current if-condition with !check_may_shrink_heap. But let us
> not do this - this is very difficult to reason about, and I am soon going
> to propose mmap(MAP_HUGEPAGE) in Linux to do away with all these workarounds.
> 
> ---
> No regression observed on SPEC. I examined my other patch doing a similar
> optimization in mremap_chunk() and that looks fine to me - mremap() does not
> drop VMA flags.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
>  malloc/arena.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 75f2f32b5f..03a812f54d 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -486,10 +486,7 @@ grow_heap (heap_info *h, long diff)
>        h->mprotect_size = new_size;
>      }
>  
> -  /* mprotect preserves MADV_HUGEPAGE semantics - this means that if the old
> -     region was marked with MADV_HUGEPAGE, the new region will retain that.  */
> -  if (h->size < mp_.thp_pagesize)
> -    madvise_thp (h, new_size);
> +  madvise_thp (h, new_size);
>  
>    h->size = new_size;
>    LIBC_PROBE (memory_heap_more, 2, h, h->size);