Re: [PATCH] mm/hugetlb: bring gigantic page allocation under hugepages_supported()

Re: [PATCH] mm/hugetlb: bring gigantic page allocation under hugepages_supported()

[Gerald Schaefer]

On Tue, 21 Jan 2025 20:34:19 +0530
Sourabh Jain <sourabhjain@linux.ibm.com> wrote:

> Despite having kernel arguments to enable gigantic hugepages, this
> provides a way for the architecture to disable gigantic hugepages on the
> fly, similar to what we do for hugepages.
> 
> Components like fadump (PowerPC-specific) need this functionality to
> disable gigantic hugepages when the kernel is booted solely to collect
> the kernel core dump.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Muchun Song <muchun.song@linux.dev>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
> 
> To evaluate the impact of this change on architectures other than
> PowerPC, I did the following analysis:
> 
> For architectures where hugepages_supported() is not redefined, it
> depends on HPAGE_SHIFT, which is found to be a constant. It is mostly
> initialized to PMD_SHIFT.
> 
> Architecture : HPAGE_SHIFT initialized with
> 
> ARC: PMD_SHIFT (constant)
> ARM: PMD_SHIFT (constant)
> ARM64: PMD_SHIFT (constant)
> Hexagon: 22 (constant)
> LoongArch: (PAGE_SHIFT + PAGE_SHIFT - 3) (appears to be constant)
> MIPS: (PAGE_SHIFT + PAGE_SHIFT - 3) (appears to be constant)
> PARISC: PMD_SHIFT (appears to be constant)
> RISC-V: PMD_SHIFT (constant)
> SH: 16 | 18 | 20 | 22 | 26 (constant)
> SPARC: 23 (constant)
> 
> So seems like this change shouldn't have any impact on above
> architectures.
> 
> On the S390 and X86 architectures, hugepages_supported() is redefined,
> and I am uncertain at what point it is safe to call
> hugepages_supported().

For s390, hugepages_supported() checks EDAT1 machine flag, which is
initialized long before any initcalls. So it is safe to be called
here.

My common code hugetlb skills got a little rusty, but shouldn't
arch_hugetlb_valid_size() already prevent getting here for gigantic
hugepages, in case they are not supported? And could you not use
that for your purpose?

BTW, please also add arch mailing list for such questions.

> 
> ---
>  mm/hugetlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index cec4b121193f..48b42b8d26b4 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4629,7 +4629,7 @@ static int __init hugepages_setup(char *s)
>  	 * But we need to allocate gigantic hstates here early to still
>  	 * use the bootmem allocator.
>  	 */
> -	if (hugetlb_max_hstate && hstate_is_gigantic(parsed_hstate))
> +	if (hugetlb_max_hstate && hstate_is_gigantic(parsed_hstate) && hugepages_supported())
>  		hugetlb_hstate_alloc_pages(parsed_hstate);
>  
>  	last_mhp = mhp;

Re: [PATCH] mm/hugetlb: bring gigantic page allocation under hugepages_supported()

[Sourabh Jain]

Hello Gerald,

On 22/01/25 19:36, Gerald Schaefer wrote:
> On Tue, 21 Jan 2025 20:34:19 +0530
> Sourabh Jain <sourabhjain@linux.ibm.com> wrote:
>
>> Despite having kernel arguments to enable gigantic hugepages, this
>> provides a way for the architecture to disable gigantic hugepages on the
>> fly, similar to what we do for hugepages.
>>
>> Components like fadump (PowerPC-specific) need this functionality to
>> disable gigantic hugepages when the kernel is booted solely to collect
>> the kernel core dump.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Heiko Carstens <hca@linux.ibm.com>
>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>> Cc: Muchun Song <muchun.song@linux.dev>
>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>
>> To evaluate the impact of this change on architectures other than
>> PowerPC, I did the following analysis:
>>
>> For architectures where hugepages_supported() is not redefined, it
>> depends on HPAGE_SHIFT, which is found to be a constant. It is mostly
>> initialized to PMD_SHIFT.
>>
>> Architecture : HPAGE_SHIFT initialized with
>>
>> ARC: PMD_SHIFT (constant)
>> ARM: PMD_SHIFT (constant)
>> ARM64: PMD_SHIFT (constant)
>> Hexagon: 22 (constant)
>> LoongArch: (PAGE_SHIFT + PAGE_SHIFT - 3) (appears to be constant)
>> MIPS: (PAGE_SHIFT + PAGE_SHIFT - 3) (appears to be constant)
>> PARISC: PMD_SHIFT (appears to be constant)
>> RISC-V: PMD_SHIFT (constant)
>> SH: 16 | 18 | 20 | 22 | 26 (constant)
>> SPARC: 23 (constant)
>>
>> So seems like this change shouldn't have any impact on above
>> architectures.
>>
>> On the S390 and X86 architectures, hugepages_supported() is redefined,
>> and I am uncertain at what point it is safe to call
>> hugepages_supported().
> For s390, hugepages_supported() checks EDAT1 machine flag, which is
> initialized long before any initcalls. So it is safe to be called
> here.
Thanks for the info.
>
> My common code hugetlb skills got a little rusty, but shouldn't
> arch_hugetlb_valid_size() already prevent getting here for gigantic
> hugepages, in case they are not supported? And could you not use
> that for your purpose?

Yes, handling this in arch_hugetlb_valid_size is even better. That way,
we can avoid initializing data structures to hold hstate, which is not
required anyway.

Thanks for the review and suggestion. I will handle this in the
architecture-specific code.

- Sourabh Jain

Re: [PATCH] mm/hugetlb: bring gigantic page allocation under hugepages_supported()

[Hari Bathini]

On 23/01/25 9:00 am, Sourabh Jain wrote:
> Hello Gerald,
> 
> On 22/01/25 19:36, Gerald Schaefer wrote:
>> On Tue, 21 Jan 2025 20:34:19 +0530
>> Sourabh Jain <sourabhjain@linux.ibm.com> wrote:
>>
>>> Despite having kernel arguments to enable gigantic hugepages, this
>>> provides a way for the architecture to disable gigantic hugepages on the
>>> fly, similar to what we do for hugepages.
>>>
>>> Components like fadump (PowerPC-specific) need this functionality to
>>> disable gigantic hugepages when the kernel is booted solely to collect
>>> the kernel core dump.
>>>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: Borislav Petkov <bp@alien8.de>
>>> Cc: Heiko Carstens <hca@linux.ibm.com>
>>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>>> Cc: Muchun Song <muchun.song@linux.dev>
>>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: linux-mm@kvack.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>>> ---
>>>
>>> To evaluate the impact of this change on architectures other than
>>> PowerPC, I did the following analysis:
>>>
>>> For architectures where hugepages_supported() is not redefined, it
>>> depends on HPAGE_SHIFT, which is found to be a constant. It is mostly
>>> initialized to PMD_SHIFT.
>>>
>>> Architecture : HPAGE_SHIFT initialized with
>>>
>>> ARC: PMD_SHIFT (constant)
>>> ARM: PMD_SHIFT (constant)
>>> ARM64: PMD_SHIFT (constant)
>>> Hexagon: 22 (constant)
>>> LoongArch: (PAGE_SHIFT + PAGE_SHIFT - 3) (appears to be constant)
>>> MIPS: (PAGE_SHIFT + PAGE_SHIFT - 3) (appears to be constant)
>>> PARISC: PMD_SHIFT (appears to be constant)
>>> RISC-V: PMD_SHIFT (constant)
>>> SH: 16 | 18 | 20 | 22 | 26 (constant)
>>> SPARC: 23 (constant)
>>>
>>> So seems like this change shouldn't have any impact on above
>>> architectures.
>>>
>>> On the S390 and X86 architectures, hugepages_supported() is redefined,
>>> and I am uncertain at what point it is safe to call
>>> hugepages_supported().
>> For s390, hugepages_supported() checks EDAT1 machine flag, which is
>> initialized long before any initcalls. So it is safe to be called
>> here.
> Thanks for the info.
>>
>> My common code hugetlb skills got a little rusty, but shouldn't
>> arch_hugetlb_valid_size() already prevent getting here for gigantic
>> hugepages, in case they are not supported? And could you not use
>> that for your purpose?
> 
> Yes, handling this in arch_hugetlb_valid_size is even better. That way,
> we can avoid initializing data structures to hold hstate, which is not
> required anyway.
> 
> Thanks for the review and suggestion. I will handle this in the
> architecture-specific code.

Yeah, adding a check for hugetlb_disabled in arch_hugetlb_valid_size()
should take care of things?

- Hari