Re: [PATCH v2 6/8] mm: hugetlb_vmemmap: improve hugetlb_vmemmap code readability

[Muchun Song]

On Tue, Jun 28, 2022 at 05:23:21PM -0700, Mike Kravetz wrote:
> On 06/28/22 17:22, Muchun Song wrote:
> > There is a discussion about the name of hugetlb_vmemmap_alloc/free in
> > thread [1].  The suggestion suggested by David is rename "alloc/free"
> > to "optimize/restore" to make functionalities clearer to users,
> > "optimize" means the function will optimize vmemmap pages, while
> > "restore" means restoring its vmemmap pages discared before. This
> > commit does this.
> > 
> > Another discussion is the confusion RESERVE_VMEMMAP_NR isn't used
> > explicitly for vmemmap_addr but implicitly for vmemmap_end in
> > hugetlb_vmemmap_alloc/free.  David suggested we can compute what
> > hugetlb_vmemmap_init() does now at runtime.  We do not need to worry
> > for the overhead of computing at runtime since the calculation is
> > simple enough and those functions are not in a hot path.  This commit
> > has the following improvements:
> > 
> >   1) The function suffixed name ("optimize/restore") is more expressive.
> >   2) The logic becomes less weird in hugetlb_vmemmap_optimize/restore().
> >   3) The hugetlb_vmemmap_init() does not need to be exported anymore.
> >   4) A ->optimize_vmemmap_pages field in struct hstate is killed.
> >   5) There is only one place where checks is_power_of_2(sizeof(struct
> >      page)) instead of two places.
> >   6) Add more comments for hugetlb_vmemmap_optimize/restore().
> >   7) For external users, hugetlb_optimize_vmemmap_pages() is used for
> >      detecting if the HugeTLB's vmemmap pages is optimizable originally.
> >      In this commit, it is killed and we introduce a new helper
> >      hugetlb_vmemmap_optimizable() to replace it.  The name is more
> >      expressive.
> > 
> > Link: https://lore.kernel.org/all/20220404074652.68024-2-songmuchun@xxxxxxxxxxxxx/ [1]
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > ---
> >  include/linux/hugetlb.h |   7 +--
> >  include/linux/sysctl.h  |   4 ++
> >  mm/hugetlb.c            |  15 ++---
> >  mm/hugetlb_vmemmap.c    | 143 ++++++++++++++++++++----------------------------
> >  mm/hugetlb_vmemmap.h    |  41 +++++++++-----
> >  5 files changed, 102 insertions(+), 108 deletions(-)
> 
> Thanks!  I like the removal of hugetlb_vmemmap_init and printing directly
> from report_hugepages.  Still need to look at your your command parsing
> patches.
>

Thanks Mike. I am also trying to think about if it is easy to split the
command parsing patch to some small patches for easily reviewing.
 
> > @@ -3191,8 +3191,10 @@ static void __init report_hugepages(void)
> >  		char buf[32];
> >  
> >  		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
> > -		pr_info("HugeTLB registered %s page size, pre-allocated %ld pages\n",
> > +		pr_info("HugeTLB: registered %s page size, pre-allocated %ld pages\n",
> >  			buf, h->free_huge_pages);
> > +		pr_info("HugeTLB: %d KiB vmemmap can be freed for a %s page\n",
> > +			hugetlb_vmemmap_optimizable_size(h) / SZ_1K, buf);
> >  	}
> >  }
> 
> My first thought was "Why report vmemmap freed pages if not enabled?".
> However, since it can be enabled at runtime it is best always print.
>

That's what I thought.

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

Thanks.
 
> -- 
> Mike Kravetz
>