[PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[Vaishali Thakkar]

When any unsupported hugepage size is specified, 'hugepagesz=' and
'hugepages=' should be ignored during command line parsing until any
supported hugepage size is found. But currently incorrect number of
hugepages are allocated when unsupported size is specified as it fails
to ignore the 'hugepages=' command.

Test case:

Note that this is specific to x86 architecture.

Boot the kernel with command line option 'hugepagesz=256M hugepages=X'.
After boot, dmesg output shows that X number of hugepages of the size 2M
is pre-allocated instead of 0.

So, to handle such command line options, introduce new routine
hugetlb_bad_size. The routine hugetlb_bad_size sets the global variable
parsed_valid_hugepagesz. We are using parsed_valid_hugepagesz to save the
state when unsupported hugepagesize is found so that we can ignore the
'hugepages=' parameters after that and then reset the variable when
supported hugepage size is found.

The routine hugetlb_bad_size can be called while setting 'hugepagesz='
parameter in an architecture specific code.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
The patch is having 2 checkpatch.pl warnings. I have just followed
the current code to maintain consistency. If we decide to silent
these warnings then may be we should silent those warnings as well.
I am fine with any option whichever works best for everyone else. 
---
 include/linux/hugetlb.h |  1 +
 mm/hugetlb.c            | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 7d953c2..e44c578 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -338,6 +338,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
 /* arch callback */
 int __init alloc_bootmem_huge_page(struct hstate *h);
 
+void __init hugetlb_bad_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 06058ea..44fae6a 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -51,6 +51,7 @@ __initdata LIST_HEAD(huge_boot_pages);
 static struct hstate * __initdata parsed_hstate;
 static unsigned long __initdata default_hstate_max_huge_pages;
 static unsigned long __initdata default_hstate_size;
+static bool __initdata parsed_valid_hugepagesz = true;
 
 /*
  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
@@ -2659,6 +2660,11 @@ static int __init hugetlb_init(void)
 subsys_initcall(hugetlb_init);
 
 /* 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;
@@ -2691,11 +2697,17 @@ static int __init hugetlb_nrpages_setup(char *s)
 	unsigned long *mhp;
 	static unsigned long *last_mhp;
 
+	if (!parsed_valid_hugepagesz) {
+		pr_warn("hugepages = %s preceded by "
+			"an unsupported hugepagesz, ignoring\n", s);
+		parsed_valid_hugepagesz = true;
+		return 1;
+	}
 	/*
 	 * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
 	 * so this hugepages= parameter goes to the "default hstate".
 	 */
-	if (!hugetlb_max_hstate)
+	else if (!hugetlb_max_hstate)
 		mhp = &default_hstate_max_huge_pages;
 	else
 		mhp = &parsed_hstate->max_huge_pages;
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[SeongJae Park]

Hello Vaishali,


The patch looks good to me.  However, I have few trivial questions.

On Tue, 22 Mar 2016, Vaishali Thakkar wrote:

> When any unsupported hugepage size is specified, 'hugepagesz=' and
> 'hugepages=' should be ignored during command line parsing until any
> supported hugepage size is found. But currently incorrect number of
> hugepages are allocated when unsupported size is specified as it fails
> to ignore the 'hugepages=' command.
>
> Test case:
>
> Note that this is specific to x86 architecture.
>
> Boot the kernel with command line option 'hugepagesz=256M hugepages=X'.
> After boot, dmesg output shows that X number of hugepages of the size 2M
> is pre-allocated instead of 0.
>
> So, to handle such command line options, introduce new routine
> hugetlb_bad_size. The routine hugetlb_bad_size sets the global variable
> parsed_valid_hugepagesz. We are using parsed_valid_hugepagesz to save the
> state when unsupported hugepagesize is found so that we can ignore the
> 'hugepages=' parameters after that and then reset the variable when
> supported hugepage size is found.
>
> The routine hugetlb_bad_size can be called while setting 'hugepagesz='
> parameter in an architecture specific code.
>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> The patch is having 2 checkpatch.pl warnings. I have just followed
> the current code to maintain consistency. If we decide to silent
> these warnings then may be we should silent those warnings as well.
> I am fine with any option whichever works best for everyone else.
> ---
> include/linux/hugetlb.h |  1 +
> mm/hugetlb.c            | 14 +++++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 7d953c2..e44c578 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -338,6 +338,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
> /* arch callback */
> int __init alloc_bootmem_huge_page(struct hstate *h);
>
> +void __init hugetlb_bad_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 06058ea..44fae6a 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -51,6 +51,7 @@ __initdata LIST_HEAD(huge_boot_pages);
> static struct hstate * __initdata parsed_hstate;
> static unsigned long __initdata default_hstate_max_huge_pages;
> static unsigned long __initdata default_hstate_size;
> +static bool __initdata parsed_valid_hugepagesz = true;
>
> /*
>  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
> @@ -2659,6 +2660,11 @@ static int __init hugetlb_init(void)
> subsys_initcall(hugetlb_init);
>
> /* 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;
> @@ -2691,11 +2697,17 @@ static int __init hugetlb_nrpages_setup(char *s)
> 	unsigned long *mhp;
> 	static unsigned long *last_mhp;
>
> +	if (!parsed_valid_hugepagesz) {
> +		pr_warn("hugepages = %s preceded by "
> +			"an unsupported hugepagesz, ignoring\n", s);

How about concatenating the format string?  `CodingStyle` now suggests to
_never_ break every user-visible strings.

> +		parsed_valid_hugepagesz = true;
> +		return 1;
> +	}
> 	/*
> 	 * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
> 	 * so this hugepages= parameter goes to the "default hstate".
> 	 */
> -	if (!hugetlb_max_hstate)
> +	else if (!hugetlb_max_hstate)

Because the upper `if` statement will do `return`, above change looks not
significantly necessary.  Is this intended?

> 		mhp = &default_hstate_max_huge_pages;
> 	else
> 		mhp = &parsed_hstate->max_huge_pages;
> -- 
> 2.1.4
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

Re: [PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[Naoya Horiguchi]

On Tue, Mar 22, 2016 at 03:30:43PM +0530, Vaishali Thakkar wrote:
> When any unsupported hugepage size is specified, 'hugepagesz=' and
> 'hugepages=' should be ignored during command line parsing until any
> supported hugepage size is found. But currently incorrect number of
> hugepages are allocated when unsupported size is specified as it fails
> to ignore the 'hugepages=' command.
> 
> Test case:
> 
> Note that this is specific to x86 architecture.
> 
> Boot the kernel with command line option 'hugepagesz=256M hugepages=X'.
> After boot, dmesg output shows that X number of hugepages of the size 2M
> is pre-allocated instead of 0.
> 
> So, to handle such command line options, introduce new routine
> hugetlb_bad_size. The routine hugetlb_bad_size sets the global variable
> parsed_valid_hugepagesz. We are using parsed_valid_hugepagesz to save the
> state when unsupported hugepagesize is found so that we can ignore the
> 'hugepages=' parameters after that and then reset the variable when
> supported hugepage size is found.
> 
> The routine hugetlb_bad_size can be called while setting 'hugepagesz='
> parameter in an architecture specific code.

> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>

Maybe parsed_hstate can do what parsed_valid_hugepagesz does, but both
are __initdata so it's not a big deal.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

> ---
> The patch is having 2 checkpatch.pl warnings. I have just followed
> the current code to maintain consistency. If we decide to silent
> these warnings then may be we should silent those warnings as well.
> I am fine with any option whichever works best for everyone else. 
> ---
>  include/linux/hugetlb.h |  1 +
>  mm/hugetlb.c            | 14 +++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 7d953c2..e44c578 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -338,6 +338,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
>  /* arch callback */
>  int __init alloc_bootmem_huge_page(struct hstate *h);
>  
> +void __init hugetlb_bad_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 06058ea..44fae6a 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -51,6 +51,7 @@ __initdata LIST_HEAD(huge_boot_pages);
>  static struct hstate * __initdata parsed_hstate;
>  static unsigned long __initdata default_hstate_max_huge_pages;
>  static unsigned long __initdata default_hstate_size;
> +static bool __initdata parsed_valid_hugepagesz = true;
>  
>  /*
>   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
> @@ -2659,6 +2660,11 @@ static int __init hugetlb_init(void)
>  subsys_initcall(hugetlb_init);
>  
>  /* 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;
> @@ -2691,11 +2697,17 @@ static int __init hugetlb_nrpages_setup(char *s)
>  	unsigned long *mhp;
>  	static unsigned long *last_mhp;
>  
> +	if (!parsed_valid_hugepagesz) {
> +		pr_warn("hugepages = %s preceded by "
> +			"an unsupported hugepagesz, ignoring\n", s);
> +		parsed_valid_hugepagesz = true;
> +		return 1;
> +	}
>  	/*
>  	 * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
>  	 * so this hugepages= parameter goes to the "default hstate".
>  	 */
> -	if (!hugetlb_max_hstate)
> +	else if (!hugetlb_max_hstate)
>  		mhp = &default_hstate_max_huge_pages;
>  	else
>  		mhp = &parsed_hstate->max_huge_pages;
> -- 
> 2.1.4
> 
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[Mike Kravetz]

On 03/22/2016 03:00 AM, Vaishali Thakkar wrote:
> When any unsupported hugepage size is specified, 'hugepagesz=' and
> 'hugepages=' should be ignored during command line parsing until any
> supported hugepage size is found. But currently incorrect number of
> hugepages are allocated when unsupported size is specified as it fails
> to ignore the 'hugepages=' command.
> 
> Test case:
> 
> Note that this is specific to x86 architecture.
> 
> Boot the kernel with command line option 'hugepagesz=256M hugepages=X'.
> After boot, dmesg output shows that X number of hugepages of the size 2M
> is pre-allocated instead of 0.
> 
> So, to handle such command line options, introduce new routine
> hugetlb_bad_size. The routine hugetlb_bad_size sets the global variable
> parsed_valid_hugepagesz. We are using parsed_valid_hugepagesz to save the
> state when unsupported hugepagesize is found so that we can ignore the
> 'hugepages=' parameters after that and then reset the variable when
> supported hugepage size is found.
> 
> The routine hugetlb_bad_size can be called while setting 'hugepagesz='
> parameter in an architecture specific code.
> 
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> ---

Looks fine to me,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

> The patch is having 2 checkpatch.pl warnings. I have just followed
> the current code to maintain consistency. If we decide to silent
> these warnings then may be we should silent those warnings as well.
> I am fine with any option whichever works best for everyone else. 
> ---
>  include/linux/hugetlb.h |  1 +
>  mm/hugetlb.c            | 14 +++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 7d953c2..e44c578 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -338,6 +338,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
>  /* arch callback */
>  int __init alloc_bootmem_huge_page(struct hstate *h);
>  
> +void __init hugetlb_bad_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 06058ea..44fae6a 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -51,6 +51,7 @@ __initdata LIST_HEAD(huge_boot_pages);
>  static struct hstate * __initdata parsed_hstate;
>  static unsigned long __initdata default_hstate_max_huge_pages;
>  static unsigned long __initdata default_hstate_size;
> +static bool __initdata parsed_valid_hugepagesz = true;
>  
>  /*
>   * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
> @@ -2659,6 +2660,11 @@ static int __init hugetlb_init(void)
>  subsys_initcall(hugetlb_init);
>  
>  /* 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;
> @@ -2691,11 +2697,17 @@ static int __init hugetlb_nrpages_setup(char *s)
>  	unsigned long *mhp;
>  	static unsigned long *last_mhp;
>  
> +	if (!parsed_valid_hugepagesz) {
> +		pr_warn("hugepages = %s preceded by "
> +			"an unsupported hugepagesz, ignoring\n", s);
> +		parsed_valid_hugepagesz = true;
> +		return 1;
> +	}
>  	/*
>  	 * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
>  	 * so this hugepages= parameter goes to the "default hstate".
>  	 */
> -	if (!hugetlb_max_hstate)
> +	else if (!hugetlb_max_hstate)
>  		mhp = &default_hstate_max_huge_pages;
>  	else
>  		mhp = &parsed_hstate->max_huge_pages;
> 


-- 
Mike Kravetz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[Vaishali Thakkar]

On Wednesday 23 March 2016 04:57 AM, SeongJae Park wrote:
> Hello Vaishali,
>
>
> The patch looks good to me.  However, I have few trivial questions.
>
> On Tue, 22 Mar 2016, Vaishali Thakkar wrote:
>
>> When any unsupported hugepage size is specified, 'hugepagesz=' and
>> 'hugepages=' should be ignored during command line parsing until any
>> supported hugepage size is found. But currently incorrect number of
>> hugepages are allocated when unsupported size is specified as it fails
>> to ignore the 'hugepages=' command.
>>
>> Test case:
>>
>> Note that this is specific to x86 architecture.
>>
>> Boot the kernel with command line option 'hugepagesz=256M hugepages=X'.
>> After boot, dmesg output shows that X number of hugepages of the size 2M
>> is pre-allocated instead of 0.
>>
>> So, to handle such command line options, introduce new routine
>> hugetlb_bad_size. The routine hugetlb_bad_size sets the global variable
>> parsed_valid_hugepagesz. We are using parsed_valid_hugepagesz to save the
>> state when unsupported hugepagesize is found so that we can ignore the
>> 'hugepages=' parameters after that and then reset the variable when
>> supported hugepage size is found.
>>
>> The routine hugetlb_bad_size can be called while setting 'hugepagesz='
>> parameter in an architecture specific code.
>>
>> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
>> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> ---
>> The patch is having 2 checkpatch.pl warnings. I have just followed
>> the current code to maintain consistency. If we decide to silent
>> these warnings then may be we should silent those warnings as well.
>> I am fine with any option whichever works best for everyone else.
>> ---
>> include/linux/hugetlb.h |  1 +
>> mm/hugetlb.c            | 14 +++++++++++++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 7d953c2..e44c578 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -338,6 +338,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
>> /* arch callback */
>> int __init alloc_bootmem_huge_page(struct hstate *h);
>>
>> +void __init hugetlb_bad_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 06058ea..44fae6a 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -51,6 +51,7 @@ __initdata LIST_HEAD(huge_boot_pages);
>> static struct hstate * __initdata parsed_hstate;
>> static unsigned long __initdata default_hstate_max_huge_pages;
>> static unsigned long __initdata default_hstate_size;
>> +static bool __initdata parsed_valid_hugepagesz = true;
>>
>> /*
>>  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
>> @@ -2659,6 +2660,11 @@ static int __init hugetlb_init(void)
>> subsys_initcall(hugetlb_init);
>>
>> /* 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;
>> @@ -2691,11 +2697,17 @@ static int __init hugetlb_nrpages_setup(char *s)
>>     unsigned long *mhp;
>>     static unsigned long *last_mhp;
>>
>> +    if (!parsed_valid_hugepagesz) {
>> +        pr_warn("hugepages = %s preceded by "
>> +            "an unsupported hugepagesz, ignoring\n", s);
>
> How about concatenating the format string?  `CodingStyle` now suggests to
> _never_ break every user-visible strings.
>

As I said above, I just followed the pattern of the current code to maintain the
consistency. Probably a separate change would be good for solving all those
warnings. :)

>> +        parsed_valid_hugepagesz = true;
>> +        return 1;
>> +    }
>>     /*
>>      * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
>>      * so this hugepages= parameter goes to the "default hstate".
>>      */
>> -    if (!hugetlb_max_hstate)
>> +    else if (!hugetlb_max_hstate)
>
> Because the upper `if` statement will do `return`, above change looks not
> significantly necessary.  Is this intended?
>

I think above change is necessary for the cases like "hugepages=X" because in that
case the X hugepages of the default size [like 2M for x86] should be allocated.

>>         mhp = &default_hstate_max_huge_pages;
>>     else
>>         mhp = &parsed_hstate->max_huge_pages;
>> -- 
>> 2.1.4
>>
>> -- 
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>

-- 
Vaishali

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/hugetlb: Introduce hugetlb_bad_size

[SeongJae Park]

On Wed, 23 Mar 2016, Vaishali Thakkar wrote:

>
>
> On Wednesday 23 March 2016 04:57 AM, SeongJae Park wrote:
>> Hello Vaishali,
>>
>>
>> The patch looks good to me.  However, I have few trivial questions.
>>
>> On Tue, 22 Mar 2016, Vaishali Thakkar wrote:
>>
>>> When any unsupported hugepage size is specified, 'hugepagesz=' and
>>> 'hugepages=' should be ignored during command line parsing until any
>>> supported hugepage size is found. But currently incorrect number of
>>> hugepages are allocated when unsupported size is specified as it fails
>>> to ignore the 'hugepages=' command.
>>>
>>> Test case:
>>>
>>> Note that this is specific to x86 architecture.
>>>
>>> Boot the kernel with command line option 'hugepagesz=256M hugepages=X'.
>>> After boot, dmesg output shows that X number of hugepages of the size 2M
>>> is pre-allocated instead of 0.
>>>
>>> So, to handle such command line options, introduce new routine
>>> hugetlb_bad_size. The routine hugetlb_bad_size sets the global variable
>>> parsed_valid_hugepagesz. We are using parsed_valid_hugepagesz to save the
>>> state when unsupported hugepagesize is found so that we can ignore the
>>> 'hugepages=' parameters after that and then reset the variable when
>>> supported hugepage size is found.
>>>
>>> The routine hugetlb_bad_size can be called while setting 'hugepagesz='
>>> parameter in an architecture specific code.
>>>
>>> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
>>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
>>> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
>>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>>> ---
>>> The patch is having 2 checkpatch.pl warnings. I have just followed
>>> the current code to maintain consistency. If we decide to silent
>>> these warnings then may be we should silent those warnings as well.
>>> I am fine with any option whichever works best for everyone else.
>>> ---
>>> include/linux/hugetlb.h |  1 +
>>> mm/hugetlb.c            | 14 +++++++++++++-
>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>>> index 7d953c2..e44c578 100644
>>> --- a/include/linux/hugetlb.h
>>> +++ b/include/linux/hugetlb.h
>>> @@ -338,6 +338,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
>>> /* arch callback */
>>> int __init alloc_bootmem_huge_page(struct hstate *h);
>>>
>>> +void __init hugetlb_bad_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 06058ea..44fae6a 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -51,6 +51,7 @@ __initdata LIST_HEAD(huge_boot_pages);
>>> static struct hstate * __initdata parsed_hstate;
>>> static unsigned long __initdata default_hstate_max_huge_pages;
>>> static unsigned long __initdata default_hstate_size;
>>> +static bool __initdata parsed_valid_hugepagesz = true;
>>>
>>> /*
>>>  * Protects updates to hugepage_freelists, hugepage_activelist, nr_huge_pages,
>>> @@ -2659,6 +2660,11 @@ static int __init hugetlb_init(void)
>>> subsys_initcall(hugetlb_init);
>>>
>>> /* 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;
>>> @@ -2691,11 +2697,17 @@ static int __init hugetlb_nrpages_setup(char *s)
>>>     unsigned long *mhp;
>>>     static unsigned long *last_mhp;
>>>
>>> +    if (!parsed_valid_hugepagesz) {
>>> +        pr_warn("hugepages = %s preceded by "
>>> +            "an unsupported hugepagesz, ignoring\n", s);
>>
>> How about concatenating the format string?  `CodingStyle` now suggests to
>> _never_ break every user-visible strings.
>>
>
> As I said above, I just followed the pattern of the current code to maintain the
> consistency. Probably a separate change would be good for solving all those
> warnings. :)

Understood and agreed. :)


>
>>> +        parsed_valid_hugepagesz = true;
>>> +        return 1;
>>> +    }
>>>     /*
>>>      * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
>>>      * so this hugepages= parameter goes to the "default hstate".
>>>      */
>>> -    if (!hugetlb_max_hstate)
>>> +    else if (!hugetlb_max_hstate)
>>
>> Because the upper `if` statement will do `return`, above change looks not
>> significantly necessary.  Is this intended?
>>
>
> I think above change is necessary for the cases like "hugepages=X" because in that
> case the X hugepages of the default size [like 2M for x86] should be allocated.

Looks like my poor English made some confusion, sorry.  I was just saying
about the addition of `else` in the line, not whole change.

Because the upper `if` statement that catching wrong `hugepagesz=` case
does `return`, below statements will not be executed at all in the case.
So, the result will not be changed even if the `else` is not added though
the addition of `else` may help readability for someone.
That's why I said the change (addition of `else`) looks not significantly
necessary.

If I am missing something wrong, please let me know.  Or, if this question
bothers you, just ignore it because I also know that this is just a
trivial question. ;)


Thanks,
SeongJae Park

>
>>>         mhp = &default_hstate_max_huge_pages;
>>>     else
>>>         mhp = &parsed_hstate->max_huge_pages;
>>> --
>>> 2.1.4
>>>
>>> --
>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>> the body to majordomo@kvack.org.  For more info on Linux MM,
>>> see: http://www.linux-mm.org/ .
>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>>
>
> -- 
> Vaishali
>
>