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