| Message ID | 20200417185049.275845-3-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up hugetlb boot command line processing | expand |
Hi Mike, On 18/04/20 12:20 am, Mike Kravetz wrote: > Now that architectures provide arch_hugetlb_valid_size(), parsing > of "hugepagesz=" can be done in architecture independent code. > Create a single routine to handle hugepagesz= parsing and remove > all arch specific routines. We can also remove the interface > hugetlb_bad_size() as this is no longer used outside arch independent > code. > > This also provides consistent behavior of hugetlbfs command line > options. The hugepagesz= option should only be specified once for > a specific size, but some architectures allow multiple instances. > This appears to be more of an oversight when code was added by some > architectures to set up ALL huge pages sizes. > > [...] > > diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c > index de54d2a37830..2c3fa0a7787b 100644 > --- a/arch/powerpc/mm/hugetlbpage.c > +++ b/arch/powerpc/mm/hugetlbpage.c > @@ -589,21 +589,6 @@ static int __init add_huge_page_size(unsigned long long size) > return 0; > } > > -static int __init hugepage_setup_sz(char *str) > -{ > - unsigned long long size; > - > - size = memparse(str, &str); > - > - if (add_huge_page_size(size) != 0) { > - hugetlb_bad_size(); > - pr_err("Invalid huge page size specified(%llu)\n", size); > - } > - > - return 1; > -} > -__setup("hugepagesz=", hugepage_setup_sz); > - > [...] This isn't working as expected on powerpc64. [ 0.000000] Kernel command line: root=UUID=dc7b49cf-95a2-4996-8e7d-7c64ddc7a6ff hugepagesz=16G hugepages=2 [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepagesz = 16G [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepages = 2 [ 0.284177] HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages [ 0.284182] HugeTLB registered 16.0 GiB page size, pre-allocated 0 pages [ 2.585062] hugepagesz=16G [ 2.585063] hugepages=2 The "huge pages not supported" messages are under a !hugepages_supported() condition which checks if HPAGE_SHIFT is non-zero. On powerpc64, HPAGE_SHIFT comes from the hpage_shift variable. At this point, it is still zero and yet to be set. Hence the check fails. The reason being hugetlbpage_init_default(), which sets hpage_shift, it now called after hugepage_setup_sz(). - Sandipan
On 4/26/20 10:04 PM, Sandipan Das wrote: > Hi Mike, > > On 18/04/20 12:20 am, Mike Kravetz wrote: >> Now that architectures provide arch_hugetlb_valid_size(), parsing >> of "hugepagesz=" can be done in architecture independent code. >> Create a single routine to handle hugepagesz= parsing and remove >> all arch specific routines. We can also remove the interface >> hugetlb_bad_size() as this is no longer used outside arch independent >> code. >> >> This also provides consistent behavior of hugetlbfs command line >> options. The hugepagesz= option should only be specified once for >> a specific size, but some architectures allow multiple instances. >> This appears to be more of an oversight when code was added by some >> architectures to set up ALL huge pages sizes. >> >> [...] >> >> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c >> index de54d2a37830..2c3fa0a7787b 100644 >> --- a/arch/powerpc/mm/hugetlbpage.c >> +++ b/arch/powerpc/mm/hugetlbpage.c >> @@ -589,21 +589,6 @@ static int __init add_huge_page_size(unsigned long long size) >> return 0; >> } >> >> -static int __init hugepage_setup_sz(char *str) >> -{ >> - unsigned long long size; >> - >> - size = memparse(str, &str); >> - >> - if (add_huge_page_size(size) != 0) { >> - hugetlb_bad_size(); >> - pr_err("Invalid huge page size specified(%llu)\n", size); >> - } >> - >> - return 1; >> -} >> -__setup("hugepagesz=", hugepage_setup_sz); >> - >> [...] > > This isn't working as expected on powerpc64. > > [ 0.000000] Kernel command line: root=UUID=dc7b49cf-95a2-4996-8e7d-7c64ddc7a6ff hugepagesz=16G hugepages=2 > [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepagesz = 16G > [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepages = 2 > [ 0.284177] HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages > [ 0.284182] HugeTLB registered 16.0 GiB page size, pre-allocated 0 pages > [ 2.585062] hugepagesz=16G > [ 2.585063] hugepages=2 > > The "huge pages not supported" messages are under a !hugepages_supported() > condition which checks if HPAGE_SHIFT is non-zero. On powerpc64, HPAGE_SHIFT > comes from the hpage_shift variable. At this point, it is still zero and yet > to be set. Hence the check fails. The reason being hugetlbpage_init_default(), > which sets hpage_shift, it now called after hugepage_setup_sz(). Thanks for catching this Sandipan. In the new arch independent version of hugepages_setup, I added the following code in patch 4 off this series: > +static int __init hugepages_setup(char *s) > { > unsigned long *mhp; > static unsigned long *last_mhp; > > + if (!hugepages_supported()) { > + pr_warn("HugeTLB: huge pages not supported, ignoring hugepages = %s\n", s); > + return 0; > + } > + > if (!parsed_valid_hugepagesz) { In fact, I added it to the beginning of all the hugetlb command line parsing routines. My 'thought' was to warn early if hugetlb pages were not supported. Previously, the first check for hugepages_supported() was in hugetlb_init() which ran after hugetlbpage_init_default(). The easy solution is to remove all the hugepages_supported() checks from command line parsing routines and rely on the later check in hugetlb_init(). Another reason for adding those early checks was to possibly prevent the preallocation of gigantic pages at command line parsing time. Gigantic pages are allocated at command line parsing time as they need to be allocated with the bootmem allocator. My concern is that there could be some strange configuration where !hugepages_supported(), yet we allocate gigantic pages from bootmem that can not be used or freeed later. powerpc is the only architecture which has it's own alloc_bootmem_huge_page routine. So, it handles this potential issue. I'll send out a fix shortly.
On 4/27/20 10:25 AM, Mike Kravetz wrote: > On 4/26/20 10:04 PM, Sandipan Das wrote: >> On 18/04/20 12:20 am, Mike Kravetz wrote: >>> Now that architectures provide arch_hugetlb_valid_size(), parsing >>> of "hugepagesz=" can be done in architecture independent code. >> >> This isn't working as expected on powerpc64. >> >> [ 0.000000] Kernel command line: root=UUID=dc7b49cf-95a2-4996-8e7d-7c64ddc7a6ff hugepagesz=16G hugepages=2 >> [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepagesz = 16G >> [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepages = 2 >> [ 0.284177] HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages >> [ 0.284182] HugeTLB registered 16.0 GiB page size, pre-allocated 0 pages >> [ 2.585062] hugepagesz=16G >> [ 2.585063] hugepages=2 >> > > In the new arch independent version of hugepages_setup, I added the following > code in patch 4 off this series: > >> + if (!hugepages_supported()) { >> + pr_warn("HugeTLB: huge pages not supported, ignoring hugepages = %s\n", s); >> + return 0; >> + } >> + > > The easy solution is to remove all the hugepages_supported() checks from > command line parsing routines and rely on the later check in hugetlb_init(). Here is a patch to address the issue. Sorry, as my series breaks all hugetlb command line processing on powerpc. Sandipan, can you test the following patch? From 480fe2847361e2a85aeec1fb39fe643bb7100a07 Mon Sep 17 00:00:00 2001 From: Mike Kravetz <mike.kravetz@oracle.com> Date: Mon, 27 Apr 2020 11:37:30 -0700 Subject: [PATCH] hugetlbfs: fix changes to command line processing Previously, a check for hugepages_supported was added before processing hugetlb command line parameters. On some architectures such as powerpc, hugepages_supported() is not set to true until after command line processing. Therefore, no hugetlb command line parameters would be accepted. Remove the additional checks for hugepages_supported. In hugetlb_init, print a warning if !hugepages_supported and command line parameters were specified. Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- mm/hugetlb.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 1075abdb5717..5548e8851b93 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3212,8 +3212,11 @@ static int __init hugetlb_init(void) { int i; - if (!hugepages_supported()) + if (!hugepages_supported()) { + if (hugetlb_max_hstate || default_hstate_max_huge_pages) + pr_warn("HugeTLB: huge pages not supported, ignoring associated command-line parameters\n"); return 0; + } /* * Make sure HPAGE_SIZE (HUGETLB_PAGE_ORDER) hstate exists. Some @@ -3315,11 +3318,6 @@ static int __init hugepages_setup(char *s) unsigned long *mhp; static unsigned long *last_mhp; - if (!hugepages_supported()) { - pr_warn("HugeTLB: huge pages not supported, ignoring hugepages = %s\n", s); - return 0; - } - if (!parsed_valid_hugepagesz) { pr_warn("HugeTLB: hugepages=%s does not follow a valid hugepagesz, ignoring\n", s); parsed_valid_hugepagesz = true; @@ -3372,11 +3370,6 @@ static int __init hugepagesz_setup(char *s) struct hstate *h; parsed_valid_hugepagesz = false; - if (!hugepages_supported()) { - pr_warn("HugeTLB: huge pages not supported, ignoring hugepagesz = %s\n", s); - return 0; - } - size = (unsigned long)memparse(s, NULL); if (!arch_hugetlb_valid_size(size)) { @@ -3424,11 +3417,6 @@ static int __init default_hugepagesz_setup(char *s) unsigned long size; parsed_valid_hugepagesz = false; - if (!hugepages_supported()) { - pr_warn("HugeTLB: huge pages not supported, ignoring default_hugepagesz = %s\n", s); - return 0; - } - if (parsed_default_hugepagesz) { pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n", s); return 0;
On Mon, 27 Apr 2020 12:09:47 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote: > Previously, a check for hugepages_supported was added before processing > hugetlb command line parameters. On some architectures such as powerpc, > hugepages_supported() is not set to true until after command line > processing. Therefore, no hugetlb command line parameters would be > accepted. > > Remove the additional checks for hugepages_supported. In hugetlb_init, > print a warning if !hugepages_supported and command line parameters were > specified. This applies to [4/4] instead of fixing [2/4]. I guess you'll straighten that out in v4? btw, was http://lkml.kernel.org/r/CADYN=9Koefrq9H1Y82Q8nMNbeyN4tzhEfvDu5u=sVFjFZCYorA@mail.gmail.com addressed?
On 4/27/20 1:18 PM, Andrew Morton wrote: > On Mon, 27 Apr 2020 12:09:47 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote: > >> Previously, a check for hugepages_supported was added before processing >> hugetlb command line parameters. On some architectures such as powerpc, >> hugepages_supported() is not set to true until after command line >> processing. Therefore, no hugetlb command line parameters would be >> accepted. >> >> Remove the additional checks for hugepages_supported. In hugetlb_init, >> print a warning if !hugepages_supported and command line parameters were >> specified. > > This applies to [4/4] instead of fixing [2/4]. I guess you'll > straighten that out in v4? Yes. > btw, was > http://lkml.kernel.org/r/CADYN=9Koefrq9H1Y82Q8nMNbeyN4tzhEfvDu5u=sVFjFZCYorA@mail.gmail.com > addressed? Yes, you pulled a patch into your tree to address this. hugetlbfs-remove-hugetlb_add_hstate-warning-for-existing-hstate-fix.patch I'll send out a v4 with both these issues addressed. Would like to wait until receiving confirmation from someone who can test on powerpc.
Hi Mike, On 28/04/20 12:39 am, Mike Kravetz wrote: > On 4/27/20 10:25 AM, Mike Kravetz wrote: >> On 4/26/20 10:04 PM, Sandipan Das wrote: >>> On 18/04/20 12:20 am, Mike Kravetz wrote: >>>> Now that architectures provide arch_hugetlb_valid_size(), parsing >>>> of "hugepagesz=" can be done in architecture independent code. >>> >>> This isn't working as expected on powerpc64. >>> >>> [ 0.000000] Kernel command line: root=UUID=dc7b49cf-95a2-4996-8e7d-7c64ddc7a6ff hugepagesz=16G hugepages=2 >>> [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepagesz = 16G >>> [ 0.000000] HugeTLB: huge pages not supported, ignoring hugepages = 2 >>> [ 0.284177] HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages >>> [ 0.284182] HugeTLB registered 16.0 GiB page size, pre-allocated 0 pages >>> [ 2.585062] hugepagesz=16G >>> [ 2.585063] hugepages=2 >>> >> >> In the new arch independent version of hugepages_setup, I added the following >> code in patch 4 off this series: >> >>> + if (!hugepages_supported()) { >>> + pr_warn("HugeTLB: huge pages not supported, ignoring hugepages = %s\n", s); >>> + return 0; >>> + } >>> + >> >> The easy solution is to remove all the hugepages_supported() checks from >> command line parsing routines and rely on the later check in hugetlb_init(). > > Here is a patch to address the issue. Sorry, as my series breaks all hugetlb > command line processing on powerpc. > > Sandipan, can you test the following patch? > The following patch does fix the issue. Thanks. Tested-by: Sandipan Das <sandipan@linux.ibm.com> > From 480fe2847361e2a85aeec1fb39fe643bb7100a07 Mon Sep 17 00:00:00 2001 > From: Mike Kravetz <mike.kravetz@oracle.com> > Date: Mon, 27 Apr 2020 11:37:30 -0700 > Subject: [PATCH] hugetlbfs: fix changes to command line processing > > Previously, a check for hugepages_supported was added before processing > hugetlb command line parameters. On some architectures such as powerpc, > hugepages_supported() is not set to true until after command line > processing. Therefore, no hugetlb command line parameters would be > accepted. > > Remove the additional checks for hugepages_supported. In hugetlb_init, > print a warning if !hugepages_supported and command line parameters were > specified. > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > mm/hugetlb.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 1075abdb5717..5548e8851b93 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3212,8 +3212,11 @@ static int __init hugetlb_init(void) > { > int i; > > - if (!hugepages_supported()) > + if (!hugepages_supported()) { > + if (hugetlb_max_hstate || default_hstate_max_huge_pages) > + pr_warn("HugeTLB: huge pages not supported, ignoring associated command-line parameters\n"); > return 0; > + } > > /* > * Make sure HPAGE_SIZE (HUGETLB_PAGE_ORDER) hstate exists. Some > @@ -3315,11 +3318,6 @@ static int __init hugepages_setup(char *s) > unsigned long *mhp; > static unsigned long *last_mhp; > > - if (!hugepages_supported()) { > - pr_warn("HugeTLB: huge pages not supported, ignoring hugepages = %s\n", s); > - return 0; > - } > - > if (!parsed_valid_hugepagesz) { > pr_warn("HugeTLB: hugepages=%s does not follow a valid hugepagesz, ignoring\n", s); > parsed_valid_hugepagesz = true; > @@ -3372,11 +3370,6 @@ static int __init hugepagesz_setup(char *s) > struct hstate *h; > > parsed_valid_hugepagesz = false; > - if (!hugepages_supported()) { > - pr_warn("HugeTLB: huge pages not supported, ignoring hugepagesz = %s\n", s); > - return 0; > - } > - > size = (unsigned long)memparse(s, NULL); > > if (!arch_hugetlb_valid_size(size)) { > @@ -3424,11 +3417,6 @@ static int __init default_hugepagesz_setup(char *s) > unsigned long size; > > parsed_valid_hugepagesz = false; > - if (!hugepages_supported()) { > - pr_warn("HugeTLB: huge pages not supported, ignoring default_hugepagesz = %s\n", s); > - return 0; > - } > - > if (parsed_default_hugepagesz) { > pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n", s); > return 0; >
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c index 069b96ee2aec..f706b821aba6 100644 --- a/arch/arm64/mm/hugetlbpage.c +++ b/arch/arm64/mm/hugetlbpage.c @@ -476,18 +476,3 @@ bool __init arch_hugetlb_valid_size(unsigned long size) return false; } - -static __init int setup_hugepagesz(char *opt) -{ - unsigned long ps = memparse(opt, &opt); - - if (arch_hugetlb_valid_size(ps)) { - add_huge_page_size(ps); - return 1; - } - - hugetlb_bad_size(); - pr_err("hugepagesz: Unsupported page size %lu K\n", ps >> 10); - return 0; -} -__setup("hugepagesz=", setup_hugepagesz); diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c index de54d2a37830..2c3fa0a7787b 100644 --- a/arch/powerpc/mm/hugetlbpage.c +++ b/arch/powerpc/mm/hugetlbpage.c @@ -589,21 +589,6 @@ static int __init add_huge_page_size(unsigned long long size) return 0; } -static int __init hugepage_setup_sz(char *str) -{ - unsigned long long size; - - size = memparse(str, &str); - - if (add_huge_page_size(size) != 0) { - hugetlb_bad_size(); - pr_err("Invalid huge page size specified(%llu)\n", size); - } - - return 1; -} -__setup("hugepagesz=", hugepage_setup_sz); - static int __init hugetlbpage_init(void) { bool configured = false; diff --git a/arch/riscv/mm/hugetlbpage.c b/arch/riscv/mm/hugetlbpage.c index da1f516bc451..4e5d7e9f0eef 100644 --- a/arch/riscv/mm/hugetlbpage.c +++ b/arch/riscv/mm/hugetlbpage.c @@ -22,22 +22,6 @@ bool __init arch_hugetlb_valid_size(unsigned long size) return false; } -static __init int setup_hugepagesz(char *opt) -{ - unsigned long ps = memparse(opt, &opt); - - if (arch_hugetlb_valid_size(ps)) { - hugetlb_add_hstate(ilog2(ps) - PAGE_SHIFT); - return 1; - } - - hugetlb_bad_size(); - pr_err("hugepagesz: Unsupported page size %lu M\n", ps >> 20); - return 0; - -} -__setup("hugepagesz=", setup_hugepagesz); - #ifdef CONFIG_CONTIG_ALLOC static __init int gigantic_pages_init(void) { diff --git a/arch/s390/mm/hugetlbpage.c b/arch/s390/mm/hugetlbpage.c index ac25b207624c..242dfc0d462d 100644 --- a/arch/s390/mm/hugetlbpage.c +++ b/arch/s390/mm/hugetlbpage.c @@ -261,24 +261,6 @@ bool __init arch_hugetlb_valid_size(unsigned long size) return false; } -static __init int setup_hugepagesz(char *opt) -{ - unsigned long size; - char *string = opt; - - size = memparse(opt, &opt); - if (arch_hugetlb_valid_size(size)) { - hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT); - } else { - hugetlb_bad_size(); - pr_err("hugepagesz= specifies an unsupported page size %s\n", - string); - return 0; - } - return 1; -} -__setup("hugepagesz=", setup_hugepagesz); - static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *file, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags) diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c index 2bfe8e22b706..4618f96fd30f 100644 --- a/arch/sparc/mm/init_64.c +++ b/arch/sparc/mm/init_64.c @@ -397,28 +397,6 @@ bool __init arch_hugetlb_valid_size(unsigned long size) return true; } - -static int __init setup_hugepagesz(char *string) -{ - unsigned long long hugepage_size; - int rc = 0; - - hugepage_size = memparse(string, &string); - - if (!arch_hugetlb_valid_size((unsigned long)hugepage_size)) { - hugetlb_bad_size(); - pr_err("hugepagesz=%llu not supported by MMU.\n", - hugepage_size); - goto out; - } - - add_huge_page_size(hugepage_size); - rc = 1; - -out: - return rc; -} -__setup("hugepagesz=", setup_hugepagesz); #endif /* CONFIG_HUGETLB_PAGE */ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t *ptep) diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c index 1c4372bfe782..937d640a89e3 100644 --- a/arch/x86/mm/hugetlbpage.c +++ b/arch/x86/mm/hugetlbpage.c @@ -191,22 +191,6 @@ bool __init arch_hugetlb_valid_size(unsigned long size) return false; } -static __init int setup_hugepagesz(char *opt) -{ - unsigned long ps = memparse(opt, &opt); - - if (arch_hugetlb_valid_size(ps)) { - hugetlb_add_hstate(ilog2(ps) - PAGE_SHIFT); - } else { - hugetlb_bad_size(); - printk(KERN_ERR "hugepagesz: Unsupported page size %lu M\n", - ps >> 20); - return 0; - } - return 1; -} -__setup("hugepagesz=", setup_hugepagesz); - #ifdef CONFIG_CONTIG_ALLOC static __init int gigantic_pages_init(void) { diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 2eb15f5ab01e..0c13706054ef 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -519,7 +519,6 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping, int __init __alloc_bootmem_huge_page(struct hstate *h); int __init alloc_bootmem_huge_page(struct hstate *h); -void __init hugetlb_bad_size(void); void __init hugetlb_add_hstate(unsigned order); bool __init arch_hugetlb_valid_size(unsigned long size); struct hstate *size_to_hstate(unsigned long size); diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 871ac1681f37..b2d276408cec 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3262,12 +3262,6 @@ bool __init __attribute((weak)) arch_hugetlb_valid_size(unsigned long size) return size == HPAGE_SIZE; } -/* Should be called on processing a hugepagesz=... option */ -void __init hugetlb_bad_size(void) -{ - parsed_valid_hugepagesz = false; -} - void __init hugetlb_add_hstate(unsigned int order) { struct hstate *h; @@ -3337,6 +3331,23 @@ static int __init hugetlb_nrpages_setup(char *s) } __setup("hugepages=", hugetlb_nrpages_setup); +static int __init hugepagesz_setup(char *s) +{ + unsigned long size; + + size = (unsigned long)memparse(s, NULL); + + if (!arch_hugetlb_valid_size(size)) { + parsed_valid_hugepagesz = false; + pr_err("HugeTLB: unsupported hugepagesz %s\n", s); + return 0; + } + + hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT); + return 1; +} +__setup("hugepagesz=", hugepagesz_setup); + static int __init default_hugepagesz_setup(char *s) { unsigned long size;