lkml.org 
[lkml]   [2017]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2017-08-15 00:10    [W:0.175 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site