Re: [PATCH v1] mm/huge_memory: fix shrinking of all-zero THPs with max_ptes_none default

[Usama Arif]

On 05/09/2025 15:46, David Hildenbrand wrote:
> [...]
> 
>>
>> The reason I did this is for the case if you change max_ptes_none after the THP is added
>> to deferred split list but *before* memory pressure, i.e. before the shrinker runs,
>> so that its considered for splitting.
> 
> Yeah, I was assuming that was the reason why the shrinker is enabled as default.
> 
> But in any sane system, the admin would enable the shrinker early. If not, we can look into handling it differently.

Yes, I do this as well, i.e. have a low value from the start.

Does it make sense to disable shrinker if max_ptes_none is 511? It wont shrink
the usecase you are describing below, but we wont encounter the increased CPU usage.> 
>>
>>> Easy to reproduce:
>>>
>>> 1) Allocate some THPs filled with 0s
>>>
>>> <prog.c>
>>>   #include <string.h>
>>>   #include <stdio.h>
>>>   #include <stdlib.h>
>>>   #include <unistd.h>
>>>   #include <sys/mman.h>
>>>
>>>   const size_t size = 1024*1024*1024;
>>>
>>>   int main(void)
>>>   {
>>>           size_t offs;
>>>           char *area;
>>>
>>>           area = mmap(0, size, PROT_READ | PROT_WRITE,
>>>                       MAP_ANON | MAP_PRIVATE, -1, 0);
>>>           if (area == MAP_FAILED) {
>>>                   printf("mmap failed\n");
>>>                   exit(-1);
>>>           }
>>>           madvise(area, size, MADV_HUGEPAGE);
>>>
>>>           for (offs = 0; offs < size; offs += getpagesize())
>>>                   area[offs] = 0;
>>>           pause();
>>>   }
>>> <\prog.c>
>>>
>>> 2) Trigger the shrinker
>>>
>>> E.g., memory pressure through memhog
>>>
>>> 3) Observe that THPs are not getting reclaimed
>>>
>>> $ cat /proc/`pgrep prog`/smaps_rollup
>>>
>>> Would list ~1GiB of AnonHugePages. With this fix, they would get
>>> reclaimed as expected.
>>>
>>> Fixes: dafff3f4c850 ("mm: split underused THPs")
>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>> Cc: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>>> Cc: Zi Yan <ziy@xxxxxxxxxx>
>>> Cc: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>>> Cc: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
>>> Cc: Nico Pache <npache@xxxxxxxxxx>
>>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>>> Cc: Dev Jain <dev.jain@xxxxxxx>
>>> Cc: Barry Song <baohua@xxxxxxxxxx>
>>> Cc: Usama Arif <usamaarif642@xxxxxxxxx>
>>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>>> ---
>>>   mm/huge_memory.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 26cedfcd74189..aa3ed7a86435b 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -4110,9 +4110,6 @@ static bool thp_underused(struct folio *folio)
>>>       void *kaddr;
>>>       int i;
>>>   -    if (khugepaged_max_ptes_none == HPAGE_PMD_NR - 1)
>>> -        return false;
>>> -
>>
>> I do agree with your usecase, but I am really worried about the amount of
>> work and cpu time the THP shrinker will consume when max_ptes_none is 511
>> (I dont have any numbers to back up my worry :)), and its less likely that
>> we will have these completely zeroed out THPs (again no numbers to back up
>> this statement).
> 
> Then then shrinker shall be deactivated as default if that becomes a problem.
> 
> Fortunately you documented the desired semantics:
> 
> "All THPs at fault and collapse time will be added to _deferred_list,
> and will therefore be split under memory pressure if they are considered
> "underused". A THP is underused if the number of zero-filled pages in
> the THP is above max_ptes_none (see below)."
> 
>> We have the huge_zero_folio as well which is installed on read.
> 
> Yes, only if the huge zero folio is not available. Which will then also get properly reclaimed.
>