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

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Sourabh Jain <sourabhjain@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@kernel.org>
>> 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>
>> Signed-off-by: Sourabh Jain <sourabhjain@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)

I guess we may as well make it's return type as bool.

>> +{
>> +	if (hugetlb_disabled)
>> +		return 0;
>> +
>> +	return HPAGE_SHIFT != 0;
>> +}
>> +#define hugepages_supported hugepages_supported
>> +

-ritesh