Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On Fri, 17 Apr 2020 11:50:45 -0700 Mike Kravetz wrote: > v3 - >Used weak attribute method of defining arch_hugetlb_valid_size. > This eliminates changes to arch specific hugetlb.h files (Peter) >Updated documentation (Peter, Randy) >Fixed handling of implicitly specified gigantic page preallocation > in existing code and removed documentation of such. There is now > no difference between handling of gigantic and non-gigantic pages. > (Peter, Nitesh). > This requires the most review as there is a small change to > undocumented behavior. See patch 4 commit message for details. >Added Acks and Reviews (Mina, Peter) > > v2 - >Fix build errors with patch 1 (Will) >Change arch_hugetlb_valid_size arg to unsigned long and remove > irrelevant 'extern' keyword (Christophe) >Documentation and other misc changes (Randy, Christophe, Mina) >Do not process command line options if !hugepages_supported() > (Dave, but it sounds like we may want to additional changes to > hugepages_supported() for x86? If that is needed I would prefer > a separate patch.) > > Longpeng(Mike) reported a weird message from hugetlb command line processing > and proposed a solution [1]. While the proposed patch does address the > specific issue, there are other related issues in command line processing. > As hugetlbfs evolved, updates to command line processing have been made to > meet immediate needs and not necessarily in a coordinated manner. The result > is that some processing is done in arch specific code, some is done in arch > independent code and coordination is problematic. Semantics can vary between > architectures. > > The patch series does the following: > - Define arch specific arch_hugetlb_valid_size routine used to validate > passed huge page sizes. > - Move hugepagesz= command line parsing out of arch specific code and into > an arch independent routine. > - Clean up command line processing to follow desired semantics and > document those semantics. > > [1] > https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpe...@huawei.com > > Mike Kravetz (4): > hugetlbfs: add arch_hugetlb_valid_size > hugetlbfs: move hugepagesz= parsing to arch independent code > hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate > hugetlbfs: clean up command line processing > > .../admin-guide/kernel-parameters.txt | 40 ++-- > Documentation/admin-guide/mm/hugetlbpage.rst | 35 > arch/arm64/mm/hugetlbpage.c | 30 +-- > arch/powerpc/mm/hugetlbpage.c | 30 +-- > arch/riscv/mm/hugetlbpage.c | 24 +-- > arch/s390/mm/hugetlbpage.c| 24 +-- > arch/sparc/mm/init_64.c | 43 +--- > arch/x86/mm/hugetlbpage.c | 23 +-- > include/linux/hugetlb.h | 2 +- > mm/hugetlb.c | 190 +++--- > 10 files changed, 271 insertions(+), 170 deletions(-) > Looks good and works fine for s390, thanks for cleaning up! Acked-by: Gerald Schaefer # s390
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On Mon, Apr 20, 2020 at 02:40:05PM -0700, Mike Kravetz wrote:
> On 4/20/20 1:29 PM, Anders Roxell wrote:
> > On Mon, 20 Apr 2020 at 20:23, Mike Kravetz wrote:
> >> On 4/20/20 8:34 AM, Qian Cai wrote:
> >>>
> >>> Reverted this series fixed many undefined behaviors on arm64 with the
> >>> config,
> >> While rearranging the code (patch 3 in series), I made the incorrect
> >> assumption that CONT_XXX_SIZE == (1UL << CONT_XXX_SHIFT). However,
> >> this is not the case. Does the following patch fix these issues?
> >>
> >> From b75cb4a0852e208bee8c4eb347dc076fcaa88859 Mon Sep 17 00:00:00 2001
> >> From: Mike Kravetz
> >> Date: Mon, 20 Apr 2020 10:41:18 -0700
> >> Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
> >>
> >> When calling hugetlb_add_hstate() to initialize a new hugetlb size,
> >> be sure to use correct huge pages size order.
> >>
> >> Signed-off-by: Mike Kravetz
> >> ---
> >> arch/arm64/mm/hugetlbpage.c | 8
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> >> index 9ca840527296..a02411a1f19a 100644
> >> --- a/arch/arm64/mm/hugetlbpage.c
> >> +++ b/arch/arm64/mm/hugetlbpage.c
> >> @@ -453,11 +453,11 @@ void huge_ptep_clear_flush(struct vm_area_struct
> >> *vma,
> >> static int __init hugetlbpage_init(void)
> >> {
> >> #ifdef CONFIG_ARM64_4K_PAGES
> >> - hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
> >> #endif
> >> - hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
> >> - hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
> >> - hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
> >>
> >> return 0;
> >> }
> >
> > I build this for an arm64 kernel and ran it in qemu and it worked.
>
> Thanks for testing Anders!
>
> Will, here is an updated version of the patch based on your suggestion.
> I added the () for emphasis but that may just be noise for some. Also,
> the naming differences and values for CONT_PTE may make some people
> look twice. Not sure if being consistent here helps?
Cheers, thanks for this. I think being consistent is worthwhile, as it's
the definitions themselves that are weird and we can conceivably clean
that up as a separate patch.
So,
Acked-by: Will Deacon
Looks like Andrew already picked it up (thanks!)
Thanks,
Will
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On Mon, 20 Apr 2020 at 23:43, Mike Kravetz wrote:
>
> On 4/20/20 1:29 PM, Anders Roxell wrote:
> > On Mon, 20 Apr 2020 at 20:23, Mike Kravetz wrote:
> >> On 4/20/20 8:34 AM, Qian Cai wrote:
> >>>
> >>> Reverted this series fixed many undefined behaviors on arm64 with the
> >>> config,
> >> While rearranging the code (patch 3 in series), I made the incorrect
> >> assumption that CONT_XXX_SIZE == (1UL << CONT_XXX_SHIFT). However,
> >> this is not the case. Does the following patch fix these issues?
> >>
> >> From b75cb4a0852e208bee8c4eb347dc076fcaa88859 Mon Sep 17 00:00:00 2001
> >> From: Mike Kravetz
> >> Date: Mon, 20 Apr 2020 10:41:18 -0700
> >> Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
> >>
> >> When calling hugetlb_add_hstate() to initialize a new hugetlb size,
> >> be sure to use correct huge pages size order.
> >>
> >> Signed-off-by: Mike Kravetz
> >> ---
> >> arch/arm64/mm/hugetlbpage.c | 8
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> >> index 9ca840527296..a02411a1f19a 100644
> >> --- a/arch/arm64/mm/hugetlbpage.c
> >> +++ b/arch/arm64/mm/hugetlbpage.c
> >> @@ -453,11 +453,11 @@ void huge_ptep_clear_flush(struct vm_area_struct
> >> *vma,
> >> static int __init hugetlbpage_init(void)
> >> {
> >> #ifdef CONFIG_ARM64_4K_PAGES
> >> - hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
> >> #endif
> >> - hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
> >> - hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
> >> - hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
> >>
> >> return 0;
> >> }
> >
> > I build this for an arm64 kernel and ran it in qemu and it worked.
>
> Thanks for testing Anders!
>
> Will, here is an updated version of the patch based on your suggestion.
> I added the () for emphasis but that may just be noise for some. Also,
> the naming differences and values for CONT_PTE may make some people
> look twice. Not sure if being consistent here helps?
>
> I have only built this. No testing.
>
> From daf833ab6b806ecc0816d84d45dcbacc052a7eec Mon Sep 17 00:00:00 2001
> From: Mike Kravetz
> Date: Mon, 20 Apr 2020 13:56:15 -0700
> Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
>
> When calling hugetlb_add_hstate() to initialize a new hugetlb size,
> be sure to use correct huge pages size order.
>
> Signed-off-by: Mike Kravetz
Tested-by: Anders Roxell
I tested this patch on qemu-aarch64.
Cheers,
Anders
> ---
> arch/arm64/mm/hugetlbpage.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 9ca840527296..bed6dc7c4276 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -455,9 +455,9 @@ static int __init hugetlbpage_init(void)
> #ifdef CONFIG_ARM64_4K_PAGES
> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> #endif
> - hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate((CONT_PMD_SHIFT + PMD_SHIFT) - PAGE_SHIFT);
> hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
> - hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate((CONT_PTE_SHIFT + PAGE_SHIFT) - PAGE_SHIFT);
>
> return 0;
> }
> --
> 2.25.2
>
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On 4/20/20 1:29 PM, Anders Roxell wrote:
> On Mon, 20 Apr 2020 at 20:23, Mike Kravetz wrote:
>> On 4/20/20 8:34 AM, Qian Cai wrote:
>>>
>>> Reverted this series fixed many undefined behaviors on arm64 with the
>>> config,
>> While rearranging the code (patch 3 in series), I made the incorrect
>> assumption that CONT_XXX_SIZE == (1UL << CONT_XXX_SHIFT). However,
>> this is not the case. Does the following patch fix these issues?
>>
>> From b75cb4a0852e208bee8c4eb347dc076fcaa88859 Mon Sep 17 00:00:00 2001
>> From: Mike Kravetz
>> Date: Mon, 20 Apr 2020 10:41:18 -0700
>> Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
>>
>> When calling hugetlb_add_hstate() to initialize a new hugetlb size,
>> be sure to use correct huge pages size order.
>>
>> Signed-off-by: Mike Kravetz
>> ---
>> arch/arm64/mm/hugetlbpage.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index 9ca840527296..a02411a1f19a 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -453,11 +453,11 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
>> static int __init hugetlbpage_init(void)
>> {
>> #ifdef CONFIG_ARM64_4K_PAGES
>> - hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
>> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
>> #endif
>> - hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
>> - hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
>> - hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
>> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
>> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
>> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
>>
>> return 0;
>> }
>
> I build this for an arm64 kernel and ran it in qemu and it worked.
Thanks for testing Anders!
Will, here is an updated version of the patch based on your suggestion.
I added the () for emphasis but that may just be noise for some. Also,
the naming differences and values for CONT_PTE may make some people
look twice. Not sure if being consistent here helps?
I have only built this. No testing.
>From daf833ab6b806ecc0816d84d45dcbacc052a7eec Mon Sep 17 00:00:00 2001
From: Mike Kravetz
Date: Mon, 20 Apr 2020 13:56:15 -0700
Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
When calling hugetlb_add_hstate() to initialize a new hugetlb size,
be sure to use correct huge pages size order.
Signed-off-by: Mike Kravetz
---
arch/arm64/mm/hugetlbpage.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 9ca840527296..bed6dc7c4276 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -455,9 +455,9 @@ static int __init hugetlbpage_init(void)
#ifdef CONFIG_ARM64_4K_PAGES
hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
#endif
- hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
+ hugetlb_add_hstate((CONT_PMD_SHIFT + PMD_SHIFT) - PAGE_SHIFT);
hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
- hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
+ hugetlb_add_hstate((CONT_PTE_SHIFT + PAGE_SHIFT) - PAGE_SHIFT);
return 0;
}
--
2.25.2
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On Mon, 20 Apr 2020 at 20:23, Mike Kravetz wrote:
>
> On 4/20/20 8:34 AM, Qian Cai wrote:
> >
> >
> >> On Apr 17, 2020, at 2:50 PM, Mike Kravetz wrote:
> >>
> >> Longpeng(Mike) reported a weird message from hugetlb command line
> >> processing
> >> and proposed a solution [1]. While the proposed patch does address the
> >> specific issue, there are other related issues in command line processing.
> >> As hugetlbfs evolved, updates to command line processing have been made to
> >> meet immediate needs and not necessarily in a coordinated manner. The
> >> result
> >> is that some processing is done in arch specific code, some is done in arch
> >> independent code and coordination is problematic. Semantics can vary
> >> between
> >> architectures.
> >>
> >> The patch series does the following:
> >> - Define arch specific arch_hugetlb_valid_size routine used to validate
> >> passed huge page sizes.
> >> - Move hugepagesz= command line parsing out of arch specific code and into
> >> an arch independent routine.
> >> - Clean up command line processing to follow desired semantics and
> >> document those semantics.
> >>
> >> [1]
> >> https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpe...@huawei.com
> >>
> >> Mike Kravetz (4):
> >> hugetlbfs: add arch_hugetlb_valid_size
> >> hugetlbfs: move hugepagesz= parsing to arch independent code
> >> hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
> >> hugetlbfs: clean up command line processing
> >
> > Reverted this series fixed many undefined behaviors on arm64 with the
> > config,
> >
> > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config
> >
> > [ 54.172683][T1] UBSAN: shift-out-of-bounds in
> > ./include/linux/hugetlb.h:555:34
> > [ 54.180411][T1] shift exponent 4294967285 is too large for 64-bit
> > type 'unsigned long'
> > [ 54.15][T1] CPU: 130 PID: 1 Comm: swapper/0 Not tainted
> > 5.7.0-rc2-next-20200420 #1
> > [ 54.197284][T1] Hardware name: HPE Apollo 70
> > /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019
> > [ 54.207888][T1] Call trace:
> > [ 54.211100][T1] dump_backtrace+0x0/0x224
> > [ 54.215565][T1] show_stack+0x20/0x2c
> > [ 54.219651][T1] dump_stack+0xfc/0x184
> > [ 54.223829][T1] __ubsan_handle_shift_out_of_bounds+0x304/0x344
> > [ 54.230204][T1] hugetlb_add_hstate+0x3ec/0x414
> > huge_page_size at include/linux/hugetlb.h:555
> > (inlined by) hugetlb_add_hstate at mm/hugetlb.c:3301
> > [ 54.235191][T1] hugetlbpage_init+0x14/0x30
> > [ 54.239824][T1] do_one_initcall+0x6c/0x144
> > [ 54.26][T1] do_initcall_level+0x158/0x1c4
> > [ 54.249336][T1] do_initcalls+0x68/0xb0
> > [ 54.253597][T1] do_basic_setup+0x28/0x30
> > [ 54.258049][T1] kernel_init_freeable+0x19c/0x228
> > [ 54.263188][T1] kernel_init+0x14/0x208
> > [ 54.267473][T1] ret_from_fork+0x10/0x18
>
> While rearranging the code (patch 3 in series), I made the incorrect
> assumption that CONT_XXX_SIZE == (1UL << CONT_XXX_SHIFT). However,
> this is not the case. Does the following patch fix these issues?
>
> From b75cb4a0852e208bee8c4eb347dc076fcaa88859 Mon Sep 17 00:00:00 2001
> From: Mike Kravetz
> Date: Mon, 20 Apr 2020 10:41:18 -0700
> Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
>
> When calling hugetlb_add_hstate() to initialize a new hugetlb size,
> be sure to use correct huge pages size order.
>
> Signed-off-by: Mike Kravetz
> ---
> arch/arm64/mm/hugetlbpage.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 9ca840527296..a02411a1f19a 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -453,11 +453,11 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
> static int __init hugetlbpage_init(void)
> {
> #ifdef CONFIG_ARM64_4K_PAGES
> - hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
> #endif
> - hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
> - hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
> - hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
>
> return 0;
> }
I build this for an arm64 kernel and ran it in qemu and it worked.
Cheers,
Anders
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On Mon, Apr 20, 2020 at 11:20:23AM -0700, Mike Kravetz wrote:
> On 4/20/20 8:34 AM, Qian Cai wrote:
> >
> >
> >> On Apr 17, 2020, at 2:50 PM, Mike Kravetz wrote:
> >>
> >> Longpeng(Mike) reported a weird message from hugetlb command line
> >> processing
> >> and proposed a solution [1]. While the proposed patch does address the
> >> specific issue, there are other related issues in command line processing.
> >> As hugetlbfs evolved, updates to command line processing have been made to
> >> meet immediate needs and not necessarily in a coordinated manner. The
> >> result
> >> is that some processing is done in arch specific code, some is done in arch
> >> independent code and coordination is problematic. Semantics can vary
> >> between
> >> architectures.
> >>
> >> The patch series does the following:
> >> - Define arch specific arch_hugetlb_valid_size routine used to validate
> >> passed huge page sizes.
> >> - Move hugepagesz= command line parsing out of arch specific code and into
> >> an arch independent routine.
> >> - Clean up command line processing to follow desired semantics and
> >> document those semantics.
> >>
> >> [1]
> >> https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpe...@huawei.com
> >>
> >> Mike Kravetz (4):
> >> hugetlbfs: add arch_hugetlb_valid_size
> >> hugetlbfs: move hugepagesz= parsing to arch independent code
> >> hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
> >> hugetlbfs: clean up command line processing
> >
> > Reverted this series fixed many undefined behaviors on arm64 with the
> > config,
> >
> > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config
> >
> > [ 54.172683][T1] UBSAN: shift-out-of-bounds in
> > ./include/linux/hugetlb.h:555:34
> > [ 54.180411][T1] shift exponent 4294967285 is too large for 64-bit
> > type 'unsigned long'
> > [ 54.15][T1] CPU: 130 PID: 1 Comm: swapper/0 Not tainted
> > 5.7.0-rc2-next-20200420 #1
> > [ 54.197284][T1] Hardware name: HPE Apollo 70
> > /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019
> > [ 54.207888][T1] Call trace:
> > [ 54.211100][T1] dump_backtrace+0x0/0x224
> > [ 54.215565][T1] show_stack+0x20/0x2c
> > [ 54.219651][T1] dump_stack+0xfc/0x184
> > [ 54.223829][T1] __ubsan_handle_shift_out_of_bounds+0x304/0x344
> > [ 54.230204][T1] hugetlb_add_hstate+0x3ec/0x414
> > huge_page_size at include/linux/hugetlb.h:555
> > (inlined by) hugetlb_add_hstate at mm/hugetlb.c:3301
> > [ 54.235191][T1] hugetlbpage_init+0x14/0x30
> > [ 54.239824][T1] do_one_initcall+0x6c/0x144
> > [ 54.26][T1] do_initcall_level+0x158/0x1c4
> > [ 54.249336][T1] do_initcalls+0x68/0xb0
> > [ 54.253597][T1] do_basic_setup+0x28/0x30
> > [ 54.258049][T1] kernel_init_freeable+0x19c/0x228
> > [ 54.263188][T1] kernel_init+0x14/0x208
> > [ 54.267473][T1] ret_from_fork+0x10/0x18
>
> While rearranging the code (patch 3 in series), I made the incorrect
> assumption that CONT_XXX_SIZE == (1UL << CONT_XXX_SHIFT). However,
> this is not the case. Does the following patch fix these issues?
>
> From b75cb4a0852e208bee8c4eb347dc076fcaa88859 Mon Sep 17 00:00:00 2001
> From: Mike Kravetz
> Date: Mon, 20 Apr 2020 10:41:18 -0700
> Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
>
> When calling hugetlb_add_hstate() to initialize a new hugetlb size,
> be sure to use correct huge pages size order.
>
> Signed-off-by: Mike Kravetz
> ---
> arch/arm64/mm/hugetlbpage.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 9ca840527296..a02411a1f19a 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -453,11 +453,11 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
> static int __init hugetlbpage_init(void)
> {
> #ifdef CONFIG_ARM64_4K_PAGES
> - hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
> #endif
> - hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
> - hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
> - hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
Might be clearer to leave the non CONT_* definitions alone and instead
convert the CONT versions along the lines of:
hugetlb_add_hstate(CONT_PMD_SHIFT + PMD_SHIFT - PAGE_SHIFT);
(untested, but I think that's right...)
But that doesn't matter until Qian has confirmed that your patch fixes the
issue.
Will
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
On 4/20/20 8:34 AM, Qian Cai wrote:
>
>
>> On Apr 17, 2020, at 2:50 PM, Mike Kravetz wrote:
>>
>> Longpeng(Mike) reported a weird message from hugetlb command line processing
>> and proposed a solution [1]. While the proposed patch does address the
>> specific issue, there are other related issues in command line processing.
>> As hugetlbfs evolved, updates to command line processing have been made to
>> meet immediate needs and not necessarily in a coordinated manner. The result
>> is that some processing is done in arch specific code, some is done in arch
>> independent code and coordination is problematic. Semantics can vary between
>> architectures.
>>
>> The patch series does the following:
>> - Define arch specific arch_hugetlb_valid_size routine used to validate
>> passed huge page sizes.
>> - Move hugepagesz= command line parsing out of arch specific code and into
>> an arch independent routine.
>> - Clean up command line processing to follow desired semantics and
>> document those semantics.
>>
>> [1]
>> https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpe...@huawei.com
>>
>> Mike Kravetz (4):
>> hugetlbfs: add arch_hugetlb_valid_size
>> hugetlbfs: move hugepagesz= parsing to arch independent code
>> hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
>> hugetlbfs: clean up command line processing
>
> Reverted this series fixed many undefined behaviors on arm64 with the config,
>
> https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config
>
> [ 54.172683][T1] UBSAN: shift-out-of-bounds in
> ./include/linux/hugetlb.h:555:34
> [ 54.180411][T1] shift exponent 4294967285 is too large for 64-bit type
> 'unsigned long'
> [ 54.15][T1] CPU: 130 PID: 1 Comm: swapper/0 Not tainted
> 5.7.0-rc2-next-20200420 #1
> [ 54.197284][T1] Hardware name: HPE Apollo 70
> /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019
> [ 54.207888][T1] Call trace:
> [ 54.211100][T1] dump_backtrace+0x0/0x224
> [ 54.215565][T1] show_stack+0x20/0x2c
> [ 54.219651][T1] dump_stack+0xfc/0x184
> [ 54.223829][T1] __ubsan_handle_shift_out_of_bounds+0x304/0x344
> [ 54.230204][T1] hugetlb_add_hstate+0x3ec/0x414
> huge_page_size at include/linux/hugetlb.h:555
> (inlined by) hugetlb_add_hstate at mm/hugetlb.c:3301
> [ 54.235191][T1] hugetlbpage_init+0x14/0x30
> [ 54.239824][T1] do_one_initcall+0x6c/0x144
> [ 54.26][T1] do_initcall_level+0x158/0x1c4
> [ 54.249336][T1] do_initcalls+0x68/0xb0
> [ 54.253597][T1] do_basic_setup+0x28/0x30
> [ 54.258049][T1] kernel_init_freeable+0x19c/0x228
> [ 54.263188][T1] kernel_init+0x14/0x208
> [ 54.267473][T1] ret_from_fork+0x10/0x18
While rearranging the code (patch 3 in series), I made the incorrect
assumption that CONT_XXX_SIZE == (1UL << CONT_XXX_SHIFT). However,
this is not the case. Does the following patch fix these issues?
>From b75cb4a0852e208bee8c4eb347dc076fcaa88859 Mon Sep 17 00:00:00 2001
From: Mike Kravetz
Date: Mon, 20 Apr 2020 10:41:18 -0700
Subject: [PATCH] arm64/hugetlb: fix hugetlb initialization
When calling hugetlb_add_hstate() to initialize a new hugetlb size,
be sure to use correct huge pages size order.
Signed-off-by: Mike Kravetz
---
arch/arm64/mm/hugetlbpage.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 9ca840527296..a02411a1f19a 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -453,11 +453,11 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
static int __init hugetlbpage_init(void)
{
#ifdef CONFIG_ARM64_4K_PAGES
- hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
+ hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
#endif
- hugetlb_add_hstate(CONT_PMD_SHIFT - PAGE_SHIFT);
- hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
- hugetlb_add_hstate(CONT_PTE_SHIFT - PAGE_SHIFT);
+ hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
+ hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
+ hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
return 0;
}
--
2.25.2
Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
> On Apr 17, 2020, at 2:50 PM, Mike Kravetz wrote: > > Longpeng(Mike) reported a weird message from hugetlb command line processing > and proposed a solution [1]. While the proposed patch does address the > specific issue, there are other related issues in command line processing. > As hugetlbfs evolved, updates to command line processing have been made to > meet immediate needs and not necessarily in a coordinated manner. The result > is that some processing is done in arch specific code, some is done in arch > independent code and coordination is problematic. Semantics can vary between > architectures. > > The patch series does the following: > - Define arch specific arch_hugetlb_valid_size routine used to validate > passed huge page sizes. > - Move hugepagesz= command line parsing out of arch specific code and into > an arch independent routine. > - Clean up command line processing to follow desired semantics and > document those semantics. > > [1] > https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpe...@huawei.com > > Mike Kravetz (4): > hugetlbfs: add arch_hugetlb_valid_size > hugetlbfs: move hugepagesz= parsing to arch independent code > hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate > hugetlbfs: clean up command line processing Reverted this series fixed many undefined behaviors on arm64 with the config, https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config [ 54.172683][T1] UBSAN: shift-out-of-bounds in ./include/linux/hugetlb.h:555:34 [ 54.180411][T1] shift exponent 4294967285 is too large for 64-bit type 'unsigned long' [ 54.15][T1] CPU: 130 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc2-next-20200420 #1 [ 54.197284][T1] Hardware name: HPE Apollo 70 /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019 [ 54.207888][T1] Call trace: [ 54.211100][T1] dump_backtrace+0x0/0x224 [ 54.215565][T1] show_stack+0x20/0x2c [ 54.219651][T1] dump_stack+0xfc/0x184 [ 54.223829][T1] __ubsan_handle_shift_out_of_bounds+0x304/0x344 [ 54.230204][T1] hugetlb_add_hstate+0x3ec/0x414 huge_page_size at include/linux/hugetlb.h:555 (inlined by) hugetlb_add_hstate at mm/hugetlb.c:3301 [ 54.235191][T1] hugetlbpage_init+0x14/0x30 [ 54.239824][T1] do_one_initcall+0x6c/0x144 [ 54.26][T1] do_initcall_level+0x158/0x1c4 [ 54.249336][T1] do_initcalls+0x68/0xb0 [ 54.253597][T1] do_basic_setup+0x28/0x30 [ 54.258049][T1] kernel_init_freeable+0x19c/0x228 [ 54.263188][T1] kernel_init+0x14/0x208 [ 54.267473][T1] ret_from_fork+0x10/0x18 [ 55.534338][T1] UBSAN: shift-out-of-bounds in ./include/linux/hugetlb.h:555:34 [ 55.542064][T1] shift exponent 4294967285 is too large for 64-bit type 'unsigned long' [ 55.550555][T1] CPU: 129 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc2-next-20200420 #1 [ 55.558992][T1] Hardware name: HPE Apollo 70 /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019 [ 55.569659][T1] Call trace: [ 55.572898][T1] dump_backtrace+0x0/0x224 [ 55.577335][T1] show_stack+0x20/0x2c [ 55.581442][T1] dump_stack+0xfc/0x184 [ 55.585621][T1] __ubsan_handle_shift_out_of_bounds+0x304/0x344 [ 55.592031][T1] __hugetlb_cgroup_file_dfl_init+0x37c/0x384 [ 55.598062][T1] hugetlb_cgroup_file_init+0x9c/0xd8 [ 55.603399][T1] hugetlb_init+0x248/0x448 [ 55.607840][T1] do_one_initcall+0x6c/0x144 [ 55.612493][T1] do_initcall_level+0x158/0x1c4 [ 55.617404][T1] do_initcalls+0x68/0xb0 [ 55.621664][T1] do_basic_setup+0x28/0x30 [ 55.626107][T1] kernel_init_freeable+0x19c/0x228 [ 55.631253][T1] kernel_init+0x14/0x208 [ 55.635519][T1] ret_from_fork+0x10/0x18 [ 153.283648][T1] [ 153.293078][T1] UBSAN: shift-out-of-bounds in ./include/linux/hugetlb.h:555:34 [ 153.300841][T1] shift exponent 4294967285 is too large for 64-bit type 'unsigned long' [ 153.309185][T1] CPU: 161 PID: 1 Comm: swapper/0 Tainted: G L 5.7.0-rc2-next-20200420 #1 [ 153.318879][T1] Hardware name: HPE Apollo 70 /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019 [ 153.329352][T1] Call trace: [ 153.332545][T1] dump_backtrace+0x0/0x224 [ 153.336945][T1] show_stack+0x20/0x2c [ 153.341000][T1] dump_stack+0xfc/0x184 [ 153.345149][T1] __ubsan_handle_shift_out_of_bounds+0x304/0x344 [ 153.351465][T1] hugetlbfs_fill_super+0x424/0x43c [ 153.356560][T1] vfs_get_super+0xcc/0x170 [ 153.360959][T1] get_tree_nodev+0x28/0x34 [ 153.365358][T1] hugetlbfs_get_tree+0xfc/0x128 [ 153.370193][T1] vfs_get_tree+0x54/0x158 [ 153.374513][T1] fc_mount+0x1c/0x5c [ 153.378399][T1] mount_one_hugetlbfs+0x54/0xc8 [ 153.383233][T1]
[PATCH v3 0/4] Clean up hugetlb boot command line processing
v3 - Used weak attribute method of defining arch_hugetlb_valid_size. This eliminates changes to arch specific hugetlb.h files (Peter) Updated documentation (Peter, Randy) Fixed handling of implicitly specified gigantic page preallocation in existing code and removed documentation of such. There is now no difference between handling of gigantic and non-gigantic pages. (Peter, Nitesh). This requires the most review as there is a small change to undocumented behavior. See patch 4 commit message for details. Added Acks and Reviews (Mina, Peter) v2 - Fix build errors with patch 1 (Will) Change arch_hugetlb_valid_size arg to unsigned long and remove irrelevant 'extern' keyword (Christophe) Documentation and other misc changes (Randy, Christophe, Mina) Do not process command line options if !hugepages_supported() (Dave, but it sounds like we may want to additional changes to hugepages_supported() for x86? If that is needed I would prefer a separate patch.) Longpeng(Mike) reported a weird message from hugetlb command line processing and proposed a solution [1]. While the proposed patch does address the specific issue, there are other related issues in command line processing. As hugetlbfs evolved, updates to command line processing have been made to meet immediate needs and not necessarily in a coordinated manner. The result is that some processing is done in arch specific code, some is done in arch independent code and coordination is problematic. Semantics can vary between architectures. The patch series does the following: - Define arch specific arch_hugetlb_valid_size routine used to validate passed huge page sizes. - Move hugepagesz= command line parsing out of arch specific code and into an arch independent routine. - Clean up command line processing to follow desired semantics and document those semantics. [1] https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpe...@huawei.com Mike Kravetz (4): hugetlbfs: add arch_hugetlb_valid_size hugetlbfs: move hugepagesz= parsing to arch independent code hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate hugetlbfs: clean up command line processing .../admin-guide/kernel-parameters.txt | 40 ++-- Documentation/admin-guide/mm/hugetlbpage.rst | 35 arch/arm64/mm/hugetlbpage.c | 30 +-- arch/powerpc/mm/hugetlbpage.c | 30 +-- arch/riscv/mm/hugetlbpage.c | 24 +-- arch/s390/mm/hugetlbpage.c| 24 +-- arch/sparc/mm/init_64.c | 43 +--- arch/x86/mm/hugetlbpage.c | 23 +-- include/linux/hugetlb.h | 2 +- mm/hugetlb.c | 190 +++--- 10 files changed, 271 insertions(+), 170 deletions(-) -- 2.25.2
