Re: [PATCH v4] powerpc/hugetlb: Disable gigantic hugepages if fadump is active

[Sourabh Jain <sourabhjain@linux.ibm.com>]

Hello Ritesh,

Thanks for the review.

On 02/03/25 12:05, Ritesh Harjani (IBM) wrote:
> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>
>> The fadump kernel boots with limited memory solely to collect the kernel
>> core dump. Having gigantic hugepages in the fadump kernel is of no use.
> Sure got it.
>
>> Many times, the fadump kernel encounters OOM (Out of Memory) issues if
>> gigantic hugepages are allocated.
>>
>> To address this, disable gigantic hugepages if fadump is active by
>> returning early from arch_hugetlb_valid_size() using
>> hugepages_supported(). When fadump is active, the global variable
>> hugetlb_disabled is set to true, which is later used by the
>> PowerPC-specific hugepages_supported() function to determine hugepage
>> support.
>>
>> Returning early from arch_hugetlb_vali_size() not only disables
>> gigantic hugepages but also avoids unnecessary hstate initialization for
>> every hugepage size supported by the platform.
>>
>> kernel logs related to hugepages with this patch included:
>> kernel argument passed: hugepagesz=1G hugepages=1
>>
>> First kernel: gigantic hugepage got allocated
>> ==============================================
>>
>> dmesg | grep -i "hugetlb"
>> -------------------------
>> HugeTLB: registered 1.00 GiB page size, pre-allocated 1 pages
>> HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
>> HugeTLB: registered 2.00 MiB page size, pre-allocated 0 pages
>> HugeTLB: 0 KiB vmemmap can be freed for a 2.00 MiB page
>>
>> $ cat /proc/meminfo | grep -i "hugetlb"
>> -------------------------------------
>> Hugetlb:         1048576 kB
> Was this tested with patch [1] in your local tree?
>
> [1]: https://web.git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=d629d7a8efc33
>
> IIUC, this patch [1] disables the boot time allocation of hugepages.
> Isn't it also disabling the boot time allocation for gigantic huge pages
> passed by the cmdline params like hugepagesz=1G and hugepages=2 ?

Yes, I had the patch [1] in my tree.

My understanding is that gigantic pages are allocated before normal huge 
pages.

In hugepages_setup() in hugetlb.c, we have:

     if (hugetlb_max_hstate && hstate_is_gigantic(parsed_hstate))
         hugetlb_hstate_alloc_pages(parsed_hstate);

I believe the above code allocates memory for gigantic pages, and 
hugetlb_init() is
called later because it is a subsys_initcall.

So, by the time the kernel reaches hugetlb_init(), the gigantic pages 
are already
allocated. Isn't that right?

Please let me know your opinion.

Thanks,
Sourabh Jain


>
>
>> HugeTLB: registered 1.00 GiB page size, pre-allocated 1 pages
> This print comes from report_hugepages(). The only place from where
> report_hugepages() gets called is hugetlb_init(). hugetlb_init() is what
> is responsible for hugepages & gigantic hugepage allocations of the
> passed kernel cmdline params.
>
> But hugetlb_init() already checks for hugepages_supported() in the very
> beginning. So I am not sure whether we need this extra patch to disable
> gigantic hugepages allocation by the kernel cmdline params like
> hugepagesz=1G and hugepages=2 type of options.
>
> Hence I was wondering if you had this patch [1] in your tree when you were
> testing this?
>
> But I may be missing something. Could you please help clarify on whether
> we really need this patch to disable gigantic hugetlb page allocations?
>
>> Fadump kernel: gigantic hugepage not allocated
>> ===============================================
>>
>> dmesg | grep -i "hugetlb"
>> -------------------------
>> [    0.000000] HugeTLB: unsupported hugepagesz=1G
>> [    0.000000] HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>> [    0.706375] HugeTLB support is disabled!
>> [    0.773530] hugetlbfs: disabling because there are no supported hugepage sizes
>>
>> $ cat /proc/meminfo | grep -i "hugetlb"
>> ----------------------------------
>> <Nothing>
>>
>> Cc: Hari Bathini <hbathini@linux.ibm.com>
>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
> I guess the extra " in the above was not adding me in the cc list.
> Hence I missed to see this patch early.

Thanks for point it out. I will fix it.


>
> -ritesh
>
>
>> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>> Changelog:
>>
>> v1:
>> https://lore.kernel.org/all/20250121150419.1342794-1-sourabhjain@linux.ibm.com/
>>
>> v2:
>> https://lore.kernel.org/all/20250124103220.111303-1-sourabhjain@linux.ibm.com/
>>   - disable gigantic hugepage in arch code, arch_hugetlb_valid_size()
>>
>> v3:
>> https://lore.kernel.org/all/20250125104928.88881-1-sourabhjain@linux.ibm.com/
>>   - Do not modify the initialization of the shift variable
>>
>> v4:
>> - Update commit message to include how hugepages_supported() detects
>>    hugepages support when fadump is active
>> - Add Reviewed-by tag
>> - NO functional change
>>
>> ---
>>   arch/powerpc/mm/hugetlbpage.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
>> index 6b043180220a..88cfd182db4e 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -138,6 +138,9 @@ bool __init arch_hugetlb_valid_size(unsigned long size)
>>   	int shift = __ffs(size);
>>   	int mmu_psize;
>>
>> +	if (!hugepages_supported())
>> +		return false;
>> +
>>   	/* Check that it is a page size supported by the hardware and
>>   	 * that it fits within pagetable and slice limits. */
>>   	if (size <= PAGE_SIZE || !is_power_of_2(size))
>> --
>> 2.48.1