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
>
