[PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc

[PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc

[Victor Aoqui]

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;
 
-- 
1.8.3.1

Re: [PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc

[Aneesh Kumar K.V]

Victor Aoqui <victora@linux.vnet.ibm.com> writes:

> 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>

I am still not sure about this. With current upstream we get

 PCI: Probing PCI hardware                                     
 PCI: Probing PCI hardware done                                                                                                                       
 HugeTLB: unsupported default_hugepagesz 2097152. Reverting to 16777216                                                                               
 HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages                                                                                         
 HugeTLB registered 16.0 GiB page size, pre-allocated 0 pages  

That warning is added by

d715cf804a0318e83c75c0a7abd1a4b9ce13e8da

Which should be good enough right ?

-aneesh

Re: [PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc

[Mike Kravetz]

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.
-- 
Mike Kravetz

Re: [PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc

[Victor Aoqui]

Em 2017-08-04 02:57, Aneesh Kumar K.V escreveu:
> Victor Aoqui <victora@linux.vnet.ibm.com> writes:
> 
>> 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>
> 
> I am still not sure about this. With current upstream we get
> 
>  PCI: Probing PCI hardware
>  PCI: Probing PCI hardware done
> 
> 
>  HugeTLB: unsupported default_hugepagesz 2097152. Reverting to
> 16777216
> 
>  HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages
> 
> 
>  HugeTLB registered 16.0 GiB page size, pre-allocated 0 pages
> 
> That warning is added by
> 
> d715cf804a0318e83c75c0a7abd1a4b9ce13e8da
> 
> Which should be good enough right ?
> 
> -aneesh

Hi Aneesh,

Sorry for the delay. I was on vacation.
Yes, that solves the issue. This patch was accepted when I was fixing 
the last version.
Sorry for the inconvenience.

-- 
Victor Aoqui

Re: [PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc

[Victor Aoqui]

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