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

[Dev Jain]

On 16/03/26 11:32 pm, Adhemerval Zanella Netto wrote:
> 
> 
> 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.

This behavior isn’t documented in the man pages because it’s an internal
implementation detail, and userspace shouldn’t rely on such details.

Regarding the realloc patch: from my understanding, it’s a reasonable
expectation that the kernel preserves the "properties of the mappings"
(which I have previously described in my patch descriptions as "VMA flags",
a kernel internal detail) across mremap(). However, strictly speaking, the
kernel is free to change such behavior without violating the uABI, since
this is not formally guaranteed.

So philosophically, even the realloc patch relies on assumptions about
kernel behavior that aren’t part of any user-facing contract. In that
sense, it is not entirely “correct” from a purist standpoint. That said, it
aligns with existing kernel behavior and practical expectations, so it
should be acceptable in practice.

On the other hand, the mprotect patch is simply incorrect.


> 
>>
>> 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);
>