Messages in this thread | | | Date | Mon, 14 Aug 2017 19:12:28 -0300 | From | Victor Aoqui <> | Subject | Re: [PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc |
| |
Em 2017-08-04 15:17, Mike Kravetz escreveu: > On 07/24/2017 04:52 PM, Victor Aoqui wrote: >> Implemented default hugepage size verification (default_hugepagesz=) >> in order to allow allocation of defined number of pages (hugepages=) >> only for supported hugepage sizes. >> >> Signed-off-by: Victor Aoqui <victora@linux.vnet.ibm.com> >> --- >> v2: >> >> - Renamed default_hugepage_setup_sz function to >> hugetlb_default_size_setup; >> - Added powerpc string to error message. >> >> v3: >> >> - Renamed hugetlb_default_size_setup() to hugepage_default_setup_sz(); >> - Implemented hugetlb_bad_default_size(); >> - Reimplemented hugepage_setup_sz() to just parse default_hugepagesz= >> and >> check if it's a supported size; >> - Added verification of default_hugepagesz= value on >> hugetlb_nrpages_setup() >> before allocating hugepages. >> >> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++ >> include/linux/hugetlb.h | 1 + >> mm/hugetlb.c | 17 +++++++++++++++-- >> 3 files changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/mm/hugetlbpage.c >> b/arch/powerpc/mm/hugetlbpage.c >> index e1bf5ca..5990381 100644 >> --- a/arch/powerpc/mm/hugetlbpage.c >> +++ b/arch/powerpc/mm/hugetlbpage.c >> @@ -780,6 +780,21 @@ static int __init hugepage_setup_sz(char *str) >> } >> __setup("hugepagesz=", hugepage_setup_sz); >> >> +static int __init hugepage_default_setup_sz(char *str) >> +{ >> + unsigned long long size; >> + >> + size = memparse(str, &str); >> + >> + if (add_huge_page_size(size) != 0) { >> + hugetlb_bad_default_size(); >> + pr_err("Invalid ppc default huge page size specified(%llu)\n", >> size); >> + } >> + >> + return 1; >> +} >> +__setup("default_hugepagesz=", hugepage_default_setup_sz); >> + >> struct kmem_cache *hugepte_cache; >> static int __init hugetlbpage_init(void) >> { >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >> index 0ed8e41..2927200 100644 >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -361,6 +361,7 @@ int huge_add_to_page_cache(struct page *page, >> struct address_space *mapping, >> int __init alloc_bootmem_huge_page(struct hstate *h); >> >> void __init hugetlb_bad_size(void); >> +void __init hugetlb_bad_default_size(void); >> void __init hugetlb_add_hstate(unsigned order); >> struct hstate *size_to_hstate(unsigned long size); >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index bc48ee7..3c24266 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -54,6 +54,7 @@ >> static unsigned long __initdata default_hstate_max_huge_pages; >> static unsigned long __initdata default_hstate_size; >> static bool __initdata parsed_valid_hugepagesz = true; >> +static bool __initdata parsed_valid_default_hugepagesz = true; >> >> /* >> * Protects updates to hugepage_freelists, hugepage_activelist, >> nr_huge_pages, >> @@ -2804,6 +2805,12 @@ void __init hugetlb_bad_size(void) >> parsed_valid_hugepagesz = false; >> } >> >> +/* Should be called on processing a default_hugepagesz=... option */ >> +void __init hugetlb_bad_default_size(void) >> +{ >> + parsed_valid_default_hugepagesz = false; >> +} >> + >> void __init hugetlb_add_hstate(unsigned int order) >> { >> struct hstate *h; >> @@ -2846,8 +2853,14 @@ static int __init hugetlb_nrpages_setup(char >> *s) >> * !hugetlb_max_hstate means we haven't parsed a hugepagesz= >> parameter yet, >> * so this hugepages= parameter goes to the "default hstate". >> */ >> - else if (!hugetlb_max_hstate) >> - mhp = &default_hstate_max_huge_pages; >> + else if (!hugetlb_max_hstate) { >> + if (!parsed_valid_default_hugepagesz) { >> + pr_warn("hugepages = %s cannot be allocated for " >> + "unsupported default_hugepagesz, ignoring\n", s); >> + parsed_valid_default_hugepagesz = true; >> + } else >> + mhp = &default_hstate_max_huge_pages; >> + } >> else >> mhp = &parsed_hstate->max_huge_pages; >> >> > > My compiler tells me, > > mm/hugetlb.c: In function ‘hugetlb_nrpages_setup’: > mm/hugetlb.c:2873:8: warning: ‘mhp’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > > You have added a way of getting out of that big if/else if statement > without > setting mhp. mhp will be examined later in the code, so this is indeed > a bug. > > Like Aneesh, I am not sure if there is great benefit in this patch. > > You added this change in functionality only for powerpc. IMO, it would > be > best if behavior was consistent in all architectures. So, if we change > it > for powerpc we may want to change everywhere.
Hi Mike,
Yes, the patch mentioned by Aneesh solves the issue.
Thanks
-- Victor Aoqui
| |