[PATCH] powerpc/book3s64/hugetlb: Fix disabling hugetlb when fadump is active

[Sourabh Jain]

Hello Ritesh,

On 17/12/24 00:07, Ritesh Harjani (IBM) wrote:
> Sourabh Jain <sourabhjain at linux.ibm.com> writes:
>
>> Commit 8597538712eb ("powerpc/fadump: Do not use hugepages when fadump
>> is active") disabled hugetlb support when fadump is active by returning
>> early from hugetlbpage_init():arch/powerpc/mm/hugetlbpage.c and not
>> populating hpage_shift/HPAGE_SHIFT.
>>
>> Later, commit 2354ad252b66 ("powerpc/mm: Update default hugetlb size
>> early") moved the allocation of hpage_shift/HPAGE_SHIFT to early boot,
>> which inadvertently re-enabled hugetlb support when fadump is active.
>>
>> Fix this by implementing hugepages_supported() on powerpc. This ensures
>> that disabling hugetlb for the fadump kernel is independent of
>> hpage_shift/HPAGE_SHIFT.
>>
> Thanks for describing the history of the changes clearly.
>
>> Fixes: 2354ad252b66 ("powerpc/mm: Update default hugetlb size early")
>> CC: Aneesh Kumar K.V <aneesh.kumar at kernel.org>
>> CC: Hari Bathini <hbathini at linux.ibm.com>
>> CC: Madhavan Srinivasan <maddy at linux.ibm.com>
>> Cc: Mahesh Salgaonkar <mahesh at linux.ibm.com>
>> Cc: Michael Ellerman <mpe at ellerman.id.au>
>> CC: Ritesh Harjani (IBM) <ritesh.list at gmail.com>
>> Signed-off-by: Sourabh Jain <sourabhjain at linux.ibm.com>
>> ---
>>
>> Note: Even with this fix included, it is possible to enable gigantic
>> pages in the fadump kernel. IIUC, gigantic pages were never disabled
>> for the fadump kernel.
>>
>> Currently, gigantic pages are allocated during early boot as long as
>> the respective hstate is supported by the architecture.
>>
>> I will introduce some changes in the generic hugetlb code to allow the
>> architecture to decide on supporting gigantic pages on the go. Bringing
>> gigantic page allocation under hugepages_supported() does work for
>> powerpc but I need verify the impact on other architectures.
>>
>> Regarding the Fixes tag: This patch fixes a bug inadvertently introduced
>> by the commit mentioned under Fixes tag in the commit message. Feel free
>> to remove the tag if it is unnecessary.
>>
>> ---
>>   arch/powerpc/include/asm/hugetlb.h | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h
>> index 18a3028ac3b6..f294e57663b0 100644
>> --- a/arch/powerpc/include/asm/hugetlb.h
>> +++ b/arch/powerpc/include/asm/hugetlb.h
>> @@ -15,6 +15,15 @@
>>   
>>   extern bool hugetlb_disabled;
>>   
>> +static inline int hugepages_supported(void)
>> +{
>> +	if (hugetlb_disabled)
>> +		return 0;
>> +
>> +	return HPAGE_SHIFT != 0;
>> +}
>> +#define hugepages_supported hugepages_supported
>> +
> In include/linux/hugetlb.h
>
> #ifndef hugepages_supported
> /*
>   * Some platform decide whether they support huge pages at boot
>   * time. Some of them, such as powerpc, set HPAGE_SHIFT to 0
>   * when there is no such support
>   */
> #define hugepages_supported() (HPAGE_SHIFT != 0)
> #endif
>
> The above comment is not entirely correct after this change 2354ad252b66
> ("powerpc/mm: Update default hugetlb size early), because we anyway go
> ahead and initialize HPAGE_SHIFT even when hugetlb_disabled is true. But
> nevertheless - we can fix the comment later. I see there are few other
> cleanups which could be clubbed too.


Agree. Since it requires generic changes, it can be handled separately.

> fadump when the capture kernel is active would like to disable hugetlb page
> allocation (to avoid OOMs) hence it uses hugetlb_disabled flag to mark
> it disabled. As you correctly pointed out, the change in question moved
> initialization of HPAGE_SHIFT to early boot as it was required to set the
> pageblock_order properly (especially for radix 2M huge pagesize).
>
> Now earlier generic hugepages_supported() was only checking for
> HPAGE_SHIFT != 0. This patch will now check for both, hugetlb_disabled
> should be false and HPAGE_SHIFT should not be 0. Only then hugetlb will
> go and allocate hugepages in hugetlb_init().
>
>
> So, the change looks good to me. Thanks for catching and fixing that.
> I hope we can add a testcase to cover this scenario as the problematic
> patch was added long ago - but we only noticed the problem now.

Yes, we have a plan to add a couple of test cases in op-test to cover
hugepages with fadump.

> Quick
> qn, was this caught due to any OOM? Or was it an observation?

Yes, the kernel was hitting OOM when hugepages were enabled and
fadump was active.

> The patch looks good to me. So please feel free to add -
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list at gmail.com>

Thanks for the review, Ritesh.

- Sourabh Jain