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

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

[Sourabh Jain]

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.

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)
+{
+	if (hugetlb_disabled)
+		return 0;
+
+	return HPAGE_SHIFT != 0;
+}
+#define hugepages_supported hugepages_supported
+
 void __init hugetlbpage_init_defaultsize(void);
 
 int slice_is_hugepage_only_range(struct mm_struct *mm, unsigned long addr,
-- 
2.47.0

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

[Ritesh Harjani]

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)
> +{
> +	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.

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. Quick
qn, was this caught due to any OOM? Or was it an observation?


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

-ritesh

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

[Ritesh Harjani]

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

Re: [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@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)
>> +{
>> +	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@gmail.com>

Thanks for the review, Ritesh.

- Sourabh Jain

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

[Sourabh Jain]

Hello Ritesh,


On 17/12/24 02:19, Ritesh Harjani (IBM) wrote:
> 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.
Yes, fixed in v2:
https://lore.kernel.org/all/20241217074640.1064510-1-sourabhjain@linux.ibm.com/

Thanks again.

- Sourabh Jain